Add semantic token modifier for non-const reference parameter

See https://github.com/clangd/clangd/issues/839

Reviewed By: sammccall

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

diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index 67315f5..d55bd9e 100644 (file)
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -339,15 +339,11 @@ public:
         LangOpts(AST.getLangOpts()) {}
 
   HighlightingToken &addToken(SourceLocation Loc, HighlightingKind Kind) {
-    Loc = getHighlightableSpellingToken(Loc, SourceMgr);
-    if (Loc.isInvalid())
+    auto Range = getRangeForSourceLocation(Loc);
+    if (!Range)
       return InvalidHighlightingToken;
-    const auto *Tok = TB.spelledTokenAt(Loc);
-    assert(Tok);
-    return addToken(
-        halfOpenToRange(SourceMgr,
-                        Tok->range(SourceMgr).toCharRange(SourceMgr)),
-        Kind);
+
+    return addToken(*Range, Kind);
   }
 
   HighlightingToken &addToken(Range R, HighlightingKind Kind) {
@@ -358,6 +354,11 @@ public:
     return Tokens.back();
   }
 
+  void addExtraModifier(SourceLocation Loc, HighlightingModifier Modifier) {
+    if (auto Range = getRangeForSourceLocation(Loc))
+      ExtraModifiers[*Range].push_back(Modifier);
+  }
+
   std::vector<HighlightingToken> collect(ParsedAST &AST) && {
     // Initializer lists can give duplicates of tokens, therefore all tokens
     // must be deduplicated.
@@ -377,12 +378,22 @@ public:
             // this predicate would never fire.
             return T.R == TokRef.front().R;
           });
-      if (auto Resolved = resolveConflict(Conflicting))
+      if (auto Resolved = resolveConflict(Conflicting)) {
+        // Apply extra collected highlighting modifiers
+        auto Modifiers = ExtraModifiers.find(Resolved->R);
+        if (Modifiers != ExtraModifiers.end()) {
+          for (HighlightingModifier Mod : Modifiers->second) {
+            Resolved->addModifier(Mod);
+          }
+        }
+
         NonConflicting.push_back(*Resolved);
+      }
       // TokRef[Conflicting.size()] is the next token with a different range (or
       // the end of the Tokens).
       TokRef = TokRef.drop_front(Conflicting.size());
     }
+
     const auto &SM = AST.getSourceManager();
     StringRef MainCode = SM.getBufferOrFake(SM.getMainFileID()).getBuffer();
 
@@ -440,10 +451,23 @@ public:
   const HeuristicResolver *getResolver() const { return Resolver; }
 
 private:
+  llvm::Optional<Range> getRangeForSourceLocation(SourceLocation Loc) {
+    Loc = getHighlightableSpellingToken(Loc, SourceMgr);
+    if (Loc.isInvalid())
+      return llvm::None;
+
+    const auto *Tok = TB.spelledTokenAt(Loc);
+    assert(Tok);
+
+    return halfOpenToRange(SourceMgr,
+                           Tok->range(SourceMgr).toCharRange(SourceMgr));
+  }
+
   const syntax::TokenBuffer &TB;
   const SourceManager &SourceMgr;
   const LangOptions &LangOpts;
   std::vector<HighlightingToken> Tokens;
+  std::map<Range, llvm::SmallVector<HighlightingModifier, 1>> ExtraModifiers;
   const HeuristicResolver *Resolver;
   // returned from addToken(InvalidLoc)
   HighlightingToken InvalidHighlightingToken;
@@ -496,6 +520,71 @@ class CollectExtraHighlightings
 public:
   CollectExtraHighlightings(HighlightingsBuilder &H) : H(H) {}
 
