[clangd] Show better message when we rename macros.

Summary:
Previously, when we rename a macro, we get an error message of "there is
no symbol found".

This patch improves the message of this case (as we don't support macros).

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, mgrang, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63922

llvm-svn: 364735

Author: hokein

clang-tools-extra/clangd/SourceCode.cpp

@@ -16,6 +16,7 @@
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -653,5 +654,31 @@ llvm::StringSet<> collectWords(llvm::StringRef Content) {
   return Result;
 }
 
+llvm::Optional<DefinedMacro> locateMacroAt(SourceLocation Loc,
+                                           Preprocessor &PP) {
+  const auto &SM = PP.getSourceManager();
+  const auto &LangOpts = PP.getLangOpts();
+  Token Result;
+  if (Lexer::getRawToken(SM.getSpellingLoc(Loc), Result, SM, LangOpts, false))
+    return None;
+  if (Result.is(tok::raw_identifier))
+    PP.LookUpIdentifierInfo(Result);
+  IdentifierInfo *IdentifierInfo = Result.getIdentifierInfo();
+  if (!IdentifierInfo || !IdentifierInfo->hadMacroDefinition())
+    return None;
+
+  std::pair<FileID, unsigned int> DecLoc = SM.getDecomposedExpansionLoc(Loc);
+  // Get the definition just before the searched location so that a macro
+  // referenced in a '#undef MACRO' can still be found.
+  SourceLocation BeforeSearchedLocation =
+      SM.getMacroArgExpandedLocation(SM.getLocForStartOfFile(DecLoc.first)
+                                         .getLocWithOffset(DecLoc.second - 1));
+  MacroDefinition MacroDef =
+      PP.getMacroDefinitionAtLoc(IdentifierInfo, BeforeSearchedLocation);
+  if (auto *MI = MacroDef.getMacroInfo())
+    return DefinedMacro{IdentifierInfo->getName(), MI};
+  return None;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/SourceCode.h

@@ -201,6 +201,14 @@ llvm::StringSet<> collectWords(llvm::StringRef Content);
 std::vector<std::string> visibleNamespaces(llvm::StringRef Code,
                                            const format::FormatStyle &Style);
 
+struct DefinedMacro {
+  llvm::StringRef Name;
+  const MacroInfo *Info;
+};
+// Gets the macro at a specified \p Loc.
+llvm::Optional<DefinedMacro> locateMacroAt(SourceLocation Loc,
+                                           Preprocessor &PP);
+
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/clangd/XRefs.cpp

@@ -128,14 +128,9 @@ SymbolLocation getPreferredLocation(const Location &ASTLoc,
   return Merged.CanonicalDeclaration;
 }
 
-struct MacroDecl {
-  llvm::StringRef Name;
-  const MacroInfo *Info;
-};
-
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
-  std::vector<MacroDecl> MacroInfos;
+  std::vector<DefinedMacro> MacroInfos;
   llvm::DenseSet<const Decl *> Decls;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
@@ -158,16 +153,18 @@ class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
     return Result;
   }
 
-  std::vector<MacroDecl> takeMacroInfos() {
+  std::vector<DefinedMacro> takeMacroInfos() {
     // Don't keep the same Macro info multiple times.
-    llvm::sort(MacroInfos, [](const MacroDecl &Left, const MacroDecl &Right) {
-      return Left.Info < Right.Info;
-    });
-
-    auto Last = std::unique(MacroInfos.begin(), MacroInfos.end(),
-                            [](const MacroDecl &Left, const MacroDecl &Right) {
-                              return Left.Info == Right.Info;
-                            });
+    llvm::sort(MacroInfos,
+               [](const DefinedMacro &Left, const DefinedMacro &Right) {
+                 return Left.Info < Right.Info;
+               });
+
+    auto Last =
+        std::unique(MacroInfos.begin(), MacroInfos.end(),
+                    [](const DefinedMacro &Left, const DefinedMacro &Right) {
+                      return Left.Info == Right.Info;
+                    });
     MacroInfos.erase(Last, MacroInfos.end());
     return std::move(MacroInfos);
   }
@@ -211,38 +208,16 @@ class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
 
 private:
   void finish() override {
-    // Also handle possible macro at the searched location.
-    Token Result;
-    auto &Mgr = AST.getSourceManager();
-    if (!Lexer::getRawToken(Mgr.getSpellingLoc(SearchedLocation), Result, Mgr,
-                            AST.getLangOpts(), false)) {
-      if (Result.is(tok::raw_identifier)) {
-        PP.LookUpIdentifierInfo(Result);
-      }
-      IdentifierInfo *IdentifierInfo = Result.getIdentifierInfo();
-      if (IdentifierInfo && IdentifierInfo->hadMacroDefinition()) {
-        std::pair<FileID, unsigned int> DecLoc =
-            Mgr.getDecomposedExpansionLoc(SearchedLocation);
-        // Get the definition just before the searched location so that a macro
-        // referenced in a '#undef MACRO' can still be found.
-        SourceLocation BeforeSearchedLocation = Mgr.getMacroArgExpandedLocation(
-            Mgr.getLocForStartOfFile(DecLoc.first)
-                .getLocWithOffset(DecLoc.second - 1));
-        MacroDefinition MacroDef =
-            PP.getMacroDefinitionAtLoc(IdentifierInfo, BeforeSearchedLocation);
-        MacroInfo *MacroInf = MacroDef.getMacroInfo();
-        if (MacroInf) {
-          MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
-          assert(Decls.empty());
-        }
-      }
+    if (auto DefinedMacro = locateMacroAt(SearchedLocation, PP)) {
+      MacroInfos.push_back(*DefinedMacro);
+      assert(Decls.empty());
     }
   }
 };
 
 struct IdentifiedSymbol {
   std::vector<const Decl *> Decls;
-  std::vector<MacroDecl> Macros;
+  std::vector<DefinedMacro> Macros;
 };
 
 IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
@@ -740,18 +715,18 @@ static HoverInfo getHoverContents(QualType T, const Decl *D,
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-static HoverInfo getHoverContents(MacroDecl Decl, ParsedAST &AST) {
+static HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) {
   HoverInfo HI;
   SourceManager &SM = AST.getSourceManager();
-  HI.Name = Decl.Name;
+  HI.Name = Macro.Name;
   HI.Kind = indexSymbolKindToSymbolKind(
-      index::getSymbolInfoForMacro(*Decl.Info).Kind);
+      index::getSymbolInfoForMacro(*Macro.Info).Kind);
   // FIXME: Populate documentation
   // FIXME: Pupulate parameters
 
   // Try to get the full definition, not just the name
-  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
-  SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+  SourceLocation StartLoc = Macro.Info->getDefinitionLoc();
+  SourceLocation EndLoc = Macro.Info->getDefinitionEndLoc();
   if (EndLoc.isValid()) {
     EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
                                         AST.getASTContext().getLangOpts());

clang-tools-extra/clangd/refactor/Rename.cpp

@@ -136,6 +136,25 @@ llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
   return ReasonToReject::UsedOutsideFile;
 }
 
+llvm::Error makeError(ReasonToReject Reason) {
+  auto Message = [](ReasonToReject Reason) {
+    switch (Reason) {
+    case NoIndexProvided:
+      return "symbol may be used in other files (no index available)";
+    case UsedOutsideFile:
+      return "the symbol is used outside main file";
+    case NonIndexable:
+      return "symbol may be used in other files (not eligible for indexing)";
+    case UnsupportedSymbol:
+      return "symbol is not a supported kind (e.g. namespace, macro)";
+    }
+    llvm_unreachable("unhandled reason kind");
+  };
+  return llvm::make_error<llvm::StringError>(
+      llvm::formatv("Cannot rename symbol: {0}", Message(Reason)),
+      llvm::inconvertibleErrorCode());
+}
+
 } // namespace
 
 llvm::Expected<tooling::Replacements>
@@ -145,6 +164,10 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
   ASTContext &ASTCtx = AST.getASTContext();
   SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier(
       AST, Pos, AST.getSourceManager().getMainFileID());
+  // FIXME: renaming macros is not supported yet, the macro-handling code should
+  // be moved to rename tooling library.
+  if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
+    return makeError(UnsupportedSymbol);
   tooling::RefactoringRuleContext Context(AST.getSourceManager());
   Context.setASTContext(ASTCtx);
   auto Rename = clang::tooling::RenameOccurrences::initiate(
@@ -155,24 +178,8 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
   const auto *RenameDecl = Rename->getRenameDecl();
   assert(RenameDecl && "symbol must be found at this point");
   if (auto Reject =
-          renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index)) {
-    auto Message = [](ReasonToReject Reason) {
-      switch (Reason) {
-      case NoIndexProvided:
-        return "symbol may be used in other files (no index available)";
-      case UsedOutsideFile:
-        return "the symbol is used outside main file";
-      case NonIndexable:
-        return "symbol may be used in other files (not eligible for indexing)";
-      case UnsupportedSymbol:
-        return "symbol is not a supported kind (e.g. namespace)";
-      }
-      llvm_unreachable("unhandled reason kind");
-    };
-    return llvm::make_error<llvm::StringError>(
-        llvm::formatv("Cannot rename symbol: {0}", Message(*Reject)),
-        llvm::inconvertibleErrorCode());
-  }
+          renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
+    return makeError(*Reject);
 
   Rename->invoke(ResultCollector, Context);
 

clang-tools-extra/clangd/unittests/RenameTests.cpp

@@ -129,6 +129,13 @@ TEST(RenameTest, Renameable) {
       )cpp",
        "not a supported kind", HeaderFile},
 
