Catcrafts/Crafter.Build
Watch
2
0
Fork
You've already forked Crafter.Build
 0
Code Issues Pull requests Projects Releases 1 Packages Wiki Activity Actions

feat(lint): naming reads the AST, deleting the scope heuristic

Browse source 
The rule was 125 lines: four std::regex, a hand-rolled {/} scope stack, a
cumulative paren-depth counter so a wrapped parameter list would not look like
a declaration, a 40-entry keyword denylist, and a "function-shaped line" guess
whose own comment conceded it was heuristic. Storage class came from
lineStr.contains("static "). It is now ~55 lines that ask clang what kind of
declaration each thing is and what encloses it.

The three regressions the old version carried special cases for — a call with
an inline lambda argument, a bare statement call, a one-liner method — need no
handling at all, because a call is not a declaration. Their tests pass
unchanged.

On this repository the exact version found 42 violations the heuristic had
never been able to see, all real:

  - 37 members of the libclang function-pointer table added two commits ago
    were PascalCase. The old varDecl regex could not match a declaration whose
    type is decltype(&f), so they were silently skipped. Renamed to camelCase,
    which mirrors clang_createIndex -> createIndex more closely anyway.
  - Crafter.Build-Shader.cpp had a snake_case local, file_name_list, invisible
    to the heuristic for the same reason (it declares a const char* array).
  - Four extern "C" declarations of libc functions in tests were reported as
    badly-named functions. Those are named by the C library, so C language
    linkage is now an exemption — the same principled test no-char-pointer
    uses, rather than another denylist entry.

New tests cover what the line-based version structurally could not reach: a
signature wrapped over several lines, `static` on its own line above the
declaration it applies to, and a member versus a local inside a method.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jorijn van der Graaf 2026-07-30 23:53:26 +02:00
commit d55657b7ce
4 changed files with 227 additions and 255 deletions
Download patch file Download diff file Expand all files Collapse all files

196
lint-rules.h
View file

