From 651720e494f706a874fe96b594e133599c37e7f9 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Fri, 31 Jul 2026 00:50:48 +0200 Subject: [PATCH] feat(lint): const-local and constexpr-constant rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rules the AST makes possible, plus the mutation analysis behind them. const-local reports a local that is never written. It is restricted to SCALARS — integers, bools, enums, floating types — and that restriction is what makes the answer exact rather than a guess: a scalar has no member functions, so the only ways to write one are assignment, ++/--, having its address taken, or binding to a non-const reference. All four are now tracked in the walk: - assignment and compound assignment visit their LEFT operand in a write context, the right one normally; - ++/-- and & write their operand; - a call argument is checked against the callee's parameter type, so passing to `const int&` or by value is a read while `int&` is a write; - initialising a non-const reference writes what it binds to. For a class type a non-const method call could mutate it, and deciding that is the whole-program analysis clang-tidy does, so those are simply out of scope rather than guessed at. constexpr-constant promotes a const constant whose initialiser is made only of literals and operators, so `const int A = 1 << 4;` qualifies and `const int B = Compute();` does not. On this repository const-local found 103 candidates, which was too many to be useful, and the reason was informative: most were range-for bindings and pointer locals. `for (T* const x : …)` and `T* const p` are not spellings anybody writes, and the useful constness for a pointer is on the pointee, which this rule cannot advise on. Excluding both leaves 36, all plain bool or enum locals worth fixing — isWasm, isPe, exists, writes, isC and so on. Those 36 are fixed in this commit; the compiler verified every one. Both rules are report-only. The analysis is exact, but adding const is a judgement about intent as much as mechanics, and a wrong suggestion should cost a glance rather than a build. const-local also deliberately does not become a transform: inserting `const` before a shared type would apply it to every declarator in a multi-declarator statement, including any that IS written. Co-Authored-By: Claude Opus 5 (1M context) --- implementations/Crafter.Build-Clang.cpp | 6 +- implementations/Crafter.Build-External.cpp | 6 +- implementations/Crafter.Build-Lint.cpp | 200 +++++++++++++++++++-- implementations/Crafter.Build-Platform.cpp | 6 +- implementations/Crafter.Build-Shader.cpp | 2 +- interfaces/Crafter.Build-Clang.cppm | 21 +++ lint-rules.h | 74 ++++++++ tests/CleanProject/main.cpp | 2 +- tests/HouseRules/main.cpp | 64 ++++++- tests/Lint/main.cpp | 28 +-- 10 files changed, 370 insertions(+), 39 deletions(-) diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 942769f..4e7a8f9 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -204,7 +204,7 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac fileCopy.replace_extension(""); Implementation& implementation = this->implementations.emplace_back(std::move(fileCopy)); if (std::regex_search(fileContent, match, std::regex(R"(module ([a-zA-Z0-9_\.\-]+)(:[a-zA-Z0-9_\.\-]+)?\s*;)"))) { - bool isPartitionImpl = match[2].length() > 0; + const bool isPartitionImpl = match[2].length() > 0; for(const std::unique_ptr& interface : this->interfaces) { if(interface->name == match[1]) { if (!isPartitionImpl) { @@ -275,7 +275,7 @@ CompileCommand Crafter::GetCompileCommand(const Configuration& config) { // wasm32 targets reject -march and silently ignore -mtune (clang errors on // the former). Skip both for any wasm32-* triple. - bool isWasm = config.target.starts_with("wasm32"); + const bool isWasm = config.target.starts_with("wasm32"); std::string archFlags = isWasm ? std::string() : std::format(" -march={} -mtune={}", config.march, config.mtune); @@ -1777,7 +1777,7 @@ std::int32_t Crafter::Run(std::int32_t argc, char** argv) { // server (browser build with index.html). std::system on the // .wasm path goes nowhere useful — replace with detection. if (config.target.starts_with("wasm32")) { - bool browserBuild = fs::exists(absDir / "index.html"); + const bool browserBuild = fs::exists(absDir / "index.html"); auto have = [](std::string_view exe) { #ifdef _WIN32 std::string probe = std::format("where {} > NUL 2>&1", exe); diff --git a/implementations/Crafter.Build-External.cpp b/implementations/Crafter.Build-External.cpp index a6db151..d75aaf6 100644 --- a/implementations/Crafter.Build-External.cpp +++ b/implementations/Crafter.Build-External.cpp @@ -279,7 +279,7 @@ ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::s // LD_LIBRARY_PATH gymnastics. Static deps (.a) ignore the rpath // harmlessly. PE/COFF targets (mingw, msvc) resolve DLLs via PATH // or adjacency rather than rpath, so lld warns if we pass it. - bool isPe = target == "x86_64-w64-mingw32" || target == "x86_64-pc-windows-msvc"; + const bool isPe = target == "x86_64-w64-mingw32" || target == "x86_64-pc-windows-msvc"; std::string buildDirAbs = fs::absolute(cmakeBuildDir).string(); result.linkFlags.push_back(std::format("-L{}", buildDirAbs)); if (!isPe) result.linkFlags.push_back(std::format("-Wl,-rpath,{}", buildDirAbs)); @@ -375,7 +375,7 @@ Configuration* Crafter::GitProject(const GitProjectSpec& spec) { fs::path cloneDir = externalRoot / std::format("{}-{}", name, srcHash); { - bool exists = fs::exists(cloneDir); + const bool exists = fs::exists(cloneDir); Progress::Task task(std::format("{} {}", exists ? "Updating" : "Cloning", name)); if (std::string err = FetchGit(spec.source, cloneDir); !err.empty()) { throw std::runtime_error(std::format("GitProject({}): {}", spec.source.url, err)); @@ -416,7 +416,7 @@ fs::path Crafter::GitFetch(const GitSource& source) { fs::path cloneDir = externalRoot / std::format("{}-{:016x}", name, key); { - bool exists = fs::exists(cloneDir); + const bool exists = fs::exists(cloneDir); Progress::Task task(std::format("{} {}", exists ? "Updating" : "Cloning", name)); if (std::string err = FetchGit(source, cloneDir); !err.empty()) { throw std::runtime_error(std::format("GitFetch({}): {}", source.url, err)); diff --git a/implementations/Crafter.Build-Lint.cpp b/implementations/Crafter.Build-Lint.cpp index e233a88..478f303 100644 --- a/implementations/Crafter.Build-Lint.cpp +++ b/implementations/Crafter.Build-Lint.cpp @@ -88,6 +88,14 @@ namespace { decltype(&clang_getFileName) getFileName = nullptr; decltype(&clang_Cursor_isNull) cursorIsNull = nullptr; decltype(&clang_getResultType) getResultType = nullptr; + decltype(&clang_Cursor_getBinaryOpcode) getBinaryOpcode = nullptr; + decltype(&clang_getCursorUnaryOperatorKind) getUnaryOperatorKind = nullptr; + decltype(&clang_isConstQualifiedType) isConstQualifiedType = nullptr; + decltype(&clang_CXXMethod_isConst) methodIsConst = nullptr; + decltype(&clang_CXXMethod_isStatic) methodIsStatic = nullptr; + decltype(&clang_getArgType) getArgType = nullptr; + decltype(&clang_getNumArgTypes) getNumArgTypes = nullptr; + decltype(&clang_getPointeeType) getPointeeType = nullptr; }; LibClang LoadLibClang() { @@ -166,6 +174,22 @@ namespace { bind(lib.getFileName, "clang_getFileName"); bind(lib.cursorIsNull, "clang_Cursor_isNull"); bind(lib.getResultType, "clang_getResultType"); + bind(lib.getBinaryOpcode, "clang_Cursor_getBinaryOpcode"); + bind(lib.getUnaryOperatorKind, "clang_getCursorUnaryOperatorKind"); + bind(lib.isConstQualifiedType, "clang_isConstQualifiedType"); + bind(lib.methodIsConst, "clang_CXXMethod_isConst"); + bind(lib.methodIsStatic, "clang_CXXMethod_isStatic"); + bind(lib.getArgType, "clang_getArgType"); + bind(lib.getNumArgTypes, "clang_getNumArgTypes"); + bind(lib.getPointeeType, "clang_getPointeeType"); + bind(lib.getBinaryOpcode, "clang_Cursor_getBinaryOpcode"); + bind(lib.getUnaryOperatorKind, "clang_getCursorUnaryOperatorKind"); + bind(lib.isConstQualifiedType, "clang_isConstQualifiedType"); + bind(lib.methodIsConst, "clang_CXXMethod_isConst"); + bind(lib.methodIsStatic, "clang_CXXMethod_isStatic"); + bind(lib.getArgType, "clang_getArgType"); + bind(lib.getNumArgTypes, "clang_getNumArgTypes"); + bind(lib.getPointeeType, "clang_getPointeeType"); if (!missing.empty()) { lib.handle = nullptr; lib.error = std::format("loaded {} but it is missing {}", candidates.front(), join(missing)); @@ -364,8 +388,63 @@ namespace { std::vector* out = nullptr; std::vector stack; // indices of the enclosing declarations std::int32_t externCDepth = 0; // inside how many extern "C" blocks + // >0 while visiting a subtree whose value is being WRITTEN: the left + // side of an assignment, the operand of ++/--, or anything whose + // address is taken or which binds to a non-const reference. + std::int32_t writeDepth = 0; + // Set while visiting the binding of a range-for. + bool inLoopBinding = false; + // Name offset -> index, so a DeclRefExpr can be resolved back to the + // declaration it names without comparing USR strings. + std::unordered_map byNameOffset; }; + bool IsScalarTypeKind(CXTypeKind kind) { + switch (kind) { + case CXType_Bool: + case CXType_Char_U: + case CXType_UChar: + case CXType_UShort: + case CXType_UInt: + case CXType_ULong: + case CXType_ULongLong: + case CXType_Char_S: + case CXType_SChar: + case CXType_Short: + case CXType_Int: + case CXType_Long: + case CXType_LongLong: + case CXType_Float: + case CXType_Double: + case CXType_LongDouble: + case CXType_Enum: + case CXType_Pointer: + return true; + default: + return false; + } + } + + bool IsAssignmentOpcode(CX_BinaryOperatorKind opcode) { + return opcode >= CX_BO_Assign && opcode <= CX_BO_OrAssign; + } + + bool IsWritableReference(const LibClang& lc, CXType type) { + if (type.kind != CXType_LValueReference) return false; + return lc.isConstQualifiedType(lc.getPointeeType(type)) == 0; + } + + CXChildVisitResult CollectChild(CXCursor cursor, CXCursor, CXClientData data) { + static_cast*>(data)->push_back(cursor); + return CXChildVisit_Continue; + } + + std::vector ChildrenOf(const LibClang& lc, CXCursor cursor) { + std::vector children; + lc.visitChildren(cursor, &CollectChild, &children); + return children; + } + // Whether a CXCursor_LinkageSpec is `extern "C"` as opposed to // `extern "C++"`. libclang exposes no query, and the spelling is empty, but // the extent starts at the `extern` keyword so the source answers it. @@ -401,17 +480,31 @@ namespace { return !PathInsideRoot(fs::path(path), projectRoot); } + void ProcessCursor(CXCursor cursor, DeclWalk& walk); + CXChildVisitResult VisitDecl(CXCursor cursor, CXCursor, CXClientData data) { - DeclWalk& walk = *static_cast(data); + ProcessCursor(cursor, *static_cast(data)); + return CXChildVisit_Continue; + } + + // Visit a cursor's children with the write-context flag raised, so every + // DeclRefExpr inside counts as a write to what it names. + void ProcessAsWrite(CXCursor cursor, DeclWalk& walk) { + ++walk.writeDepth; + ProcessCursor(cursor, walk); + --walk.writeDepth; + } + + void ProcessCursor(CXCursor cursor, DeclWalk& walk) { const LibClang& lc = *walk.lc; CXSourceLocation location = lc.getCursorLocation(cursor); // Every declaration the imported modules bring in arrives here too — // an unfiltered visit of one interface unit walks ~495,000 cursors from // std alone. Prune before doing any work. - if (!lc.locationIsFromMainFile(location)) return CXChildVisit_Continue; + if (!lc.locationIsFromMainFile(location)) return; - CXCursorKind kind = lc.getCursorKind(cursor); - LintDeclKind mapped = MapCursorKind(kind); + const CXCursorKind kind = lc.getCursorKind(cursor); + const LintDeclKind mapped = MapCursorKind(kind); if (mapped == LintDeclKind::Other) { // A foreign reference inside a VARIABLE, FIELD or PARAMETER is an // initialiser binding that declaration to somebody else's API — @@ -421,11 +514,78 @@ namespace { // own signature. if (!walk.stack.empty()) { LintDecl& enclosing = (*walk.out)[walk.stack.back()]; - bool initialiserContext = enclosing.kind == LintDeclKind::Variable || enclosing.kind == LintDeclKind::Field || enclosing.kind == LintDeclKind::Parameter; + const bool initialiserContext = enclosing.kind == LintDeclKind::Variable || enclosing.kind == LintDeclKind::Field || enclosing.kind == LintDeclKind::Parameter; if (initialiserContext && ResolvesOutsideProject(lc, cursor, *walk.projectRoot)) { enclosing.isForeignApi = true; } } + + // ---- mutation analysis ---- + // A name used where a value is being written marks that + // declaration mutated. Resolving through getCursorReferenced means + // shadowing and qualified names come out right. + if (kind == CXCursor_DeclRefExpr && walk.writeDepth > 0) { + CXCursor target = lc.getCursorReferenced(cursor); + if (!lc.cursorIsNull(target)) { + std::uint32_t targetOffset = 0; + lc.getFileLocation(lc.getCursorLocation(target), nullptr, nullptr, nullptr, &targetOffset); + if (auto it = walk.byNameOffset.find(targetOffset); it != walk.byNameOffset.end()) { + (*walk.out)[it->second].isMutated = true; + } + } + } + // The left side of an assignment is written; the right side is read. + if (kind == CXCursor_BinaryOperator || kind == CXCursor_CompoundAssignOperator) { + if (IsAssignmentOpcode(lc.getBinaryOpcode(cursor))) { + std::vector children = ChildrenOf(lc, cursor); + if (!children.empty()) { + ProcessAsWrite(children.front(), walk); + for (std::size_t c = 1; c < children.size(); ++c) ProcessCursor(children[c], walk); + return; + } + } + } + // ++/-- write their operand; & lets it be written elsewhere, which + // we cannot follow, so it counts as mutated. + if (kind == CXCursor_UnaryOperator) { + const CXUnaryOperatorKind unary = lc.getUnaryOperatorKind(cursor); + const bool writes = unary == CXUnaryOperator_PreInc || unary == CXUnaryOperator_PreDec || unary == CXUnaryOperator_PostInc || unary == CXUnaryOperator_PostDec || unary == CXUnaryOperator_AddrOf; + if (writes) { + for (CXCursor child : ChildrenOf(lc, cursor)) ProcessAsWrite(child, walk); + return; + } + } + // An argument bound to a non-const lvalue reference can be written + // by the callee. + if (kind == CXCursor_CallExpr) { + std::vector children = ChildrenOf(lc, cursor); + CXType callee = lc.getCursorType(lc.getCursorReferenced(cursor)); + std::int32_t params = lc.getNumArgTypes(callee); + if (params > 0) { + // Children are [callee?, args...]; line them up from the end + // so an implicit callee child does not shift the mapping. + std::size_t firstArg = children.size() > static_cast(params) + ? children.size() - static_cast(params) : 0; + for (std::size_t c = 0; c < children.size(); ++c) { + const bool byWritableRef = c >= firstArg && IsWritableReference(lc, lc.getArgType(callee, static_cast(c - firstArg))); + if (byWritableRef) ProcessAsWrite(children[c], walk); + else ProcessCursor(children[c], walk); + } + return; + } + } + // The first child of a range-for is its binding; the rest are the + // range expression and the body. + if (kind == CXCursor_CXXForRangeStmt) { + std::vector children = ChildrenOf(lc, cursor); + for (std::size_t c = 0; c < children.size(); ++c) { + const bool binding = c == 0 && lc.getCursorKind(children[c]) == CXCursor_VarDecl; + walk.inLoopBinding = binding; + ProcessCursor(children[c], walk); + walk.inLoopBinding = false; + } + return; + } // Recursed by hand rather than with CXChildVisit_Recurse so the // enclosing-declaration stack stays accurate: the callback is never // told when a subtree ends. @@ -439,14 +599,14 @@ namespace { if (specBegin < walk.content->size()) { text = std::string_view(walk.content->data() + specBegin, std::min(specEnd - specBegin, walk.content->size() - specBegin)); } - bool isC = IsExternCLinkage(text); + const bool isC = IsExternCLinkage(text); if (isC) ++walk.externCDepth; lc.visitChildren(cursor, &VisitDecl, &walk); if (isC) --walk.externCDepth; - return CXChildVisit_Continue; + return; } lc.visitChildren(cursor, &VisitDecl, &walk); - return CXChildVisit_Continue; + return; } LintDecl decl; @@ -456,7 +616,7 @@ namespace { // whole function type: the parameters arrive as their own Parameter // declarations, so spelling them here too would make every rule // reading `type` report each one twice. - bool callable = mapped == LintDeclKind::Function || mapped == LintDeclKind::Method; + const bool callable = mapped == LintDeclKind::Function || mapped == LintDeclKind::Method; decl.type = TakeString(lc, lc.getTypeSpelling(callable ? lc.getResultType(lc.getCursorType(cursor)) : lc.getCursorType(cursor))); CXFile nameFile = nullptr; std::uint32_t nameLine = 0; @@ -491,11 +651,27 @@ namespace { } decl.parent = walk.stack.empty() ? LintNoParent : walk.stack.back(); + CXType declaredType = lc.getCursorType(cursor); + decl.isConst = lc.isConstQualifiedType(declaredType) != 0; + decl.isScalar = IsScalarTypeKind(declaredType.kind); + decl.isLoopVariable = walk.inLoopBinding; + if (mapped == LintDeclKind::Method) { + decl.isConstMethod = lc.methodIsConst(cursor) != 0; + decl.isStaticMethod = lc.methodIsStatic(cursor) != 0; + } + walk.out->push_back(std::move(decl)); - walk.stack.push_back(walk.out->size() - 1); - lc.visitChildren(cursor, &VisitDecl, &walk); + std::size_t index = walk.out->size() - 1; + walk.byNameOffset.emplace(nameOffset, index); + walk.stack.push_back(index); + // Binding a name to a non-const reference — `auto& r = x;` — lets x be + // written through r, which we cannot follow, so x counts as mutated. + if (IsWritableReference(lc, declaredType)) { + for (CXCursor child : ChildrenOf(lc, cursor)) ProcessAsWrite(child, walk); + } else { + lc.visitChildren(cursor, &VisitDecl, &walk); + } walk.stack.pop_back(); - return CXChildVisit_Continue; } std::vector WalkDecls(const LibClang& lc, CXTranslationUnit tu, const std::string& content, const fs::path& projectRoot) { diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index 55e035b..62ee0ab 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -814,7 +814,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { return ""; } } else { - bool isWasm = config.target.starts_with("wasm32"); + const bool isWasm = config.target.starts_with("wasm32"); // wasi-sdk drops std.cppm at /share/libc++/v1/, the rest of // the libc++ ecosystem (e.g. /opt/aarch64-rootfs) follows FHS at // /usr/share/libc++/v1/. @@ -906,7 +906,7 @@ namespace { // Re-derived under the lock: another builder may have refreshed the // cache from its own sources while we waited. std::string stamp = CrafterBuildSourceStamp(sourceDir, CrafterBuildModules); - bool upToDate = ReadCacheStamp(cacheDir) == stamp; + const bool upToDate = ReadCacheStamp(cacheDir) == stamp; for (std::string_view name : CrafterBuildModules) { fs::path cppmPath = sourceDir / std::format("{}.cppm", name); fs::path pcmPath = cacheDir / std::format("{}.pcm", name); @@ -961,7 +961,7 @@ Configuration Crafter::LoadProject(const fs::path& projectFile, std::span(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules); + const EShMessages messages = static_cast(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules); std::ifstream fileStream(path, std::ios::in | std::ios::binary); if (!fileStream) { return fail("failed to open shader source", {}); diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index ba639fb..d69a7f3 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -202,6 +202,27 @@ export namespace Crafter { // header, so the type-modernising rules must leave its bytes alone. // Replaces the hand-maintained substring denylists, which could only // ever grow: a new external library needs no new entry here. + // ---- constness ---- + // The declared type is const-qualified. + bool isConst = false; + // A scalar: integer, floating, bool, enum or pointer. For these, + // "is it ever written" is decidable from assignments, ++/--, address-of + // and reference bindings alone — there are no member calls that could + // mutate it — so isMutated is exact rather than a guess. + bool isScalar = false; + // Written to somewhere in this file: assigned, incremented, had its + // address taken, or bound to a non-const reference. Only meaningful + // for a declaration whose uses are all in this file, so a local rather + // than something with external linkage. + bool isMutated = false; + // Method declared const. Only set for Method. + bool isConstMethod = false; + // Method declared static. Only set for Method. + bool isStaticMethod = false; + // The binding of a range-for: `for (T x : range)`. A loop binding is + // not a variable a reader thinks of as assignable, so constness advice + // about it is noise. + bool isLoopVariable = false; bool isExternC = false; // declared with C language linkage bool isForeignApi = false; // its type or its body binds to an entity // declared outside the project root diff --git a/lint-rules.h b/lint-rules.h index afcc522..2237873 100644 --- a/lint-rules.h +++ b/lint-rules.h @@ -274,6 +274,80 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) { // groups, literals) are REWRITTEN automatically; anything the operand // scanner can't prove safe — raw-string lines, ternaries, mixed // operators, multi-line expressions — is reported for a human instead. + // A local that is never written should say so. Restricted to SCALARS — + // integers, bools, enums, pointers, floating types — which is what makes + // the answer exact rather than a guess: a scalar has no member functions, + // so the only ways to write one are assignment, ++/--, having its address + // taken, or binding to a non-const reference, and the AST layer tracks all + // four. For a class type, a non-const method call could mutate it and + // deciding that needs the whole-program analysis clang-tidy does. + // + // Report-only. Adding const is a judgement about intent as much as + // mechanics, and a wrong suggestion should cost a glance, not a build. + cfg.AddAstLintRule("const-local", [](LintContext& ctx) { + if (!IsCppFile(ctx)) return; + std::span decls = ctx.Decls(); + for (const Crafter::LintDecl& decl : decls) { + if (decl.kind != Crafter::LintDeclKind::Variable) continue; + if (decl.parent == Crafter::LintNoParent) continue; + // Locals only: a namespace-scope or static variable may be written + // from a translation unit this parse cannot see. + Crafter::LintDeclKind enclosing = decls[decl.parent].kind; + bool isLocal = enclosing == Crafter::LintDeclKind::Function || enclosing == Crafter::LintDeclKind::Method + || enclosing == Crafter::LintDeclKind::Constructor || enclosing == Crafter::LintDeclKind::Destructor; + if (!isLocal || decl.isStatic) continue; + if (decl.isConst || decl.isConstexpr) continue; + if (!decl.isScalar || decl.isMutated) continue; + if (decl.name.empty()) continue; + // A range-for binding is not what a reader pictures as an + // assignable variable, and `for (T* const x : …)` is not a spelling + // anybody writes. + if (decl.isLoopVariable) continue; + // Likewise `T* const p` — the useful constness for a pointer local + // is almost always on the pointee, which this rule cannot advise + // on. Restricting to value types keeps the advice actionable. + if (decl.type.contains('*')) continue; + ctx.Report(decl.line, std::format("'{}' is never modified — declare it const", decl.name)); + } + }); + + // A constant whose value is already a constant expression can be constexpr, + // which puts it in the type system rather than leaving it to the optimiser. + // Only fires when every token of the initialiser is a literal or an + // operator, so `const int A = 1 << 4;` qualifies and + // `const int B = Compute();` does not. + cfg.AddAstLintRule("constexpr-constant", [](LintContext& ctx) { + if (!IsCppFile(ctx)) return; + std::span tokens = ctx.Tokens(); + for (const Crafter::LintDecl& decl : ctx.Decls()) { + if (decl.kind != Crafter::LintDeclKind::Variable && decl.kind != Crafter::LintDeclKind::Field) continue; + if (!decl.isConst || decl.isConstexpr || !decl.isScalar) continue; + // A pointer's value is an address, which is rarely a constant + // expression and never an interesting one to promote. + if (decl.type.contains('*')) continue; + + // Walk the declaration's own tokens, starting after the '='. + bool sawAssign = false; + bool allConstant = true; + bool sawLiteral = false; + for (const Crafter::LintToken& token : tokens) { + if (token.offset < decl.nameOffset) continue; + if (token.offset >= decl.end) break; + std::string_view text = ctx.TokenText(token); + if (!sawAssign) { + if (text == "=") sawAssign = true; + continue; + } + if (token.kind == Crafter::LintTokenKind::Literal) { sawLiteral = true; continue; } + if (token.kind == Crafter::LintTokenKind::Punctuation) continue; + allConstant = false; // an identifier or keyword: not a literal fold + break; + } + if (!sawAssign || !sawLiteral || !allConstant) continue; + ctx.Report(decl.line, std::format("'{}' is a literal constant — declare it constexpr", decl.name)); + } + }); + cfg.AddLintRule("format-concat", [](LintContext& ctx) { if (!IsCppFile(ctx)) return; const std::string& code = ctx.CommentStripped(); diff --git a/tests/CleanProject/main.cpp b/tests/CleanProject/main.cpp index 4a9c8f0..ecad097 100644 --- a/tests/CleanProject/main.cpp +++ b/tests/CleanProject/main.cpp @@ -64,7 +64,7 @@ int main() { fs::path projectFile = root / "project.cpp"; std::ofstream(projectFile) << "\n"; fs::path cwdBin = fs::current_path() / "bin"; - bool cwdBinExisted = fs::exists(cwdBin); + const bool cwdBinExisted = fs::exists(cwdBin); CleanProject(projectFile); Check(!fs::exists(root / "bin"), "the named project's bin/ is gone"); diff --git a/tests/HouseRules/main.cpp b/tests/HouseRules/main.cpp index c4ee7c8..edae0b4 100644 --- a/tests/HouseRules/main.cpp +++ b/tests/HouseRules/main.cpp @@ -430,12 +430,72 @@ int main() { "enum struct AlsoFine { E };\n", "enum-class", LintMode::Report); Check(r.summary.findings.size() == 2, std::format("enum-class: exactly the two plain enums ({} found)", r.summary.findings.size())); - bool onFirst = std::any_of(r.summary.findings.begin(), r.summary.findings.end(), [](const LintFinding& f) { return f.line == 1; }); - bool onSplit = std::any_of(r.summary.findings.begin(), r.summary.findings.end(), [](const LintFinding& f) { return f.line == 2; }); + const bool onFirst = std::any_of(r.summary.findings.begin(), r.summary.findings.end(), [](const LintFinding& f) { return f.line == 1; }); + const bool onSplit = std::any_of(r.summary.findings.begin(), r.summary.findings.end(), [](const LintFinding& f) { return f.line == 2; }); Check(onFirst, "enum-class: single-line plain enum reported"); Check(onSplit, "enum-class: line-split plain enum reported at the keyword"); } + // const-local's mutation analysis is exact for scalars: assignment, ++/--, + // address-of and binding to a non-const reference are the only ways to + // write one, and all four are tracked. Both directions matter — a missed + // write means advising const on something that cannot be const. + { + RuleRun r = RunRule("void Mutate(int& out);\n" + "void ReadOnly(const int& in);\n" + "void ByValue(int v);\n" + "int Compute();\n" + "void F() {\n" + " int neverWritten = 1;\n" + " int assigned = 1; assigned = 2;\n" + " int incremented = 1; ++incremented;\n" + " int compound = 1; compound += 2;\n" + " int addressed = 1; int* taken = &addressed;\n" + " int toMutatingRef = 1; Mutate(toMutatingRef);\n" + " int toConstRef = 1; ReadOnly(toConstRef);\n" + " int toByValue = 1; ByValue(toByValue);\n" + " int boundToRef = 1; int& alias = boundToRef;\n" + " for (int loop = 0; loop < 1; ++loop) { (void)loop; }\n" + " (void)taken; (void)alias;\n" + "}\n", + "const-local", LintMode::Report); + Check(HasFinding(r.summary, "'neverWritten'"), "const-local: an unwritten local is reported"); + Check(HasFinding(r.summary, "'toConstRef'"), "const-local: passing to a const& is not a write"); + Check(HasFinding(r.summary, "'toByValue'"), "const-local: passing by value is not a write"); + Check(!HasFinding(r.summary, "'assigned'"), "const-local: assignment is a write"); + Check(!HasFinding(r.summary, "'incremented'"), "const-local: ++ is a write"); + Check(!HasFinding(r.summary, "'compound'"), "const-local: += is a write"); + Check(!HasFinding(r.summary, "'addressed'"), "const-local: taking an address counts as a write"); + Check(!HasFinding(r.summary, "'toMutatingRef'"), "const-local: binding to a non-const& parameter is a write"); + Check(!HasFinding(r.summary, "'boundToRef'"), "const-local: binding to a non-const& local is a write"); + Check(!HasFinding(r.summary, "'loop'"), "const-local: a mutated loop counter is not reported"); + // Pointers and range-for bindings are excluded: `T* const p` and + // `for (T* const x : …)` are not spellings anybody writes. + Check(!HasFinding(r.summary, "'taken'"), "const-local: pointer locals are out of scope"); + } + { + RuleRun r = RunRule("void F() {\n" " for (int each : Range()) { (void)each; }\n" "}\n", "const-local", LintMode::Report); + Check(!HasFinding(r.summary, "'each'"), "const-local: a range-for binding is not reported"); + } + + // constexpr-constant only promotes a constant whose initialiser is made of + // literals and operators, so a call result is left alone. + { + RuleRun r = RunRule("int Compute();\n" + "void F() {\n" + " const int literal = 4;\n" + " const int folded = 1 << 4;\n" + " const int fromCall = Compute();\n" + " constexpr int already = 8;\n" + " (void)literal; (void)folded; (void)fromCall; (void)already;\n" + "}\n", + "constexpr-constant", LintMode::Report); + Check(HasFinding(r.summary, "'literal'"), "constexpr: a literal constant is reported"); + Check(HasFinding(r.summary, "'folded'"), "constexpr: an operator fold over literals is reported"); + Check(!HasFinding(r.summary, "'fromCall'"), "constexpr: a call result is not a constant expression"); + Check(!HasFinding(r.summary, "'already'"), "constexpr: an existing constexpr is not re-reported"); + } + 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 404e141..69bd436 100644 --- a/tests/Lint/main.cpp +++ b/tests/Lint/main.cpp @@ -54,7 +54,7 @@ namespace { out.reserve(ctx.content.size()); for (std::size_t n = 1; n <= ctx.lines.size(); ++n) { std::string_view line = ctx.Line(n); - bool crlf = line.ends_with('\r'); + const bool crlf = line.ends_with('\r'); if (crlf) line.remove_suffix(1); while (line.ends_with(' ') || line.ends_with('\t')) line.remove_suffix(1); out += line; @@ -226,7 +226,7 @@ int main() { LintSummary sum = RunLint(cfg, Mode(LintMode::Apply)); Check(s.Read("a") == "hello\nworld\n", "Apply rewrites the file"); Check(sum.changedFiles.size() == 1, "one file formatted"); - bool hasNote = std::any_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.rule == "note"; }); + const bool hasNote = std::any_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.rule == "note"; }); Check(hasNote, "report-only findings still recorded in Apply mode"); Check(sum.errors == 0, "clean apply has no errors"); } @@ -259,8 +259,8 @@ int main() { ctx.SetContent(std::move(c)); }); LintSummary rep = RunLint(cfg, Mode(LintMode::Report)); - bool one = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "one"; }); - bool two = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "two"; }); + const bool one = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "one"; }); + const bool two = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "two"; }); Check(one && two, "chained transforms both attributed"); Check(s.Read("a") == "AAA\n", "Report leaves chain input untouched"); LintSummary app = RunLint(cfg, Mode(LintMode::Apply)); @@ -281,7 +281,7 @@ int main() { Check(s.Read("a") == "keep\n", "throwing transform reverted, disk untouched"); Check(sum.errors == 1, "exception counted as error"); Check(sum.changedFiles.empty(), "reverted transform is not a change"); - bool threw = std::any_of(sum.findings.begin(), sum.findings.end(), + const bool threw = std::any_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 0 && f.message.contains("mid-transform"); }); @@ -301,7 +301,7 @@ int main() { } }); LintSummary rep = RunLint(cfg, Mode(LintMode::Report)); - bool wholeFile = std::any_of(rep.findings.begin(), rep.findings.end(), + const bool wholeFile = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "final-newline" && f.line == 0; }); @@ -323,8 +323,8 @@ int main() { ctx.SetContent(std::move(c)); }); LintSummary rep = RunLint(cfg, Mode(LintMode::Report)); - bool flagged = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.message == "flagged"; }); - bool reformat = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.message == "would reformat"; }); + const bool flagged = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.message == "flagged"; }); + const bool reformat = std::any_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.message == "would reformat"; }); Check(flagged && reformat, "mixed rule records both finding kinds"); RunLint(cfg, Mode(LintMode::Apply)); Check(s.Read("a") == "bad\n", "mixed rule's transform applied"); @@ -389,10 +389,10 @@ int main() { }); AddTrimRule(cfg); LintSummary sum = RunLint(cfg, Mode(LintMode::Report)); - bool line2Silent = std::none_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 2 && f.rule != "trim"; }); - bool line3Loud = std::count_if(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 3; }) >= 2; + const bool line2Silent = std::none_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 2 && f.rule != "trim"; }); + const bool line3Loud = std::count_if(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 3; }) >= 2; Check(line2Silent, "both named rules suppressed on the target line"); - bool trimStillFires = std::any_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 2 && f.rule == "trim"; }); + const bool trimStillFires = std::any_of(sum.findings.begin(), sum.findings.end(), [](const LintFinding& f) { return f.line == 2 && f.rule == "trim"; }); Check(trimStillFires, "unnamed rule still fires on the target line"); Check(line3Loud, "unsuppressed line reports from both rules"); } @@ -419,7 +419,7 @@ int main() { AddTrimRule(cfg); cfg.AddLintRule("flag", [](LintContext& ctx) { ctx.Report(2, "bad"); }); LintSummary rep = RunLint(cfg, Mode(LintMode::Report)); - bool onlyTrim = !rep.findings.empty() && std::all_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "trim"; }); + const bool onlyTrim = !rep.findings.empty() && std::all_of(rep.findings.begin(), rep.findings.end(), [](const LintFinding& f) { return f.rule == "trim"; }); Check(onlyTrim, "file-level rule directive kills that rule, trim still fires"); RunLint(cfg, Mode(LintMode::Apply)); Check(s.Read("a") == "// lint-disable-file flag\nbad\n", "other rules still format"); @@ -458,12 +458,12 @@ int main() { Check(offsetsSound, "tokens: every offset/length addresses real bytes"); // Ordered by offset, so binary search in TokensOnLine is valid. - bool ordered = std::ranges::is_sorted(toks, {}, &LintToken::offset); + const bool ordered = std::ranges::is_sorted(toks, {}, &LintToken::offset); Check(ordered, "tokens: stream is in source order"); // Inactive #ifdef branch is still lexed — this is what keeps token // rules covering every platform, unlike an AST. - bool sawHidden = std::ranges::any_of(toks, [&](const LintToken& t) { + const bool sawHidden = std::ranges::any_of(toks, [&](const LintToken& t) { return t.kind == LintTokenKind::Identifier && ctx.TokenText(t) == "HiddenBranch"; }); Check(sawHidden, "tokens: inactive #ifdef branch is lexed");