+  bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+    highlightMutableReferenceArguments(E->getConstructor(),
+                                       {E->getArgs(), E->getNumArgs()});
+
+    return true;
+  }
+
+  bool VisitCallExpr(CallExpr *E) {
+    // Highlighting parameters passed by non-const reference does not really
+    // make sense for literals...
+    if (isa<UserDefinedLiteral>(E))
+      return true;
+
+    // FIXME ...here it would make sense though.
+    if (isa<CXXOperatorCallExpr>(E))
+      return true;
+
+    highlightMutableReferenceArguments(
+        dyn_cast_or_null<FunctionDecl>(E->getCalleeDecl()),
+        {E->getArgs(), E->getNumArgs()});
+
+    return true;
+  }
+
+  void
+  highlightMutableReferenceArguments(const FunctionDecl *FD,
+                                     llvm::ArrayRef<const Expr *const> Args) {
+    if (!FD)
+      return;
+
+    if (auto *ProtoType = FD->getType()->getAs<FunctionProtoType>()) {
+      // Iterate over the types of the function parameters.
+      // If any of them are non-const reference paramteres, add it as a
+      // highlighting modifier to the corresponding expression
+      for (size_t I = 0;
+           I < std::min(size_t(ProtoType->getNumParams()), Args.size()); ++I) {
+        auto T = ProtoType->getParamType(I);
+
+        // Is this parameter passed by non-const reference?
+        // FIXME The condition !T->idDependentType() could be relaxed a bit,
+        // e.g. std::vector<T>& is dependent but we would want to highlight it
+        if (T->isLValueReferenceType() &&
+            !T.getNonReferenceType().isConstQualified() &&
+            !T->isDependentType()) {
+          if (auto *Arg = Args[I]) {
+            llvm::Optional<SourceLocation> Location;
+
+            // FIXME Add "unwrapping" for ArraySubscriptExpr and UnaryOperator,
+            //  e.g. highlight `a` in `a[i]`
+            // FIXME Handle dependent expression types
+            if (auto *DR = dyn_cast<DeclRefExpr>(Arg)) {
+              Location = DR->getLocation();
+            } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
+              Location = M->getMemberLoc();
+            }
+
+            if (Location)
+              H.addExtraModifier(*Location,
+                                 HighlightingModifier::UsedAsMutableReference);
+          }
+        }
+      }
+    }
+  }
+
   bool VisitDecltypeTypeLoc(DecltypeTypeLoc L) {
     if (auto K = kindForType(L.getTypePtr(), H.getResolver())) {
       auto &Tok = H.addToken(L.getBeginLoc(), *K)
@@ -912,6 +1001,8 @@ llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier) {
     return "dependentName"; // nonstandard
   case HighlightingModifier::DefaultLibrary:
     return "defaultLibrary";
+  case HighlightingModifier::UsedAsMutableReference:
+    return "usedAsMutableReference"; // nonstandard
   case HighlightingModifier::FunctionScope:
     return "functionScope"; // nonstandard
   case HighlightingModifier::ClassScope:

diff --git a/clang-tools-extra/clangd/SemanticHighlighting.h b/clang-tools-extra/clangd/SemanticHighlighting.h
index 10bcb47..b44aa50 100644 (file)
--- a/clang-tools-extra/clangd/SemanticHighlighting.h
+++ b/clang-tools-extra/clangd/SemanticHighlighting.h
@@ -69,6 +69,7 @@ enum class HighlightingModifier {
   Virtual,
   DependentName,
   DefaultLibrary,
+  UsedAsMutableReference,
 
   FunctionScope,
   ClassScope,

diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test
index 891ef2c..d356c2e 100644 (file)
--- a/clang-tools-extra/clangd/test/initialize-params.test
+++ b/clang-tools-extra/clangd/test/initialize-params.test
@@ -91,6 +91,7 @@
 # CHECK-NEXT:            "virtual",
 # CHECK-NEXT:            "dependentName",
 # CHECK-NEXT:            "defaultLibrary",
+# CHECK-NEXT:            "usedAsMutableReference",
 # CHECK-NEXT:            "functionScope",
 # CHECK-NEXT:            "classScope",
 # CHECK-NEXT:            "fileScope",

diff --git a/clang-tools-extra/clangd/test/semantic-tokens.test b/clang-tools-extra/clangd/test/semantic-tokens.test
index 6d3e324..a29993b 100644 (file)
--- a/clang-tools-extra/clangd/test/semantic-tokens.test
+++ b/clang-tools-extra/clangd/test/semantic-tokens.test
@@ -23,7 +23,7 @@
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      4097
+# CHECK-NEXT:      8193
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "1"
 # CHECK-NEXT:  }
@@ -49,7 +49,7 @@
 # CHECK-NEXT:          4,
 # CHECK-NEXT:          1,
 # CHECK-NEXT:          0,
-# CHECK-NEXT:          4097
+# CHECK-NEXT:          8193
 # CHECK-NEXT:        ],
 #                    Inserted at position 1
 # CHECK-NEXT:        "deleteCount": 0,
@@ -72,12 +72,12 @@
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      4097,
+# CHECK-NEXT:      8193,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      4097
+# CHECK-NEXT:      8193
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "3"
 # CHECK-NEXT:  }

diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index 403f506..646c6ec 100644 (file)
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -729,6 +729,47 @@ sizeof...($TemplateParameter[[Elements]]);
           }
         };
       )cpp",
+      // Modifier for variables passed as non-const references
+      R"cpp(
+        void $Function_decl[[fun]](int, const int,
+                                   int*, const int*,
+                                   int&, const int&,
+                                   int*&, const int*&, const int* const &,
+                                   int**, int**&, int** const &,
+                                   int = 123) {
+          int $LocalVariable_decl[[val]];
+          int* $LocalVariable_decl[[ptr]];
+          const int* $LocalVariable_decl_readonly[[constPtr]];
+          int** $LocalVariable_decl[[array]];
+          $Function[[fun]]($LocalVariable[[val]], $LocalVariable[[val]], 
+                           $LocalVariable[[ptr]], $LocalVariable_readonly[[constPtr]], 
+                           $LocalVariable_usedAsMutableReference[[val]], $LocalVariable[[val]], 
+
+                           $LocalVariable_usedAsMutableReference[[ptr]],
+                           $LocalVariable_readonly_usedAsMutableReference[[constPtr]],
+                           $LocalVariable_readonly[[constPtr]],
+
+                           $LocalVariable[[array]], $LocalVariable_usedAsMutableReference[[array]], 
+                           $LocalVariable[[array]]
+                           );
+        }
+        struct $Class_decl[[S]] {
+          $Class_decl[[S]](int&) {
+            $Class[[S]] $LocalVariable_decl[[s1]]($Field_usedAsMutableReference[[field]]);
+            $Class[[S]] $LocalVariable_decl[[s2]]($LocalVariable[[s1]].$Field_usedAsMutableReference[[field]]);
+
+            $Class[[S]] $LocalVariable_decl[[s3]]($StaticField_static_usedAsMutableReference[[staticField]]);
+            $Class[[S]] $LocalVariable_decl[[s4]]($Class[[S]]::$StaticField_static_usedAsMutableReference[[staticField]]);
+          }
+          int $Field_decl[[field]];
+          static int $StaticField_decl_static[[staticField]];
+        };
+        template <typename $TemplateParameter_decl[[X]]>
+        void $Function_decl[[foo]]($TemplateParameter[[X]]& $Parameter_decl[[x]]) {
+          // We do not support dependent types, so this one should *not* get the modifier.
+          $Function[[foo]]($Parameter[[x]]); 
+        }
+      )cpp",
   };
   for (const auto &TestCase : TestCases)
     // Mask off scope modifiers to keep the tests manageable.