@ -82,15 +82,6 @@ inline bool IsCharPointer(std::string_view type) {
return base == "char";
}
// The identifier ending right before position `pos` (exclusive), or empty.
inline std::string_view WordBefore(std::string_view s, std::size_t pos) {
std::size_t end = pos;
while (end > 0 && (s[end - 1] == ' ' || s[end - 1] == '\t')) --end;
std::size_t begin = end;
while (begin > 0 && (IsWordChar(s[begin - 1]) || s[begin - 1] == '~')) --begin;
return s.substr(begin, end - begin);
}
inline void AddProjectLintRules(Crafter::Configuration& cfg) {
using Crafter::LintContext;
@ -118,132 +109,77 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) {
}
});
// Naming: functions/types PascalCase, variables camelCase, statics and
// namespace-scope globals PascalCase. A line-based scope tracker decides
// whether a declaration sits at namespace scope (global) or inside a
// function/type (local/member). Heuristic by nature — report-only.
cfg.AddLintRule("naming", [](LintContext& ctx) {
// Naming: types and functions PascalCase, variables camelCase, with
// statics, namespace-scope globals and constexpr constants PascalCase.
//
// Reads the AST. The previous version needed four std::regex, a hand-rolled
// {/} scope stack, a cumulative paren-depth counter to avoid mistaking a
// wrapped parameter list for a declaration, a keyword denylist, and a
// "function-shaped line" guess whose own comment conceded it was heuristic.
// All of it existed to answer two questions clang answers directly: what
// kind of declaration is this, and what encloses it.
cfg.AddAstLintRule("naming", [](LintContext& ctx) {
if (!IsCppFile(ctx)) return;
std::vector<std::string_view> lines = Lines(ctx.CommentStripped());
enum class Scope { Namespace, Type, Function, Other };
std::vector<Scope> stack;
auto currentScope = [&]() { return stack.empty() ? Scope::Namespace : stack.back(); };
static const std::regex typeDecl(R"(\b(?:class|struct|union)\s+(?:CRAFTER_API\s+)?([A-Za-z_]\w*))");
static const std::regex enumDecl(R"(\benum\s+(?:class\s+|struct\s+)?([A-Za-z_]\w*))");
static const std::regex usingDecl(R"(^\s*using\s+([A-Za-z_]\w*)\s*=)");
static const std::regex varDecl(
R"(^\s*((?:static|constexpr|const|inline|mutable|thread_local|export|CRAFTER_API)\s+)*)"
R"((?:std::)?[A-Za-z_][\w:]*(?:<[^;={]*>)?(?:\s*[&*])*\s+([A-Za-z_]\w*)\s*(=|;|\{))");
static const std::unordered_set<std::string_view> keywords = {
"if", "for", "while", "switch", "catch", "return", "else", "do", "case", "goto",
"new", "delete", "throw", "using", "namespace", "template", "typedef", "friend",
"public", "private", "protected", "class", "struct", "enum", "union", "import",
"module", "export", "break", "continue", "co_return", "co_await", "co_yield",
"sizeof", "alignof", "decltype", "static_assert", "operator", "try", "requires",
};
// Cumulative paren depth at the start of each line: declaration and
// function checks only run at depth 0, so wrapped parameter lists and
// continuation lines (`) {` closers) never look like declarations.
std::int64_t parenDepth = 0;
for (std::size_t i = 0; i < lines.size(); ++i) {
std::string_view trimmed = Trim(lines[i]);
std::string lineStr(lines[i]);
std::smatch m;
bool atDepth0 = parenDepth == 0;
parenDepth = std::max<std::int64_t>(0, parenDepth + ParenDelta(lines[i]));
if (!trimmed.starts_with('#') && atDepth0) {
// Type / alias names must be PascalCase.
if (std::regex_search(lineStr, m, typeDecl) || std::regex_search(lineStr, m, enumDecl)) {
std::string name = m[1].str();
if (!keywords.contains(name) && !IsPascalCase(name)) {
ctx.Report(i + 1, std::format("type '{}' should be PascalCase", name));
using Kind = Crafter::LintDeclKind;
std::span<const Crafter::LintDecl> decls = ctx.Decls();
for (const Crafter::LintDecl& decl : decls) {
if (decl.name.empty()) continue; // anonymous namespace, unnamed struct
// A declaration with C language linkage is named by the C library
// it mirrors — `extern "C" int setenv(...)` is not ours to rename.
if (decl.isExternC) continue;
// Namespace scope is now a lookup rather than a brace count, and it
// is exact for a local declared inside a function body.
bool atNamespaceScope = decl.parent == Crafter::LintNoParent
|| decls[decl.parent].kind == Kind::Namespace;
switch (decl.kind) {
case Kind::Class:
case Kind::Struct:
case Kind::Union:
case Kind::Enum:
if (!IsPascalCase(decl.name)) {
ctx.Report(decl.line, std::format("type '{}' should be PascalCase", decl.name));
}
}
if (std::regex_search(lineStr, m, usingDecl) && !IsPascalCase(m[1].str())) {
ctx.Report(i + 1, std::format("type alias '{}' should be PascalCase", m[1].str()));
}
// Function definitions: identifier before the first '(' on a
// line that ends where a body opens or closes — a multi-line
// def's trailing '{' or a one-liner's trailing '}'. Statement
// calls with inline lambda arguments look similar
// (`bool x = std::any_of(..., [](T v) { ... });`) but end in
// ';' and/or carry '=' before the name — both excluded.
// Lambdas ("](") and control keywords are skipped; ctors/
// dtors pass the Pascal check by construction; `main` and
// operators exempt.
std::size_t bodyBrace = trimmed.find('{');
bool functionShaped = trimmed.ends_with('{') || trimmed.ends_with('}');
if (functionShaped && currentScope() != Scope::Function && !trimmed.starts_with("return")) {
std::size_t paren = trimmed.find('(');
if (paren != std::string_view::npos && paren > 0 && paren < bodyBrace) {
std::string_view name = WordBefore(trimmed, paren);
char before = trimmed[paren - 1];
bool looksLikeDef = !name.empty() && IsWordChar(before)
&& !keywords.contains(name) && name != "main"
&& !name.starts_with('~');
// Require a type token before the name (or a
// qualified Class::Name), with no '=' in front —
// plain calls and initializations don't match.
if (looksLikeDef) {
std::size_t nameStart = trimmed.rfind(name, paren);
std::string_view prefix = Trim(trimmed.substr(0, nameStart));
looksLikeDef = !prefix.empty() && !prefix.contains('=')
&& (IsWordChar(prefix.back()) || prefix.back() == '>'
|| prefix.back() == '*' || prefix.back() == '&'
|| prefix.ends_with("::"));
break;
case Kind::TypeAlias:
if (!IsPascalCase(decl.name)) {
ctx.Report(decl.line, std::format("type alias '{}' should be PascalCase", decl.name));
}
break;
case Kind::Function:
case Kind::Method:
// main is spelled by the language; operators by their
// symbol. Constructors and destructors take their type's
// name and are separate kinds, so they never arrive here.
if (decl.name == "main" || decl.name.starts_with("operator")) break;
if (!IsPascalCase(decl.name)) {
ctx.Report(decl.line, std::format("function '{}' should be PascalCase", decl.name));
}
break;
case Kind::Variable:
// constexpr variables are compile-time constants and take
// constant naming, like statics and globals.
if (decl.isStatic || decl.isConstexpr || atNamespaceScope) {
if (!IsPascalCase(decl.name)) {
ctx.Report(decl.line, std::format("{} '{}' should be PascalCase",
decl.isStatic ? "static variable"
: decl.isConstexpr ? "constexpr constant"
: "global variable", decl.name));
}
if (looksLikeDef && !IsPascalCase(name)) {
ctx.Report(i + 1, std::format("function '{}' should be PascalCase", name));
} else if (!IsCamelCase(decl.name)) {
ctx.Report(decl.line, std::format("variable '{}' should be camelCase", decl.name));
}
break;
case Kind::Field:
if (decl.isStatic || decl.isConstexpr) {
if (!IsPascalCase(decl.name)) {
ctx.Report(decl.line, std::format("static member '{}' should be PascalCase", decl.name));
}
} else if (!IsCamelCase(decl.name)) {
ctx.Report(decl.line, std::format("member '{}' should be camelCase", decl.name));
}
}
// Variable declarations: camelCase locally, PascalCase for
// statics and namespace-scope globals.
if (std::regex_search(lineStr, m, varDecl)) {
std::string name = m[2].str();
std::string_view typeToken = Trim(std::string_view(lineStr).substr(0, static_cast<std::size_t>(m.position(2))));
std::string_view firstWord = typeToken.substr(0, typeToken.find_first_of(" \t<"));
if (!keywords.contains(firstWord) && !keywords.contains(name)) {
bool isStatic = lineStr.contains("static ");
// constexpr variables are compile-time constants —
// constant naming (PascalCase) like statics/globals.
bool isConstexpr = lineStr.contains("constexpr ");
bool global = currentScope() == Scope::Namespace;
if ((isStatic || global || isConstexpr) && !IsPascalCase(name)) {
ctx.Report(i + 1, std::format("{} '{}' should be PascalCase",
isStatic ? "static variable"
: isConstexpr ? "constexpr constant"
: "global variable", name));
} else if (!isStatic && !global && !isConstexpr && !IsCamelCase(name)) {
ctx.Report(i + 1, std::format("variable '{}' should be camelCase", name));
}
}
}
}
// Scope tracking: classify each '{' opened on this line; pop on '}'.
for (std::size_t c = 0; c < lines[i].size(); ++c) {
if (lines[i][c] == '{') {
Scope kind = Scope::Other;
std::string_view upTo = Trim(lines[i].substr(0, c));
if (trimmed.starts_with("namespace") || upTo.contains("namespace ")) {
kind = Scope::Namespace;
} else if (std::regex_search(lineStr, typeDecl) || std::regex_search(lineStr, enumDecl)) {
kind = Scope::Type;
} else if (upTo.ends_with(')') || upTo.ends_with("const") || upTo.ends_with("noexcept")
|| upTo.ends_with("->") || trimmed.starts_with("extern")) {
kind = Scope::Function; // function/lambda/control body — all non-global
}
stack.push_back(kind);
} else if (lines[i][c] == '}') {
if (!stack.empty()) stack.pop_back();
}
break;
default:
break;
}
}
});