[clangd] Improve inlay hints of things expanded from macros

[sammccall]

Summary

When we aim a hint at some expanded tokens, we're only willing to attach it
to spelled tokens that exactly corresponde.

e.g.
int zoom(int x, int y, int z);
int dummy = zoom(NUMBERS);

Here we want to place a hint "x:" on the expanded "1", but we shouldn't
be willing to place it on NUMBERS, because it doesn't *exactly*
correspond (it has more tokens).

Fortunately we don't even have to implement this algorithm from scratch,
TokenBuffer has it.

Fixes https://github.com/clangd/clangd/issues/1289
Fixes https://github.com/clangd/clangd/issues/1118
Fixes https://github.com/clangd/clangd/issues/1018

[nridge]

Thanks for fixing!

clang-tools-extra/clangd/InlayHints.cpp

281

Is this change related to the fix?

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

281

Only tangentially...

This change makes the whole range significant (previously it was serialized but I think not used by any clients these days). So hiding the token->range conversion inside this function might encourage bugs in the future, even if today all the callers do that.

(I did consider attaching the return type hints to e.g. the parens range rather than a single paren, but can't think of a motivating case in either direction really)

clang-tools-extra/clangd/InlayHints.cpp

 //===--- InlayHints.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 "InlayHints.h"
 #include "AST.h"
 #include "Config.h"
 #include "HeuristicResolver.h"
 #include "ParsedAST.h"
+#include "SourceCode.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/ScopeExit.h"
 
collectDesignators(Syn->isSemanticForm() ? Syn : Syn->getSemanticForm(),
                      Designators, NestedBraces, EmptyPrefix);
   return Designators;
 }
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
                    const Config &Cfg, llvm::Optional<Range> RestrictRange)
-      : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
-        RestrictRange(std::move(RestrictRange)),
+      : Results(Results), AST(AST.getASTContext()), Tokens(AST.getTokens()),
+        Cfg(Cfg), RestrictRange(std::move(RestrictRange)),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
         TypeHintPolicy(this->AST.getPrintingPolicy()),
         StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
     MainFileBuf = Invalid ? StringRef{} : Buf;
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // an argument list, by checking the validity of getParenOrBraceRange().
     // Also weed out std::initializer_list constructors as there are no names
     // for the individual arguments.
     if (!E->getParenOrBraceRange().isValid() ||
         E->isStdInitListInitialization()) {
       return true;
     }
 
-    processCall(E->getParenOrBraceRange().getBegin(), E->getConstructor(),
-                {E->getArgs(), E->getNumArgs()});
+    processCall(E->getConstructor(), {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitCallExpr(CallExpr *E) {
     if (!Cfg.InlayHints.Parameters)
       return true;
 
     // Do not show parameter hints for operator calls written using operator
bool VisitCallExpr(CallExpr *E) {
     const FunctionDecl *Callee = nullptr;
     if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecls[0]))
       Callee = FD;
     else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CalleeDecls[0]))
       Callee = FTD->getTemplatedDecl();
     if (!Callee)
       return true;
 
-    processCall(E->getRParenLoc(), Callee, {E->getArgs(), E->getNumArgs()});
+    processCall(Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *FPT =
             llvm::dyn_cast<FunctionProtoType>(D->getType().getTypePtr())) {
       if (!FPT->hasTrailingReturn()) {
         if (auto FTL = D->getFunctionTypeLoc())
           addReturnTypeHint(D, FTL.getRParenLoc());
       }
     }
     return true;
   }
 
   bool VisitLambdaExpr(LambdaExpr *E) {
     FunctionDecl *D = E->getCallOperator();
     if (!E->hasExplicitResultType())
       addReturnTypeHint(D, E->hasExplicitParameters()
                                ? D->getFunctionTypeLoc().getRParenLoc()
                                : E->getIntroducerRange().getEnd());
     return true;
   }
 
