From 6438cb9ebbc7e741200ab9dd3662a0b62b035e19 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Thu, 30 Jul 2026 23:59:46 +0200 Subject: [PATCH] feat(lint): fixed-width-types keeps widths that a foreign API chose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rewrite itself stays token-shaped — it edits type SPELLINGS, which an AST discards — but what it must not touch now comes from the AST. The old exemption was per LINE: `int main`, `argc`, `argv`, `extern "`. Being a transform, a missed exemption here does not over-report, it emits code that no longer matches the API being called, so this is the rule where guessing from substrings mattered most. And being per-line, it also disabled the rule for anything sharing a line with one of those words. Now a declaration with C language linkage, or one whose initialiser binds to an entity declared outside the project, contributes a protected byte range and keeps its spelling. That answers the case directly: a function declared in somebody else's header taking `unsigned int` keeps `unsigned int`, and a local initialised from strtoul keeps `unsigned long`, because of where those are declared rather than because of what the line says. main is protected from the start of its declaration to the opening brace of its body, not for its whole extent. Its signature is fixed by the language; its body is ordinary code. Three findings on this repository came out of that distinction, all correct: for (int i = 1; i < argc; ++i) -> for (std::int32_t i = 1; ...) skipped before only because `argc` appeared on the line, plus Crafter::Run's own `int argc` and return type, which are ours rather than the language's. All three AST rules now share one interop test instead of carrying a denylist each, and it is the same test: whose header dictates this spelling. Co-Authored-By: Claude Opus 5 (1M context) --- implementations/Crafter.Build-Clang.cpp | 4 +-- interfaces/Crafter.Build-Clang.cppm | 2 +- lint-rules.h | 40 ++++++++++++++++++++----- tests/HouseRules/main.cpp | 32 ++++++++++++++++++++ 4 files changed, 68 insertions(+), 10 deletions(-) diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 6a5d2ab..942769f 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -1638,7 +1638,7 @@ Exit status: } // lint-disable-next-line no-char-pointer -int Crafter::Run(int argc, char** argv) { +std::int32_t Crafter::Run(std::int32_t argc, char** argv) { try { std::string_view argv0 = argc > 0 ? argv[0] : "crafter-build"; fs::path projectFile = "./project.cpp"; @@ -1654,7 +1654,7 @@ int Crafter::Run(int argc, char** argv) { RunLintOptions lintOpts; Progress::Verbosity verbosity = Progress::Verbosity::Default; - for (int i = 1; i < argc; ++i) { + for (std::int32_t i = 1; i < argc; ++i) { std::string_view arg = argv[i]; if (arg == "-h" || arg == "--help" || (!runTests && !runLint && !runFormat && arg == "help")) { PrintHelp(argv0); diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index d35ac23..def137b 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -554,7 +554,7 @@ export namespace Crafter { // Takes main's argv verbatim so the CLI entry point is a one-liner; // the shape is inherited from the language, not chosen here. // lint-disable-next-line no-char-pointer - CRAFTER_API int Run(int argc, char** argv); + CRAFTER_API std::int32_t Run(std::int32_t argc, char** argv); // Delete the bin/ and build/ trees beside `projectFile`, returning the paths // that existed and were removed. Backs `crafter-build clean`. diff --git a/lint-rules.h b/lint-rules.h index b18c9fc..f4054a7 100644 --- a/lint-rules.h +++ b/lint-rules.h @@ -487,11 +487,40 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { // short/int/long → fixed-width types. Lines mentioning main/argc/argv or // extern "C" keep their C-conventional ints. - cfg.AddLintRule("fixed-width-types", [](LintContext& ctx) { + cfg.AddAstLintRule("fixed-width-types", [](LintContext& ctx) { if (!IsCppFile(ctx)) return; const std::string& code = ctx.CommentStripped(); struct Rep { std::size_t pos; std::size_t len; std::string to; }; std::vector reps; + + // Byte ranges whose integer spelling is not ours to change. A wrong + // rewrite here does not merely over-report — it produces code that no + // longer matches the API it is calling — so this is derived from the + // AST rather than from substrings on the line. + // + // Replaces the old per-LINE textual exemption (`int main`, `argc`, + // `argv`, `extern "`), which both missed cases and disabled the rule + // for everything else sharing a line with one of those words. + std::vector> protectedRanges; + for (const Crafter::LintDecl& decl : ctx.Decls()) { + if (decl.isExternC || decl.isForeignApi) { + protectedRanges.emplace_back(decl.begin, decl.end); + continue; + } + // main's signature is fixed by the language. Only the signature: + // its body is ordinary code, and the declaration's extent covers + // the whole function. + if (decl.kind == Crafter::LintDeclKind::Function && decl.name == "main") { + std::size_t bodyBrace = ctx.content.find('{', decl.begin); + protectedRanges.emplace_back(decl.begin, bodyBrace == std::string::npos ? decl.end : bodyBrace); + } + } + auto isProtected = [&protectedRanges](std::size_t offset) { + return std::any_of(protectedRanges.begin(), protectedRanges.end(), + [offset](const std::pair& range) { + return offset >= range.first && offset < range.second; + }); + }; // Builtin integer type specifiers combine in any order (`unsigned // long`, `long unsigned int`, ...), so match whole RUNS of these // keywords and classify the run, rather than the words one by one. @@ -503,13 +532,8 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { std::size_t lineEnd = code.find('\n', lineStart); if (lineEnd == std::string::npos) lineEnd = code.size(); std::string_view line(code.data() + lineStart, lineEnd - lineStart); - // `int main` / argc / argv keep their C-conventional type; so do - // extern "C" prototypes (the literal body is blanked in the - // stripped text, so match `extern "`). - bool exempt = line.contains("int main") || line.contains("argc") || line.contains("argv") - || line.contains("extern \""); std::size_t pos = 0; - while (!exempt && pos < line.size()) { + while (pos < line.size()) { if (!IsWordChar(line[pos])) { ++pos; continue; } std::size_t wordEnd = pos; while (wordEnd < line.size() && IsWordChar(line[wordEnd])) ++wordEnd; @@ -546,6 +570,8 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { if (hasChar && !hasUnsigned && !hasSigned) continue; if (nextWord == "double") continue; + if (isProtected(lineStart + runBegin)) continue; + std::string_view width = hasChar ? "8" : hasShort ? "16" : hasLong ? "64" : "32"; reps.push_back({lineStart + runBegin, runEnd - runBegin, std::format("std::{}int{}_t", hasUnsigned ? "u" : "", width)}); diff --git a/tests/HouseRules/main.cpp b/tests/HouseRules/main.cpp index e34adbb..a82871e 100644 --- a/tests/HouseRules/main.cpp +++ b/tests/HouseRules/main.cpp @@ -363,6 +363,38 @@ int main() { Check(!HasFinding(r.summary, "'Holder'"), "naming: PascalCase type passes"); } + // fixed-width-types must not rewrite an integer whose width somebody else's + // header chose — the rewrite would leave code that no longer matches the API + // it calls. Derived from the AST, so the exemption is per-declaration rather + // than the old per-line textual one, which both missed cases and disabled + // the rule for anything else sharing a line with `argc`/`extern "`. + { + RuleRun r = RunRule("#include \n" + "extern \"C\" unsigned long CApiCall(unsigned int flags);\n" + "long OurOwnApi(int value) { return value; }\n" + "void F() {\n" + " unsigned long fromLibc = std::strtoul(\"1\", nullptr, 10);\n" + " unsigned ours = 1;\n" + " (void)fromLibc; (void)ours;\n" + "}\n" + "int main(int argc, char** argv) {\n" + " for (int i = 0; i < argc; ++i) { (void)argv[i]; }\n" + " return 0;\n" + "}\n", + "fixed-width-types", LintMode::Apply); + // Somebody else's widths, kept. + Check(r.text.contains("extern \"C\" unsigned long CApiCall(unsigned int flags);"), "fixed-width: extern \"C\" signature keeps its widths"); + Check(r.text.contains("unsigned long fromLibc = std::strtoul"), "fixed-width: a value from a C library keeps its width"); + Check(r.text.contains("int main(int argc, char** argv)"), "fixed-width: main's signature is untouched"); + // Ours, converted. + Check(r.text.contains("std::int64_t OurOwnApi(std::int32_t value)"), "fixed-width: our own signature converts"); + Check(r.text.contains("std::uint32_t ours = 1;"), "fixed-width: our own local converts"); + // The loop counter inside main's BODY is ordinary code. The old rule + // skipped it because `argc` appeared on the same line; only the + // signature is exempt now, not everything near it. + Check(r.text.contains("for (std::int32_t i = 0;"), "fixed-width: main's body is not exempt, only its signature"); + } + if (Failures > 0) { std::println(std::cerr, "{} assertions failed", Failures); return 1;