diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -23,11 +23,6 @@ namespace clangd { namespace { -bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) { - auto FE = SM.getFileManager().getFile(FileName); - return FE && *FE == SM.getFileEntryForID(SM.getMainFileID()); -} - class RecordHeaders : public PPCallbacks { public: RecordHeaders(const SourceManager &SM, IncludeStructure *Out) diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -376,8 +376,7 @@ const auto *ME = llvm::dyn_cast(E->IgnoreCasts()); if (!ME || !llvm::isa(ME->getBase()->IgnoreCasts())) return llvm::None; - const auto *Field = - llvm::dyn_cast(ME->getMemberDecl()); + const auto *Field = llvm::dyn_cast(ME->getMemberDecl()); if (!Field || !Field->getDeclName().isIdentifier()) return llvm::None; return Field->getDeclName().getAsIdentifierInfo()->getName(); @@ -555,7 +554,14 @@ // Try to get the full definition, not just the name SourceLocation StartLoc = Macro.Info->getDefinitionLoc(); SourceLocation EndLoc = Macro.Info->getDefinitionEndLoc(); - if (EndLoc.isValid()) { + // Ensure that EndLoc is a valid offset. For example it might come from + // preamble, and source file might've changed, in such a scenario EndLoc still + // stays valid, but getLocForEndOfToken will fail as it is no longer a valid + // offset. + // Note that this check is just to ensure there's text data inside the range. + // It will still succeed even when the data inside the range is irrelevant to + // macro definition. + if (SM.getPresumedLoc(EndLoc, /*UseLineDirectives=*/false).isValid()) { EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM, AST.getLangOpts()); bool Invalid; StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid); @@ -706,7 +712,7 @@ if (Tok.kind() == tok::identifier) { // Prefer the identifier token as a fallback highlighting range. HighlightRange = Tok.range(SM).toCharRange(SM); - if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) { + if (auto M = locateMacroAt(Tok, AST.getPreprocessor(), AST.getTokens())) { HI = getHoverContents(*M, AST); break; } @@ -869,20 +875,20 @@ if (!Suffix.empty() && !AfterEndChars.contains(Suffix.front())) return llvm::None; - return Line.slice(Offset, Next+1); + return Line.slice(Offset, Next + 1); } void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) { // Probably this is appendText(Line), but scan for something interesting. for (unsigned I = 0; I < Line.size(); ++I) { switch (Line[I]) { - case '`': - if (auto Range = getBacktickQuoteRange(Line, I)) { - Out.appendText(Line.substr(0, I)); - Out.appendCode(Range->trim("`"), /*Preserve=*/true); - return parseDocumentationLine(Line.substr(I+Range->size()), Out); - } - break; + case '`': + if (auto Range = getBacktickQuoteRange(Line, I)) { + Out.appendText(Line.substr(0, I)); + Out.appendCode(Range->trim("`"), /*Preserve=*/true); + return parseDocumentationLine(Line.substr(I + Range->size()), Out); + } + break; } } Out.appendText(Line).appendSpace(); 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 @@ -423,8 +423,10 @@ // Note that we deliberately ignore conditional directives and undefs to // reduce complexity. The former might cause problems because scanning is // imprecise and might pick directives from disabled regions. - for (const auto &TD : ModifiedScan->TextualDirectives) + for (const auto &TD : ModifiedScan->TextualDirectives) { + Patch << "#line " << TD.DirectiveLine << '\n'; Patch << TD.Text << '\n'; + } } dlog("Created preamble patch: {0}", Patch.str()); Patch.flush(); diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -292,11 +292,15 @@ struct DefinedMacro { llvm::StringRef Name; const MacroInfo *Info; + /// Subject to #line directive substitution. E.g. a macro defined in a + /// built-in file might have an identifier location inside the main file. + SourceLocation IdentLoc; }; /// Gets the macro referenced by \p SpelledTok. It must be a spelled token /// aligned to the beginning of an identifier. llvm::Optional locateMacroAt(const syntax::Token &SpelledTok, - Preprocessor &PP); + Preprocessor &PP, + const syntax::TokenBuffer &TB); /// Infers whether this is a header from the FileName and LangOpts (if /// presents). @@ -306,6 +310,9 @@ /// Returns true if the given location is in a generated protobuf file. bool isProtoFile(SourceLocation Loc, const SourceManager &SourceMgr); +/// Checks whether \p FileName is a valid spelling of main file. +bool isMainFile(llvm::StringRef FileName, const SourceManager &SM); + } // namespace clangd } // namespace clang #endif diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -945,8 +945,14 @@ return Result; } +bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) { + auto FE = SM.getFileManager().getFile(FileName); + return FE && *FE == SM.getFileEntryForID(SM.getMainFileID()); +} + llvm::Optional locateMacroAt(const syntax::Token &SpelledTok, - Preprocessor &PP) { + Preprocessor &PP, + const syntax::TokenBuffer &TB) { SourceLocation Loc = SpelledTok.location(); assert(Loc.isFileID()); const auto &SM = PP.getSourceManager(); @@ -960,9 +966,45 @@ if (SM.getLocForStartOfFile(SM.getFileID(Loc)) != Loc) Loc = Loc.getLocWithOffset(-1); MacroDefinition MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc); - if (auto *MI = MacroDef.getMacroInfo()) - return DefinedMacro{IdentifierInfo->getName(), MI}; - return None; + auto *MI = MacroDef.getMacroInfo(); + if (!MI) + return None; + + SourceLocation IdentLoc = MI->getDefinitionLoc(); + + // Macro definitions could be injected through preamble patch. These contain + // line directives to hint their original location in main file. + auto DefFile = SM.getFileID(IdentLoc); + auto IncludeLoc = SM.getIncludeLoc(DefFile); + // Preamble patch is included inside the builtin file. + if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc)) { + auto Presumed = SM.getPresumedLoc(IdentLoc); + // Check that line directive is pointing at main file. + if (Presumed.isValid() && Presumed.getFileID().isInvalid() && + isMainFile(Presumed.getFilename(), SM)) { + auto Presumed = SM.getPresumedLoc(IdentLoc); + // Now find the spelled token for macro identifier. The line looks like: + // # /*comment*/ define /*comment*/ MACRO_IDENT.... + auto LocForStartOfLine = + SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(), 1); + auto SpelledToks = TB.spelledTokens(SM.getMainFileID()); + auto *MacroIdent = llvm::partition_point( + SpelledToks, [&LocForStartOfLine](const syntax::Token &Tok) { + return Tok.location() < LocForStartOfLine; + }); + assert(MacroIdent); + // This loop should terminate in a few iterations, unless something went + // really wrong. + while (MacroIdent < SpelledToks.end() && + MacroIdent->text(SM) != SpelledTok.text(SM)) + ++MacroIdent; + + if (MacroIdent < SpelledToks.end()) + IdentLoc = MacroIdent->location(); + } + } + + return DefinedMacro{IdentifierInfo->getName(), MI, IdentLoc}; } llvm::Expected Edit::apply() const { diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -205,9 +205,10 @@ llvm::Optional locateMacroReferent(const syntax::Token &TouchedIdentifier, ParsedAST &AST, llvm::StringRef MainFilePath) { - if (auto M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor())) { - if (auto Loc = makeLocation(AST.getASTContext(), - M->Info->getDefinitionLoc(), MainFilePath)) { + if (auto M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor(), + AST.getTokens())) { + if (auto Loc = + makeLocation(AST.getASTContext(), M->IdentLoc, MainFilePath)) { LocatedSymbol Macro; Macro.Name = std::string(M->Name); Macro.PreferredDeclaration = *Loc; @@ -749,7 +750,8 @@ llvm::Optional Macro; if (const auto *IdentifierAtCursor = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens())) { - Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor()); + Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(), + AST.getTokens()); } RefsRequest Req; @@ -870,7 +872,8 @@ if (!IdentifierAtCursor) return Results; - if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor())) { + if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(), + AST.getTokens())) { SymbolDetails NewMacro; NewMacro.name = std::string(M->Name); llvm::SmallString<32> USR; diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -477,7 +477,7 @@ return makeError(ReasonToReject::NoSymbolFound); // FIXME: Renaming macros is not supported yet, the macro-handling code should // be moved to rename tooling library. - if (locateMacroAt(*IdentifierToken, AST.getPreprocessor())) + if (locateMacroAt(*IdentifierToken, AST.getPreprocessor(), AST.getTokens())) return makeError(ReasonToReject::UnsupportedSymbol); auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location()); diff --git a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp --- a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp +++ b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp @@ -91,7 +91,7 @@ ASSERT_TRUE(bool(Loc)); const auto *Id = syntax::spelledIdentifierTouching(*Loc, AST.getTokens()); ASSERT_TRUE(Id); - auto Macro = locateMacroAt(*Id, PP); + auto Macro = locateMacroAt(*Id, PP, AST.getTokens()); assert(Macro); auto SID = getSymbolID(Macro->Name, Macro->Info, SM); diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -9,14 +9,19 @@ #include "Annotations.h" #include "Compiler.h" #include "Headers.h" +#include "Hover.h" #include "Preamble.h" +#include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" +#include "XRefs.h" +#include "clang/Format/Format.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Lex/PreprocessorOptions.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/VirtualFileSystem.h" #include "gmock/gmock.h" @@ -28,6 +33,7 @@ using testing::Contains; using testing::Field; +using testing::Matcher; namespace clang { namespace clangd { @@ -198,7 +204,7 @@ ADD_FAILURE() << "Failed to build compiler invocation"; return llvm::None; } - return ParsedAST::build(testPath("main.cpp"), TU.inputs(), std::move(CI), {}, + return ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI), {}, BaselinePreamble); } @@ -243,6 +249,171 @@ EXPECT_THAT(AST->getDiagnostics(), Not(Contains(Field(&Diag::Range, Modified.range())))); } + +TEST(PreamblePatchTest, LocateMacroAtWorks) { + struct { + llvm::StringLiteral Baseline; + llvm::StringLiteral Modified; + } Cases[] = { + // Addition of new directive + { + "", + R"cpp( + #define $def^FOO + $use^FOO)cpp", + }, + // Available inside preamble section + { + "", + R"cpp( + #define $def^FOO + #undef $use^FOO)cpp", + }, + // Available after undef, as we don't patch those + { + "", + R"cpp( + #define $def^FOO + #undef FOO + $use^FOO)cpp", + }, + // Identifier on a different line + { + "", + R"cpp( + #define \ + $def^FOO + $use^FOO)cpp", + }, + // In presence of comment tokens + { + "", + R"cpp( + #\ + define /* FOO */\ + /* FOO */ $def^FOO + $use^FOO)cpp", + }, + // Moved around + { + "#define FOO", + R"cpp( + #define BAR + #define $def^FOO + $use^FOO)cpp", + }, + }; + for (const auto &Case : Cases) { + SCOPED_TRACE(Case.Modified); + llvm::Annotations Modified(Case.Modified); + auto AST = createPatchedAST(Case.Baseline, Modified.code()); + ASSERT_TRUE(AST); + + const auto &SM = AST->getSourceManager(); + auto *MacroTok = AST->getTokens().spelledTokenAt( + SM.getComposedLoc(SM.getMainFileID(), Modified.point("use"))); + ASSERT_TRUE(MacroTok); + + auto FoundMacro = + locateMacroAt(*MacroTok, AST->getPreprocessor(), AST->getTokens()); + ASSERT_TRUE(FoundMacro); + EXPECT_THAT(FoundMacro->Name, "FOO"); + + auto MacroLoc = FoundMacro->IdentLoc; + EXPECT_EQ(SM.getFileID(MacroLoc), SM.getMainFileID()); + EXPECT_EQ(SM.getFileOffset(MacroLoc), Modified.point("def")); + } +} + +TEST(PreamblePatchTest, LocateMacroAtDeletion) { + { + // We don't patch deleted define directives, make sure we don't crash. + llvm::StringLiteral Baseline = "#define FOO"; + llvm::Annotations Modified("^FOO"); + + auto AST = createPatchedAST(Baseline, Modified.code()); + ASSERT_TRUE(AST); + + const auto &SM = AST->getSourceManager(); + auto *MacroTok = AST->getTokens().spelledTokenAt( + SM.getComposedLoc(SM.getMainFileID(), Modified.point())); + ASSERT_TRUE(MacroTok); + + auto FoundMacro = + locateMacroAt(*MacroTok, AST->getPreprocessor(), AST->getTokens()); + ASSERT_TRUE(FoundMacro); + EXPECT_THAT(FoundMacro->Name, "FOO"); + auto HI = + getHover(*AST, offsetToPosition(Modified.code(), Modified.point()), + format::getLLVMStyle(), nullptr); + ASSERT_TRUE(HI); + EXPECT_THAT(HI->Definition, testing::IsEmpty()); + } + + { + // Offset is valid, but underlying text is different. + llvm::StringLiteral Baseline = "#define FOO"; + Annotations Modified(R"cpp(#define BAR + ^FOO")cpp"); + + auto AST = createPatchedAST(Baseline, Modified.code()); + ASSERT_TRUE(AST); + + auto HI = getHover(*AST, Modified.point(), format::getLLVMStyle(), nullptr); + ASSERT_TRUE(HI); + EXPECT_THAT(HI->Definition, "#define BAR"); + } +} + +TEST(PreamblePatchTest, RefsToMacros) { + struct { + llvm::StringLiteral Baseline; + llvm::StringLiteral Modified; + } Cases[] = { + // Newly added + { + "", + R"cpp( + #define ^FOO + ^[[FOO]])cpp", + }, + // Moved around + { + "#define FOO", + R"cpp( + #define BAR + #define ^FOO + ^[[FOO]])cpp", + }, + // Ref in preamble section + { + "", + R"cpp( + #define ^FOO + #undef ^FOO)cpp", + }, + }; + + for (const auto &Case : Cases) { + Annotations Modified(Case.Modified); + auto AST = createPatchedAST("", Modified.code()); + ASSERT_TRUE(AST); + + const auto &SM = AST->getSourceManager(); + std::vector> ExpectedLocations; + for (const auto &R : Modified.ranges()) + ExpectedLocations.push_back(Field(&Location::range, R)); + + for (const auto &P : Modified.points()) { + auto *MacroTok = AST->getTokens().spelledTokenAt(SM.getComposedLoc( + SM.getMainFileID(), + llvm::cantFail(positionToOffset(Modified.code(), P)))); + ASSERT_TRUE(MacroTok); + EXPECT_THAT(findReferences(*AST, P, 0).References, + testing::ElementsAreArray(ExpectedLocations)); + } + } +} } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp --- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp +++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp @@ -514,7 +514,7 @@ ASSERT_TRUE(bool(CurLoc)); const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()); ASSERT_TRUE(Id); - auto Result = locateMacroAt(*Id, AST.getPreprocessor()); + auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens()); ASSERT_TRUE(Result); EXPECT_THAT(*Result, MacroName("MACRO")); } @@ -528,7 +528,7 @@ ASSERT_TRUE(bool(CurLoc)); const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()); ASSERT_TRUE(Id); - auto Result = locateMacroAt(*Id, AST.getPreprocessor()); + auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens()); ASSERT_TRUE(Result); EXPECT_THAT(*Result, MacroName("MACRO")); }