feat(lint): const-local and constexpr-constant rules
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) <noreply@anthropic.com>
This commit is contained in:
parent
5ca2b3e1df
commit
651720e494
10 changed files with 370 additions and 39 deletions
|
|
@ -204,7 +204,7 @@ void Configuration::GetInterfacesAndImplementations(std::span<fs::path> 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<Module>& 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);
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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<LintDecl>* out = nullptr;
|
||||
std::vector<std::size_t> 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<std::size_t, std::size_t> 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<std::vector<CXCursor>*>(data)->push_back(cursor);
|
||||
return CXChildVisit_Continue;
|
||||
}
|
||||
|
||||
std::vector<CXCursor> ChildrenOf(const LibClang& lc, CXCursor cursor) {
|
||||
std::vector<CXCursor> 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<DeclWalk*>(data);
|
||||
ProcessCursor(cursor, *static_cast<DeclWalk*>(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<CXCursor> 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<CXCursor> 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<std::size_t>(params)
|
||||
? children.size() - static_cast<std::size_t>(params) : 0;
|
||||
for (std::size_t c = 0; c < children.size(); ++c) {
|
||||
const bool byWritableRef = c >= firstArg && IsWritableReference(lc, lc.getArgType(callee, static_cast<std::uint32_t>(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<CXCursor> 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<std::size_t>(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);
|
||||
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<LintDecl> WalkDecls(const LibClang& lc, CXTranslationUnit tu, const std::string& content, const fs::path& projectRoot) {
|
||||
|
|
|
|||
|
|
@ -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 <sysroot>/share/libc++/v1/, the rest of
|
||||
// the libc++ ecosystem (e.g. /opt/aarch64-rootfs) follows FHS at
|
||||
// <sysroot>/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<const
|
|||
|
||||
EnsureCrafterBuildPcms(sourceDir, cacheDir);
|
||||
|
||||
bool stale = !fs::exists(soPath) || fs::last_write_time(soPath) < fs::last_write_time(absProject) || fs::last_write_time(soPath) < fs::last_write_time(hostExe);
|
||||
const bool stale = !fs::exists(soPath) || fs::last_write_time(soPath) < fs::last_write_time(absProject) || fs::last_write_time(soPath) < fs::last_write_time(hostExe);
|
||||
|
||||
if (stale) {
|
||||
std::string compileCmd = std::format(
|
||||
|
|
|
|||
|
|
@ -61,7 +61,7 @@ namespace Crafter {
|
|||
return out;
|
||||
};
|
||||
|
||||
EShMessages messages = static_cast<EShMessages>(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules);
|
||||
const EShMessages messages = static_cast<EShMessages>(EShMsgDefault | EShMsgVulkanRules | EShMsgSpvRules);
|
||||
std::ifstream fileStream(path, std::ios::in | std::ios::binary);
|
||||
if (!fileStream) {
|
||||
return fail("failed to open shader source", {});
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
74
lint-rules.h
74
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<const Crafter::LintDecl> 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<const Crafter::LintToken> 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();
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
Loading…
Reference in a new issue