From 56ef309802850f3bea2dd8639b564117aebf9056 Mon Sep 17 00:00:00 2001 From: Steve Ramage Date: Sun, 26 Jul 2026 16:03:15 +0000 Subject: [PATCH] refactor: compose the shared BOOLEAN combinator instead of inlining its spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Three of the new validators wrote parse_boolean()'s twelve spellings out by hand, two of them folded into a larger choice set so the boolean was no longer distinguishable from the enum it sat next to. To systemd these are separate branches, and keeping them separate in the grammar means a later pass — a formatter normalising yes/Yes, completion offering only the sensible half — can tell which is which from the grammar rather than by re-sniffing the text. ConditionVirtualization= AlternativeCombinator(names, BOOLEAN) DuplicateAddressDetection= AlternativeCombinator(families, deprecatedBoolean(...)) PreferredSource= BOOLEAN_FALSE, a named home for the false-only spellings config_parse_preferred_src accepts Ordering matters in both alternations and is now commented at each site: BOOLEAN matches a *prefix*, so on `none` — a real virtualization_table entry, and a DAD family name — it would take the leading `no` and strand `ne`, which the classic first-full-match engine cannot back out of. deprecatedBoolean() returns a FRESH terminal each call. FlexibleLiteralChoiceTerminal .deprecating() mutates in place, so reusing the shared BOOLEAN for DuplicateAddressDetection= would have attached its "For historical reasons" note to every boolean-valued setting in the plugin. DeprecationsTest now pins that isolation. New tests for three things raised in review that turned out to already hold, so they stay holding: `ConditionArchitecture=ppc64-le` and `=arm64-be` are not truncated to their shorter prefixes, `ConditionControlGroupController=cpuacct io` parses as two controllers rather than stopping at `cpu`, and ConditionFirstBoot=/ConditionACPower= still take every boolean spelling with the [|] [!] markers. FlexibleLiteralChoiceTerminal sorts its choices longest-first in its init block, so declaration order in these files is cosmetic — the tests say so explicitly to stop someone "fixing" it later. Co-Authored-By: Claude Opus 5 (1M context) --- .../ai/AddressAndNextHopFlagOptionValues.kt | 14 ++-- ...eUnitConditionVirtualizationOptionValue.kt | 15 +++-- .../ai/RouteAddressNextHopOptionValues.kt | 3 +- .../optionvalues/grammar/Combinators.kt | 45 ++++++++++++- .../ai/ConditionAndAssertInspectionTest.kt | 64 ++++++++++++++++++- .../ai/NetworkSectionInspectionTest.kt | 3 + .../optionvalues/grammar/DeprecationsTest.kt | 32 ++++++++++ 7 files changed, 160 insertions(+), 16 deletions(-) diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/AddressAndNextHopFlagOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/AddressAndNextHopFlagOptionValues.kt index 5393ad2..00a700f 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/AddressAndNextHopFlagOptionValues.kt +++ b/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), ) } } diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUnitConditionVirtualizationOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUnitConditionVirtualizationOptionValue.kt index eaf0e04..7537540 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUnitConditionVirtualizationOptionValue.kt +++ b/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 diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt index 808865a..41da2ec 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt +++ b/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() ) diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/Combinators.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/Combinators.kt index 5d10819..1128e57 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/Combinators.kt +++ b/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*") diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConditionAndAssertInspectionTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConditionAndAssertInspectionTest.kt index 84e346d..f668d8d 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConditionAndAssertInspectionTest.kt +++ b/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. [|] [!] , 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") } } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/NetworkSectionInspectionTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/NetworkSectionInspectionTest.kt index 345dc14..8681fd2 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/NetworkSectionInspectionTest.kt +++ b/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") } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt index e4aed28..e60b448 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt +++ b/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")) + } }