-  void addReturnTypeHint(FunctionDecl *D, SourceLocation Loc) {
+  void addReturnTypeHint(FunctionDecl *D, SourceRange Range) {
     auto *AT = D->getReturnType()->getContainedAutoType();
     if (!AT || AT->getDeducedType().isNull())
       return;
-    addTypeHint(Loc, D->getReturnType(), /*Prefix=*/"-> ");
+    addTypeHint(Range, D->getReturnType(), /*Prefix=*/"-> ");
   }
 
   bool VisitVarDecl(VarDecl *D) {
     // Do not show hints for the aggregate in a structured binding,
     // but show hints for the individual bindings.
     if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
       for (auto *Binding : DD->bindings()) {
         addTypeHint(Binding->getLocation(), Binding->getType(), /*Prefix=*/": ",
bool VisitInitListExpr(InitListExpr *Syn) {
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
-  // The purpose of Anchor is to deal with macros. It should be the call's
-  // opening or closing parenthesis or brace. (Always using the opening would
-  // make more sense but CallExpr only exposes the closing.) We heuristically
-  // assume that if this location does not come from a macro definition, then
-  // the entire argument list likely appears in the main file and can be hinted.
-  void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
+  void processCall(const FunctionDecl *Callee,
                    llvm::ArrayRef<const Expr *> Args) {
     if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
       return;
 
-    // If the anchor location comes from a macro definition, there's nowhere to
-    // put hints.
-    if (!AST.getSourceManager().getTopMacroCallerLoc(Anchor).isFileID())
-      return;
-
     // The parameter name of a move or copy constructor is not very interesting.
     if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
       if (Ctor->isCopyOrMoveConstructor())
         return;
 
     // Resolve parameter packs to their forwarded parameter
     auto ForwardedParams = resolveForwardingParameters(Callee);
 
if (!Cfg.InlayHints.ConfigProperty)                                        \
       return;                                                                  \
     break
       CHECK_KIND(Parameter, Parameters);
       CHECK_KIND(Type, DeducedTypes);
       CHECK_KIND(Designator, Designators);
 #undef CHECK_KIND
     }
 
-    auto FileRange =
-        toHalfOpenFileRange(AST.getSourceManager(), AST.getLangOpts(), R);
-    if (!FileRange)
+    auto LSPRange = getHintRange(R);
+    if (!LSPRange)
       return;
-    Range LSPRange{
-        sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
-        sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())};
-    Position LSPPos = Side == HintSide::Left ? LSPRange.start : LSPRange.end;
+    Position LSPPos = Side == HintSide::Left ? LSPRange->start : LSPRange->end;
     if (RestrictRange &&
         (LSPPos < RestrictRange->start || !(LSPPos < RestrictRange->end)))
       return;
-    // The hint may be in a file other than the main file (for example, a header
-    // file that was included after the preamble), do not show in that case.
-    if (!AST.getSourceManager().isWrittenInMainFile(FileRange->getBegin()))
-      return;
     bool PadLeft = Prefix.consume_front(" ");
     bool PadRight = Suffix.consume_back(" ");
     Results.push_back(InlayHint{LSPPos, (Prefix + Label + Suffix).str(), Kind,
-                                PadLeft, PadRight, LSPRange});
+                                PadLeft, PadRight, *LSPRange});
+  }
+
+  // Get the range of the main file that *exactly* corresponds to R.
+  llvm::Optional<Range> getHintRange(SourceRange R) {
+    const auto &SM = AST.getSourceManager();
+    auto Spelled = Tokens.spelledForExpanded(Tokens.expandedTokens(R));
+    // TokenBuffer will return null if e.g. R corresponds to only part of a
+    // macro expansion.
+    if (!Spelled || Spelled->empty())
+      return llvm::None;
+    // Hint must be within the main file, not e.g. a non-preamble include.
+    if (SM.getFileID(Spelled->front().location()) != SM.getMainFileID() ||
+        SM.getFileID(Spelled->back().location()) != SM.getMainFileID())
+      return llvm::None;
+    return Range{sourceLocToPosition(SM, Spelled->front().location()),
+                 sourceLocToPosition(SM, Spelled->back().endLocation())};
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
     addTypeHint(R, T, Prefix, TypeHintPolicy);
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
                    const PrintingPolicy &Policy) {
     if (!Cfg.InlayHints.DeducedTypes || T.isNull())
       return;
 
     std::string TypeName = T.getAsString(Policy);
     if (TypeName.length() < TypeNameLimit)
       addInlayHint(R, HintSide::Right, InlayHintKind::Type, Prefix, TypeName,
                    /*Suffix=*/"");
   }
 
   void addDesignatorHint(SourceRange R, llvm::StringRef Text) {
     addInlayHint(R, HintSide::Left, InlayHintKind::Designator,
                  /*Prefix=*/"", Text, /*Suffix=*/"=");
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
+  const syntax::TokenBuffer &Tokens;
   const Config &Cfg;
   llvm::Optional<Range> RestrictRange;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
   // We want to suppress default template arguments, but otherwise print
   // canonical types. Unfortunately, they're conflicting policies so we can't
   // have both. For regular types, suppressing template arguments is more

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

assertParameterHints(R"cpp(
     void abort();
     #define ASSERT(expr) if (!expr) abort()
     int foo(int param);
     void bar() {
       ASSERT(foo($param[[42]]) == 0);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
+
+  // If the macro expands to multiple arguments, don't hint it.
+  assertParameterHints(R"cpp(
+    void foo(double x, double y);
+    #define CONSTANTS 3.14, 2.72
+    void bar() {
+      foo(CONSTANTS);
+    }
+  )cpp");
 }
 
 TEST(ParameterHints, ConstructorParens) {
   assertParameterHints(R"cpp(
     struct S {
       S(int param);
     };
     void bar() {