diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index cb5e2a5..6a5d2ab 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -350,6 +350,22 @@ CompileCommand Crafter::GetCompileCommand(const Configuration& config) { } out.command += out.includeFlags; + // External dependency includes, gathered transitively. Kept off `command` + // so it stays exactly what it was before this was extracted. + { + std::unordered_set seen; + std::function addExternal = [&](const Configuration* cfg) { + if (!seen.insert(cfg).second) return; + for (const ExternalDependency& dep : cfg->externalDependencies) { + for (const std::string& flag : ExternalIncludeFlags(dep, cfg->target)) { + out.externalIncludeFlags += std::format(" {}", flag); + } + } + for (const Configuration* sub : cfg->dependencies) addExternal(sub); + }; + addExternal(&config); + } + // Defines belong on both C and C++ compiles so vendored C dependencies // can see configuration-level macros consistently with module sources. for(const Define& define : config.defines) { @@ -1621,6 +1637,7 @@ Exit status: )", argv0); } +// lint-disable-next-line no-char-pointer int Crafter::Run(int argc, char** argv) { try { std::string_view argv0 = argc > 0 ? argv[0] : "crafter-build"; diff --git a/implementations/Crafter.Build-External.cpp b/implementations/Crafter.Build-External.cpp index fb2ed44..a6db151 100644 --- a/implementations/Crafter.Build-External.cpp +++ b/implementations/Crafter.Build-External.cpp @@ -206,6 +206,25 @@ std::string BuildCMake(const fs::path& cmakeBuildDir) { } // namespace +fs::path Crafter::ExternalCloneDir(const ExternalDependency& dep, std::string_view target) { + std::string name = dep.name.empty() ? DeriveName(dep.source) : dep.name; + if (name.empty()) return {}; + std::string keyMaterial = std::format("{}|{}|{}|{}|{}", dep.source.url, dep.source.branch, dep.source.commit, JoinOptions(dep.options), target); + std::size_t key = std::hash{}(keyMaterial); + return GetCacheDir() / "external" / std::format("{}-{:016x}", name, key); +} + +std::vector Crafter::ExternalIncludeFlags(const ExternalDependency& dep, std::string_view target) { + std::vector flags; + fs::path cloneDir = ExternalCloneDir(dep, target); + if (cloneDir.empty()) return flags; + for (const fs::path& include : dep.includeDirs) { + fs::path full = include.empty() ? cloneDir : cloneDir / include; + flags.push_back(std::format("-I{}", fs::absolute(full).string())); + } + return flags; +} + ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::string_view target, std::atomic& cancelled) { ExternalBuildResult result; @@ -227,9 +246,7 @@ ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::s } } - std::string keyMaterial = std::format("{}|{}|{}|{}|{}", dep.source.url, dep.source.branch, dep.source.commit, JoinOptions(dep.options), target); - std::size_t key = std::hash{}(keyMaterial); - fs::path cloneDir = externalRoot / std::format("{}-{:016x}", name, key); + fs::path cloneDir = ExternalCloneDir(dep, target); std::string fetchErr = FetchGit(dep.source, cloneDir); if (!fetchErr.empty()) { @@ -253,10 +270,7 @@ ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::s } } - for (const fs::path& include : dep.includeDirs) { - fs::path full = include.empty() ? cloneDir : cloneDir / include; - result.compileFlags.push_back(std::format("-I{}", fs::absolute(full).string())); - } + result.compileFlags = ExternalIncludeFlags(dep, target); if (dep.builder == ExternalBuilder::CMake) { // Each search path gets both a -L (link-time) and a -Wl,-rpath diff --git a/implementations/Crafter.Build-Lint.cpp b/implementations/Crafter.Build-Lint.cpp index 1c41544..0defc5a 100644 --- a/implementations/Crafter.Build-Lint.cpp +++ b/implementations/Crafter.Build-Lint.cpp @@ -74,7 +74,6 @@ namespace { decltype(&clang_getCursorLocation) GetCursorLocation = nullptr; decltype(&clang_getCursorExtent) GetCursorExtent = nullptr; decltype(&clang_getCursorReferenced) GetCursorReferenced = nullptr; - decltype(&clang_getCursorLanguage) GetCursorLanguage = nullptr; decltype(&clang_Cursor_getStorageClass) GetStorageClass = nullptr; decltype(&clang_EnumDecl_isScoped) EnumDeclIsScoped = nullptr; decltype(&clang_isCursorDefinition) IsCursorDefinition = nullptr; @@ -88,6 +87,7 @@ namespace { decltype(&clang_disposeString) DisposeString = nullptr; decltype(&clang_getFileName) GetFileName = nullptr; decltype(&clang_Cursor_isNull) CursorIsNull = nullptr; + decltype(&clang_getResultType) GetResultType = nullptr; }; LibClang LoadLibClang() { @@ -152,7 +152,6 @@ namespace { bind(lib.GetCursorLocation, "clang_getCursorLocation"); bind(lib.GetCursorExtent, "clang_getCursorExtent"); bind(lib.GetCursorReferenced, "clang_getCursorReferenced"); - bind(lib.GetCursorLanguage, "clang_getCursorLanguage"); bind(lib.GetStorageClass, "clang_Cursor_getStorageClass"); bind(lib.EnumDeclIsScoped, "clang_EnumDecl_isScoped"); bind(lib.IsCursorDefinition, "clang_isCursorDefinition"); @@ -166,6 +165,7 @@ namespace { bind(lib.DisposeString, "clang_disposeString"); bind(lib.GetFileName, "clang_getFileName"); bind(lib.CursorIsNull, "clang_Cursor_isNull"); + bind(lib.GetResultType, "clang_getResultType"); if (!missing.empty()) { lib.handle = nullptr; lib.error = std::format("loaded {} but it is missing {}", candidates.front(), join(missing)); @@ -265,10 +265,10 @@ namespace { // ---------------- AST ---------------- std::string TakeString(const LibClang& lc, CXString s) { - // clang_getCString returns const char*; the spelling is libclang's, not - // ours. Suppressed by hand until no-char-pointer reads the AST and can - // see that for itself. - // lint-disable-next-line no-char-pointer + // const char* because clang_getCString returns one. no-char-pointer + // works this out for itself now: the initialiser resolves to a + // declaration in clang-c/, outside the project, so the declaration is + // flagged as foreign API and exempt. No suppression comment needed. const char* raw = lc.GetCString(s); std::string out = raw ? raw : ""; lc.DisposeString(s); @@ -363,8 +363,20 @@ namespace { const fs::path* projectRoot = nullptr; std::vector* out = nullptr; std::vector stack; // indices of the enclosing declarations + std::int32_t externCDepth = 0; // inside how many extern "C" blocks }; + // 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. + bool IsExternCLinkage(std::string_view text) { + std::size_t quote = text.find('"'); + if (quote == std::string_view::npos) return false; + std::size_t close = text.find('"', quote + 1); + if (close == std::string_view::npos) return false; + return text.substr(quote + 1, close - quote - 1) == "C"; + } + bool PathInsideRoot(const fs::path& p, const fs::path& root); // True when `cursor` names something declared outside the project — a @@ -398,18 +410,41 @@ namespace { // std alone. Prune before doing any work. if (!lc.LocationIsFromMainFile(location)) return CXChildVisit_Continue; - LintDeclKind mapped = MapCursorKind(lc.GetCursorKind(cursor)); + CXCursorKind kind = lc.GetCursorKind(cursor); + LintDeclKind mapped = MapCursorKind(kind); if (mapped == LintDeclKind::Other) { - // Not a declaration we model, but a reference to a foreign entity - // inside one — an argument to a libc call, a member of a libc++ - // type — is exactly what marks the enclosing declaration as sitting - // on an interop boundary. - if (!walk.stack.empty() && ResolvesOutsideProject(lc, cursor, *walk.projectRoot)) { - (*walk.out)[walk.stack.back()].isForeignApi = true; + // A foreign reference inside a VARIABLE, FIELD or PARAMETER is an + // initialiser binding that declaration to somebody else's API — + // `char* p = getenv(...)`. Deliberately not applied when the + // enclosing declaration is a function: a function that merely + // touches libc++ somewhere in its body would otherwise exempt its + // 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; + if (initialiserContext && ResolvesOutsideProject(lc, cursor, *walk.projectRoot)) { + enclosing.isForeignApi = true; + } } // Recursed by hand rather than with CXChildVisit_Recurse so the // enclosing-declaration stack stays accurate: the callback is never // told when a subtree ends. + if (kind == CXCursor_LinkageSpec) { + CXSourceRange extent = lc.GetCursorExtent(cursor); + std::uint32_t specBegin = 0; + std::uint32_t specEnd = 0; + lc.GetFileLocation(lc.GetRangeStart(extent), nullptr, nullptr, nullptr, &specBegin); + lc.GetFileLocation(lc.GetRangeEnd(extent), nullptr, nullptr, nullptr, &specEnd); + std::string_view text; + 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); + if (isC) ++walk.externCDepth; + lc.VisitChildren(cursor, &VisitDecl, &walk); + if (isC) --walk.externCDepth; + return CXChildVisit_Continue; + } lc.VisitChildren(cursor, &VisitDecl, &walk); return CXChildVisit_Continue; } @@ -417,7 +452,12 @@ namespace { LintDecl decl; decl.kind = mapped; decl.name = TakeString(lc, lc.GetCursorSpelling(cursor)); - decl.type = TakeString(lc, lc.GetTypeSpelling(lc.GetCursorType(cursor))); + // For anything callable, `type` is the RESULT type rather than the + // 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; + decl.type = TakeString(lc, lc.GetTypeSpelling(callable ? lc.GetResultType(lc.GetCursorType(cursor)) : lc.GetCursorType(cursor))); CXFile nameFile = nullptr; std::uint32_t nameLine = 0; std::uint32_t nameColumn = 0; @@ -435,9 +475,11 @@ namespace { decl.isDefinition = lc.IsCursorDefinition(cursor) != 0; decl.isStatic = lc.GetStorageClass(cursor) == CX_SC_Static; decl.isScopedEnum = mapped == LintDeclKind::Enum && lc.EnumDeclIsScoped(cursor) != 0; - // C language linkage: set for anything inside an extern "C" block, which - // is where a C API's spelling is not ours to modernise. - decl.isExternC = lc.GetCursorLanguage(cursor) == CXLanguage_C; + // Inside an extern "C" block, where a C API's spelling is not ours to + // modernise. NOT clang_getCursorLanguage: its default answer for a + // plain function, variable or parameter is CXLanguage_C even in a C++ + // translation unit, so trusting it exempted essentially everything. + decl.isExternC = walk.externCDepth > 0; // libclang exposes no constexpr query. The keyword can only appear in // this declaration's own specifier list, i.e. between the start of its // extent and its name, so a search bounded to that span is exact rather @@ -924,7 +966,12 @@ format` applies it to disk, and `crafter-build lint` reports where it would. std::string command; try { CompileCommand assembled = GetCompileCommand(cfg); - command = assembled.command; + // Sources that #include an external dependency's headers — + // Crafter.Build-Shader.cpp and glslang here — need those -I + // flags to parse at all. Build appends its own authoritative + // set after the external build; these come straight from the + // declaration, which is all a parse needs. + command = assembled.command + assembled.externalIncludeFlags; if (!fs::exists(assembled.stdPcmDir/"std.pcm")) { Progress::Task task(std::format("Building std PCM ({}-{})", cfg.target, cfg.march)); fs::create_directories(assembled.stdPcmDir); @@ -963,6 +1010,15 @@ format` applies it to disk, and `crafter-build lint` reports where it would. // decides what is safe to touch. Skip it and make the run fail. if (rule->needsAst) { if (opts.noAst) continue; + // No compile command means this file is not a translation unit + // of the build graph — project.cpp, which LoadProject compiles + // with its own flags, or a header. A semantic rule does not + // apply there, the same way a rule self-filters by extension, + // so skip it quietly. That is a different thing from a file we + // SHOULD have been able to parse and could not, which is an + // error: reporting nothing for it would be indistinguishable + // from reporting it clean. + if (ctx.compileCommand.empty()) continue; if (!ctx.AstAvailable()) { ctx.Report(0, std::format("rule '{}' needs an AST, which is unavailable: {}", rule->name, ctx.AstUnavailableReason())); ++summary.errors; diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index 9b1e19c..d35ac23 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -536,6 +536,12 @@ export namespace Crafter { std::string userFlags; std::string ltoCompileFlags; std::string ltoLinkFlags; + // -I flags from external dependencies' declared includeDirs, for this + // configuration and its dependencies. Deliberately NOT folded into + // `command`: Build appends the authoritative set from the external build + // results instead. Exposed for callers that only PARSE sources and so + // cannot wait for a build to tell them where the headers are. + std::string externalIncludeFlags; // ThinLTO is on: objects hold bitcode, so archiving needs llvm-ar. bool useLto = false; fs::path stdPcmDir; @@ -545,6 +551,9 @@ export namespace Crafter { CRAFTER_API BuildResult Build(Configuration& config, std::unordered_map>& depResults, std::mutex& depMutex); + // Takes main's argv verbatim so the CLI entry point is a one-liner; + // the shape is inherited from the language, not chosen here. + // lint-disable-next-line no-char-pointer CRAFTER_API int Run(int argc, char** argv); // Delete the bin/ and build/ trees beside `projectFile`, returning the paths diff --git a/interfaces/Crafter.Build-External.cppm b/interfaces/Crafter.Build-External.cppm index 868b943..df5b190 100644 --- a/interfaces/Crafter.Build-External.cppm +++ b/interfaces/Crafter.Build-External.cppm @@ -44,6 +44,16 @@ export namespace Crafter { fs::file_time_type latestArtifact = fs::file_time_type::min(); }; + // Where this dependency's clone lives in the cache. A pure function of the + // declaration and target — BuildExternal derives its own working directory + // through this, so a caller that only needs to know where the headers ended + // up cannot drift from where they actually are. + CRAFTER_API fs::path ExternalCloneDir(const ExternalDependency& dep, std::string_view target); + // The -I flags this dependency contributes, from its declared includeDirs. + // Also pure, which is what lets the linter's AST layer parse a source that + // includes an external's headers without running a build to find out where + // they are. The paths only resolve once something has fetched the clone. + CRAFTER_API std::vector ExternalIncludeFlags(const ExternalDependency& dep, std::string_view target); CRAFTER_API ExternalBuildResult BuildExternal(const ExternalDependency& dep, std::string_view target, std::atomic& cancelled); // Specification for a sibling crafter-build project to fetch and depend on. diff --git a/lint-rules.h b/lint-rules.h index 3cf8c92..b9c1dd6 100644 --- a/lint-rules.h +++ b/lint-rules.h @@ -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 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 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 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)); } }); diff --git a/tests/HouseRules/main.cpp b/tests/HouseRules/main.cpp index 7afa0a5..34c6398 100644 --- a/tests/HouseRules/main.cpp +++ b/tests/HouseRules/main.cpp @@ -295,6 +295,38 @@ int main() { Check(r.text == source, "fixed-width-types leaves type names inside a raw string alone"); } + // no-char-pointer reads the AST, so it distinguishes OUR char* from one + // whose spelling belongs to somebody else's header. This is what replaced + // the substring denylist (argv, getenv, c_str, reinterpret_cast, …): each + // entry there disabled the rule for a whole line, and the list could only + // grow as new libraries arrived. + { + RuleRun r = RunRule("#include \n" + "#include \n" + "extern \"C\" const char* CApiEntry(const char* path);\n" + "char* OurBadApi(char* input) { return input; }\n" + "void F() {\n" + " char* fromLibc = std::getenv(\"HOME\");\n" + " std::string mine = \"ok\";\n" + " const char* toLibc = mine.c_str();\n" + " auto raw = reinterpret_cast(&mine);\n" + "}\n", + "no-char-pointer", LintMode::Report); + // Ours, so reported: the declaration and its parameter. + Check(HasFinding(r.summary, "'OurBadApi'"), "no-char-pointer: our own char* return is reported"); + Check(HasFinding(r.summary, "'input'"), "no-char-pointer: our own char* parameter is reported"); + // Foreign, so exempt — each for a reason, not by name. + Check(!HasFinding(r.summary, "'CApiEntry'"), "no-char-pointer: extern \"C\" declaration is exempt"); + Check(!HasFinding(r.summary, "'path'"), "no-char-pointer: extern \"C\" parameter is exempt"); + Check(!HasFinding(r.summary, "'fromLibc'"), "no-char-pointer: a value from libc is exempt"); + Check(!HasFinding(r.summary, "'toLibc'"), "no-char-pointer: a value from c_str() is exempt"); + // A local whose deduced type is char* is still our declaration, so it + // is reported. The old denylist exempted every line mentioning + // reinterpret_cast; a deliberate low-level cast now takes an explicit + // lint-disable comment, which is at least visible at the site. + Check(HasFinding(r.summary, "'raw'"), "no-char-pointer: a deduced char* local is still ours"); + } + if (Failures > 0) { std::println(std::cerr, "{} assertions failed", Failures); return 1;