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): no-char-pointer reads the AST, retiring the interop denylist

Browse source 
The rule was `\bchar\s*\*` over the text minus a substring denylist — argv,
getenv, setenv, dlerror, c_str, .data(, reinterpret_cast, extern ". Every entry
was a patch for one interop site, the list could only grow as libraries
arrived, and each entry disabled the rule for the whole LINE it appeared on.

It now walks declarations and asks the question the denylist was approximating:
whose header dictates this spelling? A declaration with C language linkage, or
one whose initialiser binds to an entity declared outside the project root, is
somebody else's API and keeps its spelling. getenv, c_str and friends are
exempt because of where they are declared, not because they are named here, so
a new external library needs no new entry.

Two bugs found while testing this, both of which had made the rule silently
pass over the entire repository:

clang_getCursorLanguage cannot be used to detect extern "C". Its default answer
is CXLanguage_C for a plain function, variable or parameter even in a C++
translation unit, so isExternC was true almost everywhere and exempted
everything. Replaced by tracking CXCursor_LinkageSpec depth during the walk,
reading the extent text to tell extern "C" from extern "C++".

Attributing any foreign reference in a subtree to the enclosing declaration was
too broad: a function that merely touched libc++ somewhere in its body would
exempt its own signature. Narrowed to initialiser contexts — a variable, field
or parameter — which is where a binding to a foreign API actually occurs.

Also: functions now carry their RESULT type rather than the whole function
type, since the parameters arrive as their own declarations and would otherwise
be reported twice. main's parameters are exempt structurally, its signature
being fixed by the language rather than chosen here.

Two sites keep an explicit lint-disable, both Crafter::Run taking main's argv
verbatim. That is two visible, reasoned suppressions in place of a denylist
that silently disabled the rule for every line mentioning one of eight tokens.

ExternalCloneDir and ExternalIncludeFlags are now exposed from :External, so a
source that includes an external dependency's headers can be parsed without
running a build to discover where they are. BuildExternal derives its own
working directory through the same function, so the two cannot drift.
Crafter.Build-Shader.cpp needed this to parse at all.

A file with no compile command — project.cpp, which LoadProject builds with its
own flags — is not a translation unit of the build graph, so AST rules skip it
the way a rule self-filters by extension. That is distinct from a file that
should have parsed and did not, which stays an error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jorijn van der Graaf 2026-07-30 23:46:32 +02:00
commit 91e4f59a94
7 changed files with 209 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

61
lint-rules.h
View file

@ -70,6 +70,18 @@ inline bool IsCamelCase(std::string_view name) {
return !name.empty() && ((name[0] >= 'a' && name[0] <= 'z') || name[0] == '_') && name.find('_', 1) == std::string_view::npos;
}
// True for a pointer to char, as clang spells a type: "char *",
// "const char *", "char **". Deliberately not signed/unsigned char, which are
// byte buffers rather than text and were never the target.
inline bool IsCharPointer(std::string_view type) {
std::size_t star = type.find('*');
if (star == std::string_view::npos) return false;
std::string_view base = Trim(type.substr(0, star));
if (base.starts_with("const ")) base.remove_prefix(6);
if (base.starts_with("volatile ")) base.remove_prefix(9);
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;
@ -260,23 +272,42 @@ inline void AddProjectLintRules(Crafter::Configuration& cfg) {
}
});
// Prefer std::string / std::string_view over char*. OS interop stays:
// getenv returns char*, argv is char**, extern "C" prototypes mirror C.
cfg.AddLintRule("no-char-pointer", [](LintContext& ctx) {
// Prefer std::string / std::string_view over char* in our own declarations.
//
// Reads the AST rather than the text, which retires the substring denylist
// this rule used to carry (argv, getenv, setenv, dlerror, c_str, .data(,
// reinterpret_cast, extern "). Every entry was a patch for one interop
// site, the list could only grow, and it disabled the rule for a whole LINE
// whenever one appeared. The question is now asked directly: whose header
// dictates this spelling? A declaration with C language linkage, or one
// that binds to an entity declared outside the project, is somebody else's
// API and keeps its spelling.
//
// Only declarations are considered. A char* inside a cast or an expression
// is not an interface, and reinterpret_cast<char*> for binary IO — the case
// the denylist existed to permit — is no longer a finding to suppress.
cfg.AddAstLintRule("no-char-pointer", [](LintContext& ctx) {
if (!IsCppFile(ctx)) return;
std::vector<std::string_view> lines = Lines(ctx.CommentStripped());
static const std::regex charPtr(R"(\bchar\s*\*)");
for (std::size_t i = 0; i < lines.size(); ++i) {
std::string lineStr(lines[i]);
// C-interop stays char*: argv, getenv/setenv, dlerror, C APIs fed
// by c_str()/data(), binary IO reinterpret_casts, extern "C"
// prototypes. (extern " matches with the literal body blanked.)
if (lineStr.contains("argv") || lineStr.contains("getenv") || lineStr.contains("setenv")
|| lineStr.contains("dlerror") || lineStr.contains("c_str") || lineStr.contains(".data(")
|| lineStr.contains("reinterpret_cast") || lineStr.contains("extern \"")) continue;
if (std::regex_search(lineStr, charPtr)) {
ctx.Report(i + 1, "prefer std::string / std::string_view over char*");
std::span<const Crafter::LintDecl> decls = ctx.Decls();
for (const Crafter::LintDecl& decl : decls) {
// main's signature is fixed by the language, so its argv is no more
// ours to modernise than a C API's is.
if (decl.parent != Crafter::LintNoParent && decls[decl.parent].name == "main") continue;
if (decl.kind == Crafter::LintDeclKind::Function && decl.name == "main") continue;
switch (decl.kind) {
case Crafter::LintDeclKind::Variable:
case Crafter::LintDeclKind::Parameter:
case Crafter::LintDeclKind::Field:
case Crafter::LintDeclKind::TypeAlias:
case Crafter::LintDeclKind::Function:
case Crafter::LintDeclKind::Method:
break;
default:
continue;
}
if (decl.isExternC || decl.isForeignApi) continue;
if (!IsCharPointer(decl.type)) continue;
ctx.Report(decl.line, std::format("prefer std::string / std::string_view over char* ('{}' is '{}')", decl.name, decl.type));
}
});