+      {
+          R"cpp(
+         #define MACRO 1
+         int s = MAC^RO;
+       )cpp",
+          "not a supported kind", HeaderFile},
+
       {R"cpp(// foo is declared outside the file.
         void fo^o() {}
       )cpp", "used outside main file", !HeaderFile/*cc file*/},

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

@@ -9,6 +9,7 @@
 #include "Context.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "TestTU.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
@@ -28,6 +29,8 @@ MATCHER_P2(Pos, Line, Col, "") {
   return arg.line == int(Line) && arg.character == int(Col);
 }
 
+MATCHER_P(MacroName, Name, "") { return arg.Name == Name; }
+
 /// A helper to make tests easier to read.
 Position position(int line, int character) {
   Position Pos;
@@ -404,6 +407,20 @@ TEST(SourceCodeTests, VisibleNamespaces) {
   }
 }
 
+TEST(SourceCodeTests, GetMacros) {
+  Annotations Code(R"cpp(
+     #define MACRO 123
+     int abc = MA^CRO;
+   )cpp");
+  TestTU TU = TestTU::withCode(Code.code());
+  auto AST = TU.build();
+  auto Loc = getBeginningOfIdentifier(AST, Code.point(),
+                                      AST.getSourceManager().getMainFileID());
+  auto Result = locateMacroAt(Loc, AST.getPreprocessor());
+  ASSERT_TRUE(Result);
+  EXPECT_THAT(*Result, MacroName("MACRO"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang