refactor: compose the shared BOOLEAN combinator instead of inlining i…

src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/AddressAndNextHopFlagOptionValues.kt

@@ -1,7 +1,9 @@
 package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai
 
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.deprecatedBoolean
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.FlexibleLiteralChoiceTerminal
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.LiteralChoiceTerminal
@@ -72,13 +74,11 @@ class ConfigParseAddressSectionDadOptionValue : SimpleGrammarOptionValues(
             "For historical reasons a boolean here means the opposite of what it looks like: " +
             "yes means none and no means both. Please use 'both', 'ipv4', 'ipv6' or 'none' instead."
 
-        val DAD = FlexibleLiteralChoiceTerminal(
-            "none", "both", "ipv4", "ipv6",
-            // parse_boolean() spellings, all deprecated.
-            "1", "yes", "y", "true", "t", "on", "0", "no", "n", "false", "f", "off",
-        ).deprecating(
-            listOf("1", "yes", "y", "true", "t", "on", "0", "no", "n", "false", "f", "off")
-                .associateWith { HISTORICAL }
+        // The family names come first: a boolean terminal matches a prefix of the value, so it would
+        // otherwise take the leading "no" out of "none" and strand "ne".
+        val DAD = AlternativeCombinator(
+            FlexibleLiteralChoiceTerminal("none", "both", "ipv4", "ipv6"),
+            deprecatedBoolean(HISTORICAL),
         )
     }
 }

src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUnitConditionVirtualizationOptionValue.kt

@@ -1,6 +1,8 @@
 package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai
 
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.FlexibleLiteralChoiceTerminal
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.conditionString
 
@@ -24,17 +26,20 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram
  * `private-users`, any boolean parse_boolean() understands, the categories `vm` and `container`, and
  * finally any id in virtualization_table (src/basic/virt.c).
  *
