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 @@ -120,6 +120,36 @@ } }; +// Spells directive in \p DirectiveRange while prepending it with \p Prefix. +// Returned string can be used with a #line directive on line \p DirectiveLine +// to correctly align with \p DirectiveRange. +std::string spellDirective(llvm::StringRef Prefix, SourceRange DirectiveRange, + unsigned &DirectiveLine, const SourceManager &SM) { + std::string SpelledDirective; + llvm::raw_string_ostream OS(SpelledDirective); + OS << Prefix; + + auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin()); + DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second); + auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1; + + // Pad with spaces before DirectiveRange to make sure it will be on right + // column when patched. + if (SpaceBefore < Prefix.size()) { + // Prefix was longer than the space we had. Pad from start of a new line. + OS << "\\\n" << std::string(SpaceBefore, ' '); + // Decrement because returned string has a line break before directive now. + --DirectiveLine; + } else { + // There is enough space for Prefix and space before directive, use it. + // We try to squeeze the Prefix into the same line whenever we can, as + // putting onto a separate line won't work at the beginning of the file. + OS << std::string(SpaceBefore - Prefix.size(), ' '); + } + OS << toSourceCode(SM, DirectiveRange); + return OS.str(); +} + // Collects #define directives inside the main file. struct DirectiveCollector : public PPCallbacks { DirectiveCollector(const Preprocessor &PP, @@ -140,15 +170,11 @@ TextualDirectives.emplace_back(); TextualPPDirective &TD = TextualDirectives.back(); - auto DecompLoc = SM.getDecomposedLoc(MacroNameTok.getLocation()); - TD.DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second); - SourceRange DefRange( MD->getMacroInfo()->getDefinitionLoc(), Lexer::getLocForEndOfToken(MD->getMacroInfo()->getDefinitionEndLoc(), 0, SM, LangOpts)); - llvm::raw_string_ostream OS(TD.Text); - OS << "#define " << toSourceCode(SM, DefRange); + TD.Text = spellDirective("#define ", DefRange, TD.DirectiveLine, SM); } private: @@ -425,8 +451,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,16 @@ struct DefinedMacro { llvm::StringRef Name; const MacroInfo *Info; + /// Location of the identifier that names the macro. + /// Unlike Info->Location, this translates preamble-patch locations to + /// main-file locations. + SourceLocation NameLoc; }; /// 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 +311,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,27 @@ 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 NameLoc = 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(NameLoc); + auto IncludeLoc = SM.getIncludeLoc(DefFile); + // Preamble patch is included inside the builtin file. + if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc)) { + auto Presumed = SM.getPresumedLoc(NameLoc); + // Check that line directive is pointing at main file. + if (Presumed.isValid() && Presumed.getFileID().isInvalid() && + isMainFile(Presumed.getFilename(), SM)) { + NameLoc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(), + Presumed.getColumn()); + } + } + + return DefinedMacro{IdentifierInfo->getName(), MI, NameLoc}; } 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->NameLoc, MainFilePath)) { LocatedSymbol Macro; Macro.Name = std::string(M->Name); Macro.PreferredDeclaration = *Loc; @@ -765,7 +766,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; @@ -886,7 +888,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; using testing::MatchesRegex; namespace clang { @@ -199,7 +205,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); } @@ -228,7 +234,8 @@ #define BAR [[BAR]])cpp", R"cpp(#line 0 ".*main.cpp" -#define BAR +#line 2 +#define BAR )cpp", }, // multiline macro @@ -238,7 +245,8 @@ [[BAR]])cpp", R"cpp(#line 0 ".*main.cpp" -#define BAR +#line 2 +#define BAR )cpp", }, // multiline macro @@ -248,7 +256,8 @@ BAR [[BAR]])cpp", R"cpp(#line 0 ".*main.cpp" -#define BAR +#line 3 +#define BAR )cpp", }, }; @@ -275,8 +284,10 @@ )cpp"); llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp" -#define BAR\(X, Y\) X Y -#define BAR\(X\) X +#line 2 +#define BAR\(X, Y\) X Y +#line 3 +#define BAR\(X\) X )cpp"); EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()), MatchesRegex(ExpectedPatch.str())); @@ -286,6 +297,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->NameLoc; + 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")); }