[clangd] Show better message when we rename macros.

[hokein]

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).

[sammccall]

clang-tools-extra/clangd/SourceCode.cpp

661

"also" no longer makes sense here

clang-tools-extra/clangd/SourceCode.h

204

not sure about "sym" - it's an abbreviation and not very descriptive.
MacroDefinition is really the best name, but taken :-(

Maybe DefinedMacro or ResolvedMacro?

204

(thanks for moving this here, definitely makes sense!)

209

Can we add tests for this now that it's a public API?

210

you can get the SM and LangOpts from the PP

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

167

is the fixme to detect the error, or to support renaming macros?
(And if we're not going to support them, why not?)

[hokein]

Address review comments:

• renaming
• add unittest

[hokein]

clang-tools-extra/clangd/SourceCode.h

204

DefinedMacro sounds good to me.

210

thanks, remove these two parameters.

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

167

not sure whether we'd support renaming macros eventually, but I believe it is not prioritized at least for now. I think the macro-handling logic (detect the error, or support) should live in rename tooling library.

Rephrased the FIXME to make it less confusing.

clang-tools-extra/clangd/SourceCode.h

 ///   using namespace a;
 ///   namespace foo {
 ///     using namespace b;
 ///
 /// visibleNamespaces are {"foo::", "", "a::", "b::", "foo::b::"}, not "a::b::".
 std::vector<std::string> visibleNamespaces(llvm::StringRef Code,
                                            const format::FormatStyle &Style);
 
+struct MacroSym {
+  llvm::StringRef Name;
+  const MacroInfo *Info;
+};
+// Gets the macro at a specified \p Loc.
+llvm::Optional<MacroSym> locateMacroAt(SourceLocation Loc,
+                                       const SourceManager &SM,
+                                       const LangOptions &LangOpts,
+                                       Preprocessor &PP);
+
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/clangd/SourceCode.cpp

 #include "FuzzyMatch.h"
 #include "Logger.h"
 #include "Protocol.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/SourceManager.h"
 #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"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
case Separator:
       break;
     }
   }
   Flush();
 
   return Result;
 }
 
+llvm::Optional<MacroSym> locateMacroAt(SourceLocation Loc,
+                                       const SourceManager &SM,
+                                       const LangOptions &LangOpts,
+                                       Preprocessor &PP) {
+  // Also handle possible macro at the searched location.
+  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 MacroSym{IdentifierInfo->getName(), MI};
+  return None;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/XRefs.cpp

SymbolLocation getPreferredLocation(const Location &ASTLoc,
   Symbol ASTSym, IdxSym;
   ASTSym.ID = IdxSym.ID = SymbolID("dummy_id");
   ASTSym.CanonicalDeclaration = toIndexLocation(ASTLoc, Scratch);
   IdxSym.CanonicalDeclaration = IdxLoc;
   auto Merged = mergeSymbol(ASTSym, IdxSym);
   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<MacroSym> MacroInfos;
   llvm::DenseSet<const Decl *> Decls;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
   Preprocessor &PP;
 
 public:
   DeclarationAndMacrosFinder(const SourceLocation &SearchedLocation,
                              ASTContext &AST, Preprocessor &PP)
       : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
   // The results are sorted by declaration location.
   std::vector<const Decl *> getFoundDecls() const {
     std::vector<const Decl *> Result;
     for (const Decl *D : Decls)
       Result.push_back(D);
 
     llvm::sort(Result, [](const Decl *L, const Decl *R) {
       return L->getBeginLoc() < R->getBeginLoc();
     });
     return Result;
   }
 
-  std::vector<MacroDecl> takeMacroInfos() {
+  std::vector<MacroSym> takeMacroInfos() {
     // Don't keep the same Macro info multiple times.
-    llvm::sort(MacroInfos, [](const MacroDecl &Left, const MacroDecl &Right) {
+    llvm::sort(MacroInfos, [](const MacroSym &Left, const MacroSym &Right) {
       return Left.Info < Right.Info;
     });
 
     auto Last = std::unique(MacroInfos.begin(), MacroInfos.end(),
-                            [](const MacroDecl &Left, const MacroDecl &Right) {
+                            [](const MacroSym &Left, const MacroSym &Right) {
                               return Left.Info == Right.Info;
                             });
     MacroInfos.erase(Last, MacroInfos.end());
     return std::move(MacroInfos);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
if (Loc == SearchedLocation) {
         Decls.insert(D);
       }
     }
     return true;
   }
 
 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 MacroSym = locateMacroAt(SearchedLocation, AST.getSourceManager(),
+                                      AST.getLangOpts(), PP)) {
+      MacroInfos.push_back(*MacroSym);
 };
 
 struct IdentifiedSymbol {
   std::vector<const Decl *> Decls;
-  std::vector<MacroDecl> Macros;
+  std::vector<MacroSym> Macros;
 };
 
 IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
   auto DeclMacrosFinder = DeclarationAndMacrosFinder(Pos, AST.getASTContext(),
                                                      AST.getPreprocessor());
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
static HoverInfo getHoverContents(QualType T, const Decl *D,
   T.print(OS, Policy);
 
   if (D)
     HI.Kind = indexSymbolKindToSymbolKind(index::getSymbolInfo(D).Kind);
   return HI;
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-static HoverInfo getHoverContents(MacroDecl Decl, ParsedAST &AST) {
+static HoverInfo getHoverContents(MacroSym Decl, ParsedAST &AST) {
   HoverInfo HI;
   SourceManager &SM = AST.getSourceManager();
   HI.Name = Decl.Name;
   HI.Kind = indexSymbolKindToSymbolKind(
       index::getSymbolInfoForMacro(*Decl.Info).Kind);
   // FIXME: Populate documentation
   // FIXME: Pupulate parameters
 

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

llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
   // we rename it.
   if (!OtherFile)
     return None;
   // If the symbol is indexable and has refs from other files in the index,
   // we disallow rename.
   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>
 renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
                  llvm::StringRef NewName, const SymbolIndex *Index) {
   RefactoringResultCollector ResultCollector;
   ASTContext &ASTCtx = AST.getASTContext();
   SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier(
       AST, Pos, AST.getSourceManager().getMainFileID());
+  // FIXME: this should be moved to rename tooling library?
+  if (locateMacroAt(SourceLocationBeg, ASTCtx.getSourceManager(),
+                    ASTCtx.getLangOpts(), AST.getPreprocessor()))
+    return makeError(UnsupportedSymbol);
   tooling::RefactoringRuleContext Context(AST.getSourceManager());
   Context.setASTContext(ASTCtx);
   auto Rename = clang::tooling::RenameOccurrences::initiate(
       Context, SourceRange(SourceLocationBeg), NewName);
   if (!Rename)
     return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics());
 
   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);
 
   assert(ResultCollector.Result.hasValue());
   if (!ResultCollector.Result.getValue())
     return expandDiagnostics(ResultCollector.Result->takeError(),
                              ASTCtx.getDiagnostics());
 

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

Case Cases[] = {
       )cpp",
        "not eligible for indexing", HeaderFile},
 
       {R"cpp(// disallow -- namespace symbol isn't supported
         namespace fo^o {}
       )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*/},
   };
   const char *CommonHeader = R"cpp(
     class Outside {};
     void foo();
   )cpp";