From 78fbf8f80c11238657526ffb8261eed0ad45b3fc Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Mon, 27 Jul 2026 03:08:06 +0200 Subject: [PATCH] fix(lint): make the reflow guards token-accurate instead of textual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transforms guard themselves against comments and raw strings before joining or rewriting a line, because pulling text up past a `//` buries it and reflowing a multi-line literal changes the string. Those guards were substring probes over the raw line, so they answered the wrong question: Line(n).contains("//") fires on // inside a string literal Line(n).contains("R\"") fires on the characters R" inside a literal, and MISSES a raw string opened on an earlier line Both misfire on this repo's own sources. "MARKER" ends in R", and any string mentioning a lint-disable directive contains //. Two wrapped call sites in tests/Lint were being left unjoined for exactly these reasons; they join now, and the results are in this commit. Replaced by LineHasComment() (added with the token layer) and a new LineHasMultiLineToken(), which reports whether any token actually covering that line spans a line boundary — a raw string or a block comment. Backed by a per-line bitmap derived from the token cache and invalidated with it. The guards themselves stay: joining across a real comment or a real multi-line literal is still unsafe, and there are tests for both. What changes is that they now fire on comments and literals rather than on the characters that spell them. format-concat gets narrower as a result. It used to refuse any line containing R" and ask for a manual fix; now only a literal that genuinely spans lines does that, because a single-line raw string reduces to R"…" in the stripped view, fails the plain-literal test, and travels through as an argument with its spelling intact. Co-Authored-By: Claude Opus 5 (1M context) --- implementations/Crafter.Build-Lint.cpp | 16 ++++++++++ interfaces/Crafter.Build-Clang.cppm | 11 +++++++ lint-rules.h | 27 +++++++++------- tests/HouseRules/main.cpp | 43 ++++++++++++++++++++++++++ tests/Lint/main.cpp | 6 ++-- 5 files changed, 87 insertions(+), 16 deletions(-) diff --git a/implementations/Crafter.Build-Lint.cpp b/implementations/Crafter.Build-Lint.cpp index b010b8a..8571e29 100644 --- a/implementations/Crafter.Build-Lint.cpp +++ b/implementations/Crafter.Build-Lint.cpp @@ -364,6 +364,21 @@ bool LintContext::LineHasComment(std::size_t line) { return std::ranges::any_of(onLine, [](const LintToken& t) { return t.kind == LintTokenKind::Comment; }); } +bool LintContext::LineHasMultiLineToken(std::size_t line) { + if (!spannedLineCache) { + std::vector spanned(lines.size(), false); + for (const LintToken& token : Tokens()) { + std::size_t crossed = static_cast(std::ranges::count(TokenText(token), '\n')); + if (crossed == 0) continue; + for (std::size_t n = token.line; n <= token.line + crossed && n <= spanned.size(); ++n) { + spanned[n - 1] = true; + } + } + spannedLineCache = std::move(spanned); + } + return line >= 1 && line <= spannedLineCache->size() && (*spannedLineCache)[line - 1]; +} + void LintContext::Report(std::size_t line, std::string message) { sink->push_back({file, line, activeRule, std::move(message)}); } @@ -427,6 +442,7 @@ void LintContext::SetContent(std::string newContent) { commentStrippedCache.reset(); suppressionsCache.reset(); // line numbers may have shifted — re-parse tokenCache.reset(); // offsets refer to the old buffer — re-lex + spannedLineCache.reset(); // derived from tokenCache } void Configuration::AddLintRule(std::string name, std::function check) { diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index e5a1f1f..be52f4b 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -214,6 +214,14 @@ export namespace Crafter { // True when a comment token starts on `line`. Replaces `Line(n).contains("//")`, // which false-positives on a `//` inside a string literal. CRAFTER_API bool LineHasComment(std::size_t line); + // True when `line` (1-based) is touched by a token that spans more + // than one line — a raw string or a block comment. A transform that + // joins, splits or rewrites such a line changes what is inside that + // token, so this is the guard to consult before reflowing anything. + // Replaces `Line(n).contains("R\"")`, which both misses raw strings + // opened on an earlier line and fires on the characters R" appearing + // inside an ordinary literal. + CRAFTER_API bool LineHasMultiLineToken(std::size_t line); // Driver wiring — set by RunLint before each check call. Not for rules. std::string activeRule; @@ -221,6 +229,9 @@ export namespace Crafter { std::optional commentStrippedCache; std::optional suppressionsCache; std::optional> tokenCache; + // Per-line flag, 0-based, for LineHasMultiLineToken. Derived from + // tokenCache and invalidated with it. + std::optional> spannedLineCache; }; // A named lint rule: `check` runs once per (rule, file) over the project's diff --git a/lint-rules.h b/lint-rules.h index 12461fd..3cf8c92 100644 --- a/lint-rules.h +++ b/lint-rules.h @@ -408,9 +408,12 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { lineStart += line.size() + 1; std::string_view trimmed = Trim(line); if (trimmed.starts_with('#')) continue; - // Raw strings defeat the comment stripper's quote tracking; any - // literal-+ pattern on such a line is a manual fix. - bool hasRaw = std::string_view(ctx.content).substr(lineOff, line.size()).contains("R\""); + // A chain is read within one line, so a literal that spans lines + // would be sliced in half by chainEnd. Single-line raw strings are + // fine: they reduce to R"…" in the stripped view, so they fail the + // "is a plain literal" test below and travel through as an + // argument, spelling and all. + bool spansLines = ctx.LineHasMultiLineToken(li + 1); std::size_t searchFrom = 0; while (searchFrom < line.size()) { @@ -427,8 +430,8 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { bool literalAdjacent = (leftEnd > 0 && line[leftEnd - 1] == '"') || (rightBegin < line.size() && line[rightBegin] == '"'); if (!literalAdjacent) continue; - if (hasRaw) { - ctx.Report(li + 1, "use std::format instead of string concatenation with + (raw-string line, fix manually)"); + if (spansLines) { + ctx.Report(li + 1, "use std::format instead of string concatenation with + (multi-line literal, fix manually)"); break; } @@ -607,7 +610,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { std::string lineStr(stripped[i]); std::smatch m; std::string_view raw = i + 1 <= ctx.lines.size() ? ctx.Line(i + 1) : std::string_view{}; - if (!raw.contains("//") && !ctx.Suppressed("single-declaration", i + 1) + if (!ctx.LineHasComment(i + 1) && !ctx.Suppressed("single-declaration", i + 1) && std::regex_match(lineStr, m, simpleMulti)) { std::string indent = m[1].str(), type = m[2].str(), decls = m[3].str(); std::size_t start = 0; @@ -644,7 +647,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { if (trimmed == "{" && !outLines.empty()) { std::string_view prevTrim = i > 0 ? Trim(stripped[i - 1]) : std::string_view{}; bool headerBefore = prevTrim.ends_with(')') || prevTrim == "else" || prevTrim == "do" || prevTrim == "try"; - bool hasComment = ctx.Line(i + 1).contains("//") || (i > 0 && ctx.Line(i).contains("//")); + bool hasComment = ctx.LineHasComment(i + 1) || (i > 0 && ctx.LineHasComment(i)); bool suppressed = ctx.Suppressed("brace-style", i) || ctx.Suppressed("brace-style", i + 1); if (headerBefore && !hasComment && !suppressed) { std::string& prev = outLines.back(); @@ -680,7 +683,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { if (i + 1 < ctx.lines.size() && std::regex_match(lineStr, ifHeader) && ParenDelta(stripped[i]) == 0) { std::string_view body = Trim(stripped[i + 1]); bool joinable = !body.empty() && body != "{" && !body.starts_with("if") && body.ends_with(';') - && !ctx.Line(i + 1).contains("//") && !ctx.Line(i + 2).contains("//") + && !ctx.LineHasComment(i + 1) && !ctx.LineHasComment(i + 2) && !ctx.Suppressed("if-single-line", i + 1) && !ctx.Suppressed("if-single-line", i + 2); std::string joined = std::string(ctx.Line(i + 1)); while (!joined.empty() && (joined.back() == ' ' || joined.back() == '\t')) joined.pop_back(); @@ -724,7 +727,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { std::string_view trimmed = Trim(stripped[i]); std::int64_t delta = ParenDelta(stripped[i]); bool candidate = delta > 0 && !trimmed.empty() && !trimmed.starts_with('#') - && !trimmed.ends_with('{') && !ctx.Line(i + 1).contains("//") && !ctx.Line(i + 1).contains("R\""); + && !trimmed.ends_with('{') && !ctx.LineHasComment(i + 1) && !ctx.LineHasMultiLineToken(i + 1); if (candidate) { std::string joined(ctx.Line(i + 1)); std::size_t j = i + 1; @@ -736,7 +739,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { // lambda body starts — leave those wrapped. bool closes = delta + ParenDelta(stripped[j]) <= 0; if (next.empty() || (next.ends_with('{') && !closes) - || ctx.Line(j + 1).contains("//") || ctx.Line(j + 1).contains("R\"")) { + || ctx.LineHasComment(j + 1) || ctx.LineHasMultiLineToken(j + 1)) { ok = false; break; } @@ -757,7 +760,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { continue; } } else if (delta == 0 && i + 1 < ctx.lines.size() && !trimmed.starts_with('#') - && !ctx.Line(i + 1).contains("//") && !ctx.Line(i + 1).contains("R\"")) { + && !ctx.LineHasComment(i + 1) && !ctx.LineHasMultiLineToken(i + 1)) { auto isOpStart = [](std::string_view s) { return s.starts_with("&&") || s.starts_with("||") || s.starts_with("| "); }; @@ -771,7 +774,7 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { std::size_t j = i + 1; bool ok = true; while (j < ctx.lines.size() && isOpStart(Trim(stripped[j]))) { - if (ParenDelta(stripped[j]) != 0 || ctx.Line(j + 1).contains("//") || ctx.Line(j + 1).contains("R\"")) { + if (ParenDelta(stripped[j]) != 0 || ctx.LineHasComment(j + 1) || ctx.LineHasMultiLineToken(j + 1)) { ok = false; break; } diff --git a/tests/HouseRules/main.cpp b/tests/HouseRules/main.cpp index 0b3593e..7afa0a5 100644 --- a/tests/HouseRules/main.cpp +++ b/tests/HouseRules/main.cpp @@ -252,6 +252,49 @@ int main() { Check(second.summary.changedFiles.empty(), "wrap-join is idempotent after the fixpoint"); } + // The guards used to be substring probes over the raw line, so a literal + // whose TEXT contained `//` or `R"` looked like a comment or a raw string + // and silently disabled the rule. Both shapes occur in this repo's own + // sources — "MARKER" ends in the characters R", and any string mentioning + // a lint-disable directive contains //. + { + RuleRun r = RunRule("void F() {\n" " auto hit = text.find(\"MARKER\",\n" " start);\n" "}\n", "wrap-join", LintMode::Apply); + Check(r.text.contains("text.find(\"MARKER\", start);"), "R\" inside a literal no longer blocks wrap-join"); + } + { + RuleRun r = RunRule("void F() {\n" " Check(read() == \"// lint-disable-next-line trim\\n\",\n" " \"message\");\n" "}\n", "wrap-join", LintMode::Apply); + Check(r.text.contains("\\n\", \"message\");"), "// inside a literal no longer blocks wrap-join"); + } + + // A real trailing comment still blocks the join: text pulled up past a + // `//` would be swallowed by it. + { + RuleRun r = RunRule("void F() {\n" " auto v = g(alpha, // why\n" " beta);\n" "}\n", "wrap-join", LintMode::Apply); + Check(r.text.contains("g(alpha, // why\n"), "a real comment still blocks wrap-join"); + } + + // A genuinely multi-line literal is never reflowed — joining its lines + // would change the string's contents. + { + std::string_view source = "void F() {\n" + " auto text = R\"sql(SELECT a,\n" + " b FROM t)sql\";\n" + "}\n"; + RuleRun r = RunRule(source, "wrap-join", LintMode::Apply); + Check(r.text == source, "wrap-join leaves a multi-line raw string alone"); + RuleRun paren = RunRule(source, "paren-spacing", LintMode::Apply); + Check(paren.text == source, "paren-spacing leaves a multi-line raw string alone"); + } + + // Type keywords inside a literal are text, not declarations. + { + std::string_view source = "void F() {\n" + " auto sql = R\"q(int x; unsigned long y;)q\";\n" + "}\n"; + RuleRun r = RunRule(source, "fixed-width-types", LintMode::Apply); + Check(r.text == source, "fixed-width-types leaves type names inside a raw string alone"); + } + if (Failures > 0) { std::println(std::cerr, "{} assertions failed", Failures); return 1; diff --git a/tests/Lint/main.cpp b/tests/Lint/main.cpp index cad9b5e..fa81ec4 100644 --- a/tests/Lint/main.cpp +++ b/tests/Lint/main.cpp @@ -156,8 +156,7 @@ int main() { Configuration cfg = FixtureConfig(); cfg.AddLintRule("marker", [](LintContext& ctx) { const std::string& code = ctx.CommentStripped(); - for (std::size_t pos = code.find("MARKER"); pos != std::string::npos; - pos = code.find("MARKER", pos + 1)) { + for (std::size_t pos = code.find("MARKER"); pos != std::string::npos; pos = code.find("MARKER", pos + 1)) { std::size_t line = 1 + std::count(code.begin(), code.begin() + pos, '\n'); ctx.Report(line, "MARKER in code"); } @@ -370,8 +369,7 @@ int main() { Configuration cfg = s.Config({"a"}); AddTrimRule(cfg); RunLint(cfg, Mode(LintMode::Apply)); - Check(s.Read("a") == "// lint-disable-next-line trim\nkeep \ntrim\n", - "suppressed line keeps its bytes; the unsuppressed one is fixed"); + Check(s.Read("a") == "// lint-disable-next-line trim\nkeep \ntrim\n", "suppressed line keeps its bytes; the unsuppressed one is fixed"); } // Multiple rule names on one directive (space- or comma-separated).