From 596c3e2342ca3d29f094d0021fa041e1d47a049b Mon Sep 17 00:00:00 2001 From: Steve Ramage Date: Sat, 24 Oct 2026 13:01:48 -0700 Subject: [PATCH] fix: keep validating after a line without '=' (Resolves #562) A line that is not a section, comment or key=value assignment, such as free text with several words, lexes as a KEY followed by BAD_CHARACTER tokens. The parser had no error recovery, so it gave up at that point and the rest of the file became one error element: no later property or section was validated. systemd logs "Missing '=', ignoring line." and carries on. Add recoverWhile rules to the grammar: after a (possibly broken) property, skip to the next token that can start a line (key, section or comment), and if junk appears where a line item should start, skip to the next section header. The bad line is still reported as a missing separator. Found by comparing the plugin with systemd's own parsers over the Debian unit corpus: the kf6/ksyntax-highlighting test units have free text in an [X-...] section, after which several hundred invalid values went unreported. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../generated/UnitFileParser.java | 42 +++++++++-- .../grammar/SystemdUnitFile.bnf | 12 +++- ...validValueAfterLineWithoutSeparatorTest.kt | 70 +++++++++++++++++++ .../parser/UnitFileParserTest.kt | 34 +++++++++ 4 files changed, 151 insertions(+), 7 deletions(-) create mode 100644 src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/InvalidValueAfterLineWithoutSeparatorTest.kt diff --git a/src/main/gen/net/sjrx/intellij/plugins/systemdunitfiles/generated/UnitFileParser.java b/src/main/gen/net/sjrx/intellij/plugins/systemdunitfiles/generated/UnitFileParser.java index e5cf505c..60516c3f 100644 --- a/src/main/gen/net/sjrx/intellij/plugins/systemdunitfiles/generated/UnitFileParser.java +++ b/src/main/gen/net/sjrx/intellij/plugins/systemdunitfiles/generated/UnitFileParser.java @@ -57,14 +57,13 @@ static boolean key_(PsiBuilder builder_, int level_) { // key_ separator_ value? public static boolean property(PsiBuilder builder_, int level_) { if (!recursion_guard_(builder_, level_, "property")) return false; - if (!nextTokenIs(builder_, KEY)) return false; boolean result_, pinned_; - Marker marker_ = enter_section_(builder_, level_, _NONE_, PROPERTY, null); + Marker marker_ = enter_section_(builder_, level_, _NONE_, PROPERTY, ""); result_ = key_(builder_, level_ + 1); pinned_ = result_; // pin = 1 result_ = result_ && report_error_(builder_, separator_(builder_, level_ + 1)); result_ = pinned_ && property_2(builder_, level_ + 1) && result_; - exit_section_(builder_, level_, marker_, result_, pinned_, null); + exit_section_(builder_, level_, marker_, result_, pinned_, UnitFileParser::property_recover); return result_ || pinned_; } @@ -75,6 +74,27 @@ private static boolean property_2(PsiBuilder builder_, int level_) { return true; } + /* ********************************************************** */ + // !(KEY | SECTION | COMMENT) + static boolean property_recover(PsiBuilder builder_, int level_) { + if (!recursion_guard_(builder_, level_, "property_recover")) return false; + boolean result_; + Marker marker_ = enter_section_(builder_, level_, _NOT_); + result_ = !property_recover_0(builder_, level_ + 1); + exit_section_(builder_, level_, marker_, result_, false, null); + return result_; + } + + // KEY | SECTION | COMMENT + private static boolean property_recover_0(PsiBuilder builder_, int level_) { + if (!recursion_guard_(builder_, level_, "property_recover_0")) return false; + boolean result_; + result_ = consumeToken(builder_, KEY); + if (!result_) result_ = consumeToken(builder_, SECTION); + if (!result_) result_ = consumeToken(builder_, COMMENT); + return result_; + } + /* ********************************************************** */ // SECTION static boolean section_(PsiBuilder builder_, int level_) { @@ -85,13 +105,12 @@ static boolean section_(PsiBuilder builder_, int level_) { // section_ section_items* public static boolean section_groups(PsiBuilder builder_, int level_) { if (!recursion_guard_(builder_, level_, "section_groups")) return false; - if (!nextTokenIs(builder_, "
", SECTION)) return false; boolean result_, pinned_; Marker marker_ = enter_section_(builder_, level_, _NONE_, SECTION_GROUPS, "
"); result_ = section_(builder_, level_ + 1); pinned_ = result_; // pin = 1 result_ = result_ && section_groups_1(builder_, level_ + 1); - exit_section_(builder_, level_, marker_, result_, pinned_, null); + exit_section_(builder_, level_, marker_, result_, pinned_, UnitFileParser::section_recover); return result_ || pinned_; } @@ -112,8 +131,21 @@ static boolean section_items(PsiBuilder builder_, int level_) { if (!recursion_guard_(builder_, level_, "section_items")) return false; if (!nextTokenIs(builder_, "", COMMENT, KEY)) return false; boolean result_; + Marker marker_ = enter_section_(builder_); result_ = property(builder_, level_ + 1); if (!result_) result_ = comment_(builder_, level_ + 1); + exit_section_(builder_, marker_, null, result_); + return result_; + } + + /* ********************************************************** */ + // !SECTION + static boolean section_recover(PsiBuilder builder_, int level_) { + if (!recursion_guard_(builder_, level_, "section_recover")) return false; + boolean result_; + Marker marker_ = enter_section_(builder_, level_, _NOT_); + result_ = !consumeToken(builder_, SECTION); + exit_section_(builder_, level_, marker_, result_, false, null); return result_; } diff --git a/src/main/resources/net/sjrx/intellij/plugins/systemdunitfiles/grammar/SystemdUnitFile.bnf b/src/main/resources/net/sjrx/intellij/plugins/systemdunitfiles/grammar/SystemdUnitFile.bnf index 9fd34b2e..e0e69481 100644 --- a/src/main/resources/net/sjrx/intellij/plugins/systemdunitfiles/grammar/SystemdUnitFile.bnf +++ b/src/main/resources/net/sjrx/intellij/plugins/systemdunitfiles/grammar/SystemdUnitFile.bnf @@ -19,14 +19,22 @@ unitFile ::= section_items* section_groups* -section_groups ::= section_ section_items* { name="section header" pin=1 } +section_groups ::= section_ section_items* { name="section header" pin=1 recoverWhile=section_recover } private section_items ::= property | comment_ -property ::= key_ separator_ value? { pin=1 } +property ::= key_ separator_ value? { pin=1 recoverWhile=property_recover } value ::= (CONTINUING_VALUE| comment_ )* COMPLETED_VALUE +// Error recovery (#562). A line that is not a section, comment or key=value (e.g. "some free text") lexes as a KEY +// followed by BAD_CHARACTERs. Without recovery the parser gave up there and the rest of the file became one error +// element, so nothing after it was validated. systemd itself logs "Missing '='" and carries on with the next line. +// After a (possibly broken) property, skip to the next token that can start a line item; if junk appears where an +// item should start, skip to the next section header. +private property_recover ::= !(KEY | SECTION | COMMENT) +private section_recover ::= !SECTION + // Single rules for tokens so that I can alias them for better error handling shown above. private comment_ ::= COMMENT {name="comment" } private section_ ::= SECTION diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/InvalidValueAfterLineWithoutSeparatorTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/InvalidValueAfterLineWithoutSeparatorTest.kt new file mode 100644 index 00000000..4cf8ea17 --- /dev/null +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/InvalidValueAfterLineWithoutSeparatorTest.kt @@ -0,0 +1,70 @@ +package net.sjrx.intellij.plugins.systemdunitfiles.inspections + +import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest + +/** + * A line that is not a section, comment or key=value must not stop validation of the lines after it (#562). + * systemd logs "Missing '=', ignoring line." and keeps going; the plugin used to stop validating the rest of the file. + */ +class InvalidValueAfterLineWithoutSeparatorTest : AbstractUnitFileTest() { + + private fun invalidValueHighlightTexts(fileName: String, file: String): List { + setupFileInEditor(fileName, file) + enableInspection(InvalidValueInspection::class.java) + return myFixture.doHighlighting().filter { it.inspectionToolId == "InvalidValue" }.map { it.text } + } + + fun testValuesAfterAFreeTextLineInTheSameSectionAreStillValidated() { + // language="unit file (systemd)" + val file = """ + [Service] + Nice=abc + this line has no equals + Nice=def + """.trimIndent() + + assertEquals(listOf("abc", "def"), invalidValueHighlightTexts("file.service", file)) + } + + fun testLaterSectionsAfterAFreeTextLineAreStillValidated() { + // The shape that hid ~300 invalid values in the kf6-syntax-highlighting test unit: free text inside an + // extension section, followed by more sections. + // language="unit file (systemd)" + val file = """ + [X-Unit] + name = value + Some text in an extension section. + The extension section ends with the next section header. + + [Service] + Nice=abc + [Unit] + StartLimitIntervalSec=invalid + """.trimIndent() + + assertEquals(listOf("abc", "invalid"), invalidValueHighlightTexts("file.service", file)) + } + + fun testFreeTextBeforeTheFirstSectionDoesNotStopValidation() { + // language="unit file (systemd)" + val file = """ + some free text here + [Service] + Nice=abc + """.trimIndent() + + assertEquals(listOf("abc"), invalidValueHighlightTexts("file.service", file)) + } + + fun testValidFileIsUnaffected() { + // language="unit file (systemd)" + val file = """ + [Service] + Nice=5 + [Unit] + StartLimitIntervalSec=10 + """.trimIndent() + + assertEquals(emptyList(), invalidValueHighlightTexts("file.service", file)) + } +} diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/parser/UnitFileParserTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/parser/UnitFileParserTest.kt index 0d11c695..16346636 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/parser/UnitFileParserTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/parser/UnitFileParserTest.kt @@ -2,6 +2,9 @@ package net.sjrx.intellij.plugins.systemdunitfiles.parser import com.intellij.testFramework.ParsingTestCase import junit.framework.TestCase +import com.intellij.psi.PsiErrorElement +import com.intellij.psi.util.PsiTreeUtil +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileProperty import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileSectionGroups import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileValueType @@ -769,6 +772,37 @@ KeyThree=value 2\ */assertSameLines(expectedPsiTree, parseTree) } + fun testLineWithoutSeparatorDoesNotSwallowTheRestOfTheFile() { + // A free-text line (several words, no '=') used to make the parser give up, turning everything after it into + // one error element (#562). It must stay a local error and the following properties and sections must parse. + val sourceCode = "[Service]\nNice=abc\nthis line has no equals\nNice=def\n[Unit]\nDescription=x\n" + + val file = createPsiFile("a", sourceCode) + ensureParsed(file) + TestCase.assertEquals(sourceCode, file.text) + + val sections = PsiTreeUtil.getChildrenOfTypeAsList(file, UnitFileSectionGroups::class.java) + TestCase.assertEquals(listOf("[Service]", "[Unit]"), sections.map { it.sectionName.let { n -> "[$n]" } }) + + val properties = PsiTreeUtil.findChildrenOfType(file, UnitFileProperty::class.java).map { it.key to it.valueText } + TestCase.assertEquals(listOf("Nice" to "abc", "this" to null, "Nice" to "def", "Description" to "x"), properties) + + val errors = PsiTreeUtil.findChildrenOfType(file, PsiErrorElement::class.java) + TestCase.assertEquals(1, errors.size) + TestCase.assertTrue(errors.first().errorDescription.contains("key-value separator")) + } + + fun testLineWithoutSeparatorBeforeFirstSectionDoesNotSwallowTheRestOfTheFile() { + val sourceCode = "some free text here\n[Service]\nNice=abc\n" + + val file = createPsiFile("a", sourceCode) + ensureParsed(file) + + val properties = PsiTreeUtil.findChildrenOfType(file, UnitFileProperty::class.java).map { it.key to it.valueText } + TestCase.assertEquals(listOf("some" to null, "Nice" to "abc"), properties) + TestCase.assertEquals(1, PsiTreeUtil.getChildrenOfTypeAsList(file, UnitFileSectionGroups::class.java).size) + } + /** * Converts the source code supplied as an argument to a parse tree. *