diff --git a/clang-tools-extra/clangd/CollectMacros.h b/clang-tools-extra/clangd/CollectMacros.h --- a/clang-tools-extra/clangd/CollectMacros.h +++ b/clang-tools-extra/clangd/CollectMacros.h @@ -24,6 +24,8 @@ // SourceManager from preamble is not available when we build the AST. Range Rng; bool IsDefinition; + // True if the occurence is used in a conditional directive, e.g. #ifdef MACRO + bool InConditionalDirective; }; struct MainFileMacros { @@ -43,17 +45,16 @@ /// - collect macros after the preamble of the main file (in ParsedAST.cpp) class CollectMainFileMacros : public PPCallbacks { public: - explicit CollectMainFileMacros(const SourceManager &SM, MainFileMacros &Out) - : SM(SM), Out(Out) {} + explicit CollectMainFileMacros(const SourceManager &SM, + const Preprocessor &PP, MainFileMacros &Out) + : SM(SM), PP(PP), Out(Out) {} void FileChanged(SourceLocation Loc, FileChangeReason, SrcMgr::CharacteristicKind, FileID) override { InMainFile = isInsideMainFile(Loc, SM); } - void MacroDefined(const Token &MacroName, const MacroDirective *MD) override { - add(MacroName, MD->getMacroInfo(), /*IsDefinition=*/true); - } + void MacroDefined(const Token &MacroName, const MacroDirective *MD) override; void MacroExpands(const Token &MacroName, const MacroDefinition &MD, SourceRange Range, const MacroArgs *Args) override { @@ -68,17 +69,20 @@ void Ifdef(SourceLocation Loc, const Token &MacroName, const MacroDefinition &MD) override { - add(MacroName, MD.getMacroInfo()); + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); } void Ifndef(SourceLocation Loc, const Token &MacroName, const MacroDefinition &MD) override { - add(MacroName, MD.getMacroInfo()); + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); } void Defined(const Token &MacroName, const MacroDefinition &MD, SourceRange Range) override { - add(MacroName, MD.getMacroInfo()); + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); } void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override { @@ -91,8 +95,9 @@ private: void add(const Token &MacroNameTok, const MacroInfo *MI, - bool IsDefinition = false); + bool IsDefinition = false, bool InConditionalDirective = false); const SourceManager &SM; + const Preprocessor &PP; bool InMainFile = true; MainFileMacros &Out; }; diff --git a/clang-tools-extra/clangd/CollectMacros.cpp b/clang-tools-extra/clangd/CollectMacros.cpp --- a/clang-tools-extra/clangd/CollectMacros.cpp +++ b/clang-tools-extra/clangd/CollectMacros.cpp @@ -9,12 +9,13 @@ #include "CollectMacros.h" #include "AST.h" #include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/STLExtras.h" namespace clang { namespace clangd { void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI, - bool IsDefinition) { + bool IsDefinition, bool InIfCondition) { if (!InMainFile) return; auto Loc = MacroNameTok.getLocation(); @@ -26,9 +27,9 @@ auto Range = halfOpenToRange( SM, CharSourceRange::getCharRange(Loc, MacroNameTok.getEndLoc())); if (auto SID = getSymbolID(Name, MI, SM)) - Out.MacroRefs[SID].push_back({Range, IsDefinition}); + Out.MacroRefs[SID].push_back({Range, IsDefinition, InIfCondition}); else - Out.UnknownMacros.push_back({Range, IsDefinition}); + Out.UnknownMacros.push_back({Range, IsDefinition, InIfCondition}); } class CollectPragmaMarks : public PPCallbacks { @@ -58,5 +59,24 @@ return std::make_unique(SM, Out); } +void CollectMainFileMacros::MacroDefined(const Token &MacroName, + const MacroDirective *MD) { + + if (!InMainFile) + return; + const auto *MI = MD->getMacroInfo(); + add(MacroName, MD->getMacroInfo(), true); + if (MI) + for (const auto &Tok : MI->tokens()) { + auto *II = Tok.getIdentifierInfo(); + // Could this token be a reference to a macro? (Not param to this macro). + if (!II || !II->hadMacroDefinition() || + llvm::is_contained(MI->params(), II)) + continue; + if (const MacroInfo *MI = PP.getMacroInfo(II)) + add(Tok, MI); + } +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -164,22 +164,34 @@ } std::vector -collectMacroReferences(ParsedAST &AST) { +convertMacroReferences(ParsedAST &AST) { const auto &SM = AST.getSourceManager(); - // FIXME: !!this is a hacky way to collect macro references. - std::vector Macros; + const auto &MacroRefs = AST.getMacros(); auto &PP = AST.getPreprocessor(); - for (const syntax::Token &Tok : - AST.getTokens().spelledTokens(SM.getMainFileID())) { - auto Macro = locateMacroAt(Tok, PP); - if (!Macro) - continue; - if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) - Macros.push_back( - {Tok.location(), - include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)), - DefLoc}, - include_cleaner::RefType::Explicit}); + std::vector Macros; + for (const auto &MAndRefs : MacroRefs.MacroRefs) { + for (const auto &Ref : MAndRefs.second) { + auto L = sourceLocationInMainFile(SM, Ref.Rng.start); + if (!L) { + llvm::consumeError(L.takeError()); + continue; + } + if (const auto *Tok = + syntax::spelledIdentifierTouching(*L, AST.getTokens())) { + auto Macro = locateMacroAt(*Tok, PP); + if (!Macro) + continue; + + if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) + Macros.push_back( + {Tok->location(), + include_cleaner::Macro{ + /*Name=*/PP.getIdentifierInfo(Tok->text(SM)), DefLoc}, + Ref.InConditionalDirective + ? include_cleaner::RefType::Implicit + : include_cleaner::RefType::Explicit}); + } + } } return Macros; } @@ -350,7 +362,7 @@ const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); std::vector Macros = - collectMacroReferences(AST); + convertMacroReferences(AST); std::vector MissingIncludes; llvm::DenseSet Used; trace::Span Tracer("include_cleaner::walkUsed"); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -608,8 +608,8 @@ if (Preamble) Macros = Preamble->Macros; Clang->getPreprocessor().addPPCallbacks( - std::make_unique(Clang->getSourceManager(), - Macros)); + std::make_unique( + Clang->getSourceManager(), Clang->getPreprocessor(), Macros)); std::vector Marks; // FIXME: We need to patch the marks for stale preambles. diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -126,6 +126,7 @@ CanonIncludes.addSystemHeadersMapping(CI.getLangOpts()); LangOpts = &CI.getLangOpts(); SourceMgr = &CI.getSourceManager(); + PP = &CI.getPreprocessor(); Includes.collect(CI); if (Config::current().Diagnostics.UnusedIncludes == Config::IncludesPolicy::Strict || @@ -137,11 +138,11 @@ } std::unique_ptr createPPCallbacks() override { - assert(SourceMgr && LangOpts && - "SourceMgr and LangOpts must be set at this point"); + assert(SourceMgr && LangOpts && PP && + "SourceMgr, LangOpts and PP must be set at this point"); return std::make_unique( - std::make_unique(*SourceMgr, Macros), + std::make_unique(*SourceMgr, *PP, Macros), collectPragmaMarksCallback(*SourceMgr, Marks)); } @@ -208,6 +209,7 @@ std::unique_ptr IWYUHandler = nullptr; const clang::LangOptions *LangOpts = nullptr; const SourceManager *SourceMgr = nullptr; + const Preprocessor *PP = nullptr; PreambleBuildStats *Stats; bool ParseForwardingFunctions; std::function BeforeExecuteCallback; diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -25,6 +25,7 @@ #include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" #include "llvm/Support/ScopedPrinter.h" +#include "llvm/Testing/Annotations/Annotations.h" #include "llvm/Testing/Support/SupportHelpers.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -51,7 +52,12 @@ "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { return arg.Range == Range && arg.Message == Message; } - +MATCHER_P(Range, R, "") { + return arg.SymRefRange == R; +} +MATCHER_P(SingleHeader, Header, "") { + return arg.Providers.size() == 1 && arg.Providers[0] == Header; +} MATCHER_P3(Fix, Range, Replacement, Message, "Fix " + llvm::to_string(Range) + " => " + ::testing::PrintToString(Replacement) + " = [" + Message + "]") { @@ -169,6 +175,53 @@ EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo)); } +TEST(IncludeCleaner, MissingIncludesForMacroRefs) { + Config Cfg; + Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict; + WithContextValue Ctx(Config::Key, std::move(Cfg)); + llvm::StringRef TestCases[] = { + R"cpp( + #include "all.h" + int a = [[FOO]]; + )cpp", + + R"cpp( + #include "all.h" + #define BAR [[FOO]]; + )cpp", + + // No missing include insertion for ambiguous macro refs. + R"cpp( + #include "all.h" + #if defined(FOO) + #endif + + #ifdef FOO + #endif + )cpp", + }; + + for (const auto& T : TestCases) { + TestTU TU; + llvm::Annotations MainFile = T; + TU.Code = MainFile.code(); + TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\""); + TU.AdditionalFiles["foo.h"] = guard("#define FOO 1"); + ParsedAST AST = TU.build(); + const auto &SM = AST.getSourceManager(); + include_cleaner::Header FooH = *SM.getFileManager().getFile("foo.h"); + + std::vector> Matcher; + for (const auto &R : MainFile.ranges()) + Matcher.push_back( + AllOf(Range(syntax::FileRange(AST.getSourceManager().getMainFileID(), + R.Begin, R.End)), + SingleHeader(FooH))); + EXPECT_THAT(computeIncludeCleanerFindings(AST).MissingIncludes, + testing::UnorderedElementsAreArray(Matcher)); + } +} + TEST(IncludeCleaner, GenerateMissingHeaderDiags) { Config Cfg; Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict; diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp --- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -399,7 +399,7 @@ #define $Macro_decl[[MACRO_CONCAT]](X, V, T) T foo##X = V #define $Macro_decl[[DEF_VAR]](X, V) int X = V #define $Macro_decl[[DEF_VAR_T]](T, X, V) T X = V - #define $Macro_decl[[DEF_VAR_REV]](V, X) DEF_VAR(X, V) + #define $Macro_decl[[DEF_VAR_REV]](V, X) $Macro[[DEF_VAR]](X, V) #define $Macro_decl[[CPY]](X) X #define $Macro_decl[[DEF_VAR_TYPE]](X, Y) X Y #define $Macro_decl[[SOME_NAME]] variable @@ -431,7 +431,7 @@ )cpp", R"cpp( #define $Macro_decl[[fail]](expr) expr - #define $Macro_decl[[assert]](COND) if (!(COND)) { fail("assertion failed" #COND); } + #define $Macro_decl[[assert]](COND) if (!(COND)) { $Macro[[fail]]("assertion failed" #COND); } // Preamble ends. int $Variable_def[[x]]; int $Variable_def[[y]];