[clangd] Add "usedAsMutablePointer" highlighting modifier

[ckandeler]

Summary

Counterpart to "usedAsMutableReference". Just as for references, there
are const and non-const pointer parameters, and it's valuable to be able
to have different highlighting for the two cases at the call site.
We could have re-used the existing modifier, but having a dedicated one
maximizes client flexibility.

[ckandeler]

ping?

[nridge]

Sorry, I'm travelling and probably won't be able to respond promptly, but my first thought here is that this doesn't seem as well motivated as usedAsMutableReference, since taking a pointer to an object involves using explicit syntax (&obj) that taking a reference does not. My mental model of usedAsMutableReference is that it's a workaround for the fact that C++ does not have explicit syntax for forming a mutable reference (compare Rust's ref obj which in my mind makes this modifier likewise unnecessary there).

[ckandeler]

IMO the relevant point is the (non-)const-ness of the corresponding parameter, i.e. can the passed object get mutated or not. The fact that there's an ampersand at the call site (which is not even the case if the variable is itself a pointer) does not tell me that.

[kadircet]

I agree with Nathan to some extent that this doesn't carry as much weight, but if we can get it without introducing much complexity to the user (i.e. a new modifier they need to remember), it's worth having since we don't need too much effort to support it.

clang-tools-extra/clangd/SemanticHighlighting.cpp

578

nit: we don't do consts for locals (not saying this is a bad practice, it's just not inline with the convention in the rest of the codebase).

603

why do you think we should have different modifiers for these? they both indicate the same thing, the parameter might get mutated by the call, and it being a pointer or reference doesn't seem to matter.

[ckandeler]

Fixed style issues.

clang-tools-extra/clangd/SemanticHighlighting.cpp

603

Because I'm aware that there are people (like Nathan) who don't find it useful for pointers, and having just one modifier would force it on everybody. With two distinct modifiers, clients can decide for themselves, possibly even exposing the choice to users.

[nridge]

In D130015#3702004, @ckandeler wrote:

IMO the relevant point is the (non-)const-ness of the corresponding parameter, i.e. can the passed object get mutated or not. The fact that there's an ampersand at the call site (which is not even the case if the variable is itself a pointer) does not tell me that.

Taking another look at this, I find this use case convincing and I'm fine with taking this patch.

I do prefer the separate modifier for customizability, as I think the desirability of highlighting this can vary based on the coding convention. For example, one project I work with is slightly older and a lot of objects are passed around by non-const pointer, but actual "out parameters" almost always take the form of &var at the call site or use a reference; in such a case, the pointer modifier would be fairly noisy and it's nice to be able to disable it while keeping the reference modifier on.

clang-tools-extra/clangd/SemanticHighlighting.cpp

587

nit: you can remove the "and UnaryOperator" part of the FIXME, since you're addressing it

592

Could you add a test case that exercises the UnaryOperator case?

[ckandeler]

clang-tools-extra/clangd/SemanticHighlighting.cpp

592

Doesn't the very first hunk in SemanticHighlightingTests.cpp do that?

[nridge]

clang-tools-extra/clangd/SemanticHighlighting.cpp

592

Yep, my bad, overlooked that

[ckandeler]

Rebased and updated comment as indicated in review.

clang-tools-extra/clangd/SemanticHighlighting.h

enum class HighlightingModifier {
   Deduced,
   Readonly,
   Static,
   Abstract,
   Virtual,
   DependentName,
   DefaultLibrary,
   UsedAsMutableReference,
+  UsedAsMutablePointer,
 
   FunctionScope,
   ClassScope,
   FileScope,
   GlobalScope,
 
   LastModifier = GlobalScope
 };

clang-tools-extra/clangd/SemanticHighlighting.cpp

bool VisitCallExpr(CallExpr *E) {
 
     return true;
   }
 
   void highlightMutableReferenceArgument(QualType T, const Expr *Arg) {
     if (!Arg)
       return;
 
-    // Is this parameter passed by non-const reference?
+    // Is this parameter passed by non-const pointer or 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()) {
+    const bool IsRef = T->isLValueReferenceType();
+    const bool IsPtr = T->isPointerType();
+    if ((!IsRef && !IsPtr) || T->getPointeeType().isConstQualified() ||
+        T->isDependentType()) {
       return;
     }
 
     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 *IC = dyn_cast<ImplicitCastExpr>(Arg))
+      Arg = IC->getSubExprAsWritten();
+    if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
+      if (UO->getOpcode() == UO_AddrOf)
+        Arg = UO->getSubExpr();
+    }
     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);
