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")) + } }