[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 ↗

(On Diff #207017)

"also" no longer makes sense here

clang-tools-extra/clangd/SourceCode.h

204 ↗

(On Diff #207017)

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 ↗

(On Diff #207017)

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

209 ↗

(On Diff #207017)

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

210 ↗

(On Diff #207017)

you can get the SM and LangOpts from the PP

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

167 ↗

(On Diff #207017)

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 ↗

(On Diff #207017)

DefinedMacro sounds good to me.

210 ↗

(On Diff #207017)

thanks, remove these two parameters.

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

167 ↗

(On Diff #207017)

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/trunk/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 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/trunk/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<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/trunk/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<DefinedMacro> 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<DefinedMacro> takeMacroInfos() {
     // Don't keep the same Macro info multiple times.
-    llvm::sort(MacroInfos, [](const MacroDecl &Left, const MacroDecl &Right) {
+    llvm::sort(MacroInfos,
+               [](const DefinedMacro &Left, const DefinedMacro &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;
-                            });
+    auto Last =
+        std::unique(MacroInfos.begin(), MacroInfos.end(),
+                    [](const DefinedMacro &Left, const DefinedMacro &Right) {
     MacroInfos.erase(Last, MacroInfos.end());
     return std::move(MacroInfos);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       llvm::ArrayRef<index::SymbolRelation> Relations,
                       SourceLocation Loc,
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 DefinedMacro = locateMacroAt(SearchedLocation, PP)) {
+      MacroInfos.push_back(*DefinedMacro);
 };
 
 struct IdentifiedSymbol {
   std::vector<const Decl *> Decls;
-  std::vector<MacroDecl> Macros;
+  std::vector<DefinedMacro> 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(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());
     bool Invalid;
     StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
     if (!Invalid) {
       unsigned StartOffset = SM.getFileOffset(StartLoc);
       unsigned EndOffset = SM.getFileOffset(EndLoc);

clang-tools-extra/trunk/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: 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(
       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/trunk/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";

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

 //===-- SourceCodeTests.cpp  ------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "Annotations.h"
 #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"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using llvm::Failed;
 using llvm::HasValue;
 using ::testing::UnorderedElementsAreArray;
 
 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;
   Pos.line = line;
   Pos.character = character;
   return Pos;
 }
 
TEST(SourceCodeTests, VisibleNamespaces) {
   };
   for (const auto& Case : Cases) {
     EXPECT_EQ(Case.second,
               visibleNamespaces(Case.first, format::getLLVMStyle()))
         << Case.first;
   }
 }
 
+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