+                         IsRef ? HighlightingModifier::UsedAsMutableReference
+                               : HighlightingModifier::UsedAsMutablePointer);
   }
 
   void
   highlightMutableReferenceArguments(const FunctionDecl *FD,
                                      llvm::ArrayRef<const Expr *const> Args) {
     if (!FD)
       return;
 
llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier) {
   case HighlightingModifier::Virtual:
     return "virtual";
   case HighlightingModifier::DependentName:
     return "dependentName"; // nonstandard
   case HighlightingModifier::DefaultLibrary:
     return "defaultLibrary";
   case HighlightingModifier::UsedAsMutableReference:
     return "usedAsMutableReference"; // nonstandard
+  case HighlightingModifier::UsedAsMutablePointer:
+    return "usedAsMutablePointer"; // nonstandard
   case HighlightingModifier::FunctionScope:
     return "functionScope"; // nonstandard
   case HighlightingModifier::ClassScope:
     return "classScope"; // nonstandard
   case HighlightingModifier::FileScope:
     return "fileScope"; // nonstandard
   case HighlightingModifier::GlobalScope:
     return "globalScope"; // nonstandard

clang-tools-extra/clangd/test/initialize-params.test

 # CHECK-NEXT:            "deduced",
 # CHECK-NEXT:            "readonly",
 # CHECK-NEXT:            "static",
 # CHECK-NEXT:            "abstract",
 # CHECK-NEXT:            "virtual",
 # CHECK-NEXT:            "dependentName",
 # CHECK-NEXT:            "defaultLibrary",
 # CHECK-NEXT:            "usedAsMutableReference",
+# CHECK-NEXT:            "usedAsMutablePointer",
 # CHECK-NEXT:            "functionScope",
 # CHECK-NEXT:            "classScope",
 # CHECK-NEXT:            "fileScope",
 # CHECK-NEXT:            "globalScope"
 # CHECK-NEXT:          ],
 # CHECK-NEXT:          "tokenTypes": [
 # CHECK-NEXT:            "variable",
 # CHECK:               ]

clang-tools-extra/clangd/test/semantic-tokens.test

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "data": [
 #                  First line, char 5, variable, declaration+globalScope
 # CHECK-NEXT:      0,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      8193
+# CHECK-NEXT:      16385
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "1"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{
   "textDocument": {"uri":"test:///foo.cpp","version":2},
   "contentChanges":[{"text":"int x = 2;\nint y = 3;"}]
 }}

 # CHECK-NEXT:    "edits": [
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "data": [
 #                      Next line, char 5, variable, declaration+globalScope
 # CHECK-NEXT:          1,
 # CHECK-NEXT:          4,
 # CHECK-NEXT:          1,
 # CHECK-NEXT:          0,
-# CHECK-NEXT:          8193
+# CHECK-NEXT:          16385
 # CHECK-NEXT:        ],
 #                    Inserted at position 1
 # CHECK-NEXT:        "deleteCount": 0,
 # CHECK-NEXT:        "start": 5
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "2"
 # CHECK-NEXT:  }
 ---
 # Incremental token request with incorrect baseline => full tokens list.
 {"jsonrpc":"2.0","id":2,"method":"textDocument/semanticTokens/full/delta","params":{
   "textDocument": {"uri":"test:///foo.cpp"},
   "previousResultId": "bogus"
 }}
 # CHECK:       "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "data": [
 # CHECK-NEXT:      0,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      8193,
+# CHECK-NEXT:      16385,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      8193
+# CHECK-NEXT:      16385
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "3"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

)cpp",
         void $Method_decl[[f]]() {
           (*$TemplateParameter_readonly[[Func]])();
         }
       };
 
       void $Function_decl[[foo]]() {
         $Class[[F]] $LocalVariable_decl[[FF]];
         $Class[[G]]<$Class[[F]], &$Class[[F]]::$Method[[f]]> $LocalVariable_decl[[GG]];
-        $LocalVariable[[GG]].$Method[[foo]](&$LocalVariable[[FF]]);
+        $LocalVariable[[GG]].$Method[[foo]](&$LocalVariable_usedAsMutablePointer[[FF]]);
         $Class[[A]]<$Function[[foo]]> $LocalVariable_decl[[AA]];
       }
     )cpp",
       // Tokens that share a source range but have conflicting Kinds are not
       // highlighted.
       R"cpp(
       #define $Macro_decl[[DEF_MULTIPLE]](X) namespace X { class X { int X; }; }
       #define $Macro_decl[[DEF_CLASS]](T) class T {};
)cpp",
                                    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_usedAsMutablePointer[[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_usedAsMutablePointer[[array]], $LocalVariable_usedAsMutableReference[[array]],
                            $LocalVariable[[array]]
                            );
           [](int){}($LocalVariable[[val]]);
           [](int&){}($LocalVariable_usedAsMutableReference[[val]]);
           [](const int&){}($LocalVariable[[val]]);
           $Class[[ClassWithOp]] $LocalVariable_decl[[c]];
           const $Class[[ClassWithOp]] $LocalVariable_decl_readonly[[c2]];
           $LocalVariable[[c]]($LocalVariable[[val]]);