- * The names are folded into one terminal rather than an alternation so that the whole value is one
- * token — that keeps error localization and completion pointing at the value itself.
+ * The boolean is kept as its own [BOOLEAN] alternative rather than folded into the name list: to
+ * systemd these really are two different branches, and a later formatting or completion pass can only
+ * tell "this span is a boolean" from "this span is a virtualization id" if the grammar says so.
+ *
+ * [BOOLEAN] has to come second. It matches a prefix of the value, so on `none` — a real entry in
+ * virtualization_table — it would otherwise match the leading `no` and strand `ne`, and under the
+ * classic engine AlternativeCombinator never backtracks out of a branch that matched.
  */
 class ConfigParseUnitConditionVirtualizationOptionValue : SimpleGrammarOptionValues(
     "config_parse_unit_condition_string",
-    conditionString(VIRTUALIZATION)
+    conditionString(AlternativeCombinator(VIRTUALIZATION, BOOLEAN))
 ) {
     companion object {
         private val VIRTUALIZATION = FlexibleLiteralChoiceTerminal(
-            // parse_boolean()
-            "1", "yes", "y", "true", "t", "on", "0", "no", "n", "false", "f", "off",
             // categories, plus the userns special case
             "vm", "container", "private-users",
             // virtualization_table — VMs

src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt

@@ -3,6 +3,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN_FALSE
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.FlexibleLiteralChoiceTerminal
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IPV4_ADDR
@@ -56,7 +57,7 @@ class ConfigParseRoutePreferredSourceOptionValue : SimpleGrammarOptionValues(
     SequenceCombinator(
         AlternativeCombinator(
             IP_ADDR,
-            FlexibleLiteralChoiceTerminal("0", "no", "n", "false", "f", "off"),
+            BOOLEAN_FALSE,
         ),
         EOF()
     )

src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/Combinators.kt

@@ -23,7 +23,50 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.Simp
  *    authority for which (parser, ltype) pairs exist and which keys use them.
  */
 
-val BOOLEAN = FlexibleLiteralChoiceTerminal("1", "yes", "y", "true", "t", "on", "0", "no", "n", "false", "f", "off")
+/**
+ * The spellings parse_boolean() accepts (src/basic/parse-util.c). Kept as an array so a caller that
+ * needs its *own* boolean terminal can build one without reusing [BOOLEAN] — see [deprecatedBoolean].
+ */
+val BOOLEAN_SPELLINGS = arrayOf("1", "yes", "y", "true", "t", "on", "0", "no", "n", "false", "f", "off")
+
+/**
+ * A boolean, as parse_boolean() reads it.
+ *
+ * Prefer composing this as its own alternative over inlining the twelve spellings into a larger choice
+ * set. A setting that takes "a boolean or one of these names" is two distinct things to systemd, and
+ * keeping them separate means a later pass — a formatter normalising `yes`/`Yes`, say, or completion
+ * offering only the sensible half — can tell which is which from the grammar instead of re-sniffing
+ * the text.
+ *
+ * Watch the ordering when you do: this terminal matches a *prefix* of the value, so on
+ * `ConditionVirtualization=none` it would happily match the leading `no` and strand `ne`. Under the
+ * classic engine AlternativeCombinator commits to the first branch that matches and never backtracks,
+ * so the names have to come first.
+ *
+ * Do NOT call [FlexibleLiteralChoiceTerminal.deprecating] on this instance — it mutates in place and
+ * this one is shared across every validator.
+ */
+val BOOLEAN = FlexibleLiteralChoiceTerminal(*BOOLEAN_SPELLINGS)
+
+/**
+ * Only the *false* half of [BOOLEAN_SPELLINGS].
+ *
+ * A few settings reach parse_boolean() but act on the result only when it is false, letting a true-ish
+ * spelling fall through to a later branch that then rejects it — config_parse_preferred_src is the
+ * example: `PreferredSource=no` forbids a DHCP-supplied source, while `PreferredSource=yes` is simply
+ * not an address.
+ */
+val BOOLEAN_FALSE = FlexibleLiteralChoiceTerminal("0", "no", "n", "false", "f", "off")
+
+/**
+ * A fresh boolean terminal with every spelling marked deprecated for [reason].
+ *
+ * For settings that still accept a boolean for backwards compatibility but tell you not to use one.
+ * Returns a new instance each call, because `deprecating()` mutates the terminal it is called on and
+ * the shared [BOOLEAN] must not be poisoned.
+ */
+fun deprecatedBoolean(reason: String): FlexibleLiteralChoiceTerminal =
+  FlexibleLiteralChoiceTerminal(*BOOLEAN_SPELLINGS).deprecating(BOOLEAN_SPELLINGS.associateWith { reason })
 val BYTES = RegexTerminal("[0-9]+[a-zA-Z]*\\s*", "[0-9]+[KMGT]?\\s*")
 val DEVICE = RegexTerminal("\\S+\\s*", "/[^\\u0000. ]+\\s*")
 val IOPS = RegexTerminal("[0-9]+[a-zA-Z]*\\s*", "[0-9]+[KMGT]?\\s*")

src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConditionAndAssertInspectionTest.kt

@@ -136,6 +136,24 @@ class ConditionAndAssertInspectionTest : AbstractUnitFileTest() {
     )
   }
 
+  @Test
+  fun testArchitectureMatchesTheLongestNameNotThePrefix() {
+    // Several table entries are prefixes of others (ppc64 / ppc64-le, arm64 / arm64-be, mips / mips64).
+    // FlexibleLiteralChoiceTerminal sorts its choices longest-first in its init block, so the order the
+    // names are written in below is cosmetic and the classic engine can't stop on a short prefix.
+    assertAccepted(
+      "ConditionArchitecture=ppc64",
+      "ConditionArchitecture=ppc64-le",
+      "ConditionArchitecture=arm64",
+      "ConditionArchitecture=arm64-be",
+      "ConditionArchitecture=mips",
+      "ConditionArchitecture=mips64",
+      "ConditionArchitecture=mips64-le",
+      "ConditionArchitecture=arc",
+      "ConditionArchitecture=arc-be",
+    )
+  }
+
   @Test
   fun testArchitectureRejectsUnknownAndLists() {
     assertRejected("ConditionArchitecture=x86_64")   // the table spells it with a hyphen
@@ -163,6 +181,21 @@ class ConditionAndAssertInspectionTest : AbstractUnitFileTest() {
     )
   }
 
+  @Test
+  fun testVirtualizationBooleanDoesNotShadowTheNames() {
+    // BOOLEAN is its own alternative here, and it matches a prefix -- so it must be tried after the
+    // names, or "none" would be read as the boolean "no" followed by a stray "ne".
+    assertAccepted(
+      "ConditionVirtualization=none",
+      "ConditionVirtualization=no",
+      "ConditionVirtualization=n",
+      "ConditionVirtualization=off",
+      "ConditionVirtualization=openvz",
+      "ConditionVirtualization=t",
+      "ConditionVirtualization=1",
+    )
+  }
+
   @Test
   fun testVirtualizationRejectsUnknownAndLists() {
     assertRejected("ConditionVirtualization=invalid")
@@ -258,6 +291,19 @@ class ConditionAndAssertInspectionTest : AbstractUnitFileTest() {
     )
   }
 
+  @Test
+  fun testControlGroupControllerListsMatchTheLongestControllerName() {
+    // cg_mask_from_string splits on whitespace, and "cpu" is a prefix of both "cpuacct" and "cpuset".
+    // The longest-first sort inside the terminal is what stops the first word of `cpuacct io` being
+    // read as "cpu" and the rest being reported as garbage.
+    assertAccepted(
+      "ConditionControlGroupController=cpuacct io",
+      "ConditionControlGroupController=cpuset cpu",
+      "ConditionControlGroupController=cpu cpuacct cpuset io blkio memory devices pids",
+      "ConditionControlGroupController=bpf-firewall bpf-devices",
+    )
+  }
+
   @Test
   fun testControlGroupControllerRejectsUnknownNames() {
     assertRejected("ConditionControlGroupController=invalid")
@@ -287,12 +333,26 @@ class ConditionAndAssertInspectionTest : AbstractUnitFileTest() {
   // ------------------------------------------------------------------ boolean conditions
 
   @Test
-  fun testBooleanConditionsStillWork() {
+  fun testBooleanConditionsTakeEverySpellingWithTheMarkers() {
+    // "Takes a boolean argument" -- systemd.unit(5), for both of these. The grammar is the shared
+    // conditionString(BOOLEAN), i.e. [|] [!] <boolean>, which is what this validator has always
+    // accepted; only the spelling of the marker prefix changed, to avoid an error range running past
+    // the end of the value on inputs like `!!yes`.
     assertAccepted(
       "ConditionFirstBoot=yes",
-      "ConditionACPower=true",
+      "ConditionFirstBoot=no",
+      "ConditionFirstBoot=1",
+      "ConditionFirstBoot=0",
+      "ConditionFirstBoot=t",
+      "ConditionFirstBoot=off",
       "AssertFirstBoot=|false",
+      "AssertFirstBoot=!true",
+      "AssertFirstBoot=|! true",
+      "ConditionACPower=true",
+      "AssertACPower=|yes",
     )
     assertRejected("ConditionFirstBoot=sometimes")
+    assertRejected("ConditionACPower=maybe")
+    assertRejected("ConditionFirstBoot=yes no")
   }
 }

src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/NetworkSectionInspectionTest.kt

@@ -167,6 +167,9 @@ class NetworkSectionInspectionTest : AbstractUnitFileTest() {
       "RouteMetric=128",
     )
     assertRejected("f.network", "[Address]\nAddPrefixRoute=bogus\n")
+    // Same prefix hazard as ConditionVirtualization: the deprecated boolean must not eat the "no"
+    // out of "none".
+    assertAccepted("f.network", "[Address]", "DuplicateAddressDetection=none", "DuplicateAddressDetection=n")
     assertRejected("f.network", "[Address]\nRouteMetric=hoge\n")
   }
 

src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt

@@ -1,7 +1,10 @@
 package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar
 
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai.ConfigParseAddressFamiliesOptionValue
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai.ConfigParseAddressSectionDadOptionValue
 import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai.ConfigParseIpMasqueradeOptionValue
+import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai.ConfigParseUnitConditionStringOptionValue
+import org.junit.Assert.assertFalse
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Test
@@ -43,4 +46,33 @@ class DeprecationsTest {
     assertTrue(ipMasquerade.deprecatedTokens("ipv4").isEmpty())
     assertTrue(ipMasquerade.deprecatedTokens("both").isEmpty())
   }
+
+  @Test
+  fun testDuplicateAddressDetectionBooleansAreDeprecatedButAccepted() {
+    // config_parse_address_dad tries parse_boolean() first and accepts the result with a
+    // "For historical reasons" warning, so these are valid values that deserve a nudge, not errors.
+    val dad = ConfigParseAddressSectionDadOptionValue().combinator
+    for (spelling in listOf("yes", "no", "1", "0", "off")) {
+      val deprecated = dad.deprecatedTokens(spelling)
+      assertEquals(spelling, 1, deprecated.size)
+      assertTrue(spelling, deprecated.single().message.contains("historical reasons"))
+    }
+    // The four family names are the spelling systemd asks for, so they carry no note...
+    for (name in listOf("none", "both", "ipv4", "ipv6")) {
+      assertTrue(name, dad.deprecatedTokens(name).isEmpty())
+    }
+  }
+
+  @Test
+  fun testDeprecatingOneBooleanTerminalDoesNotPoisonTheSharedOne() {
+    // deprecatedBoolean() has to hand back a FRESH terminal: FlexibleLiteralChoiceTerminal.deprecating
+    // mutates in place, so reusing the shared BOOLEAN would attach DuplicateAddressDetection='s note to
+    // every boolean-valued setting in the plugin.
+    assertTrue(BOOLEAN.deprecationFor("yes") == null)
+    val firstBoot = ConfigParseUnitConditionStringOptionValue().combinator
+    assertTrue(firstBoot.deprecatedTokens("yes").isEmpty())
+    assertTrue(firstBoot.deprecatedTokens("|! no").isEmpty())
+    // ...and the two really are separate instances, not the same object reached twice.
+    assertFalse(deprecatedBoolean("a") === deprecatedBoolean("b"))
+  }
 }