Add semantic token modifier for non-const reference parameter

[tom-anders]

Summary

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

[nridge]

Thanks -- this patch is looking great so far!

clang-tools-extra/clangd/SemanticHighlighting.cpp

312

We should consider the case where a dependent name is passed by non-const reference, for example:

void increment_counter(int&);

template <typename T>
void bar() {
   increment_counter(T::static_counter);
}

This case does not work yet with the current patch (the dependent name is a DependentScopeDeclRefExpr rather than a DeclRefExpr), but we'll want to make it work in the future.

With the conflict resolution logic in this patch, the Unknown token kind from highlightPassedByNonConstReference() will be chosen over the dependent token kind.

As it happens, the dependent token kind for expressions is also Unknown so it doesn't matter, but perhaps we shouldn't be relying on this. Perhaps the following would make more sense:

1. a token with Unknown as the kind has the lowest priority
2. then a token with the DependentName modifier (next lowest)
3. then everything else?

535

nit: sense

550

We can avoid the callback by constructing an ArrayRef of the arguments, see a similar case here:

https://searchfox.org/llvm/source/clang-tools-extra/clangd/InlayHints.cpp#56
https://searchfox.org/llvm/source/clang-tools-extra/clangd/InlayHints.cpp#79

562

I think there are some edge cases where isConst() doesn't do what we want.

For example, I think for a parameter of type const int*&, it will return true (and thus we will not highlight the corresponding argument), even thus this is actually a non-const reference.

So, we may want to use a dedicated function that specifically handles reference-related logic only.

(This probably also makes a good test case.)

567

For a qualified name (e.g. A::B), I think this is going to return the beginning of the qualifier, whereas we only want to highlight the last name (otherwise there won't be a matching token from the first pass).

So I think we want getLocation() instead.

(Also makes a good test case.)

[sammccall]

Agree this is nice, well done! A few more notes for consideration...

clang-tools-extra/clangd/SemanticHighlighting.cpp

312

The conflict-resolution idea is subtle (and IME hard to debug). I'm wary of overloading it by deliberately introducing "conflicts" that should actually be merged. Did you consider the idea of tracking extra modifiers separately and merging them in at the end?

---

BTW: we're stretching the meaning of Unknown here. There are two subtly different concepts:

• clangd happens not to have determined the kind of this token, e.g. because we missed a case (uses in this patch)
• clangd has determined that per C++ rules the kind of token is ambiguous (uses prior to this patch)

Call me weird, but I have "Unknown" highlighted in bright orange in my editor, because I want to know about the second case :-)

534

CXXOperatorCallExpr seems to make sense to me for the most part:

• functor calls are CXXOperatorCallExpr
• if x + y mutates y, I want to know that

There are some exceptions though:

• the functor object itself is more like the function callee, and to be consistent we shouldn't highlight it
• highlighting x in x += y is weird
• a << b is a weird ambiguous case ("stream" vs "shift" want different behavior)

I think these can be handled by choosing a minimum argument index to highlight based on the operator type.

I think it makes sense to leave operators out of scope for this patch, but IMO should be a "FIXME" rather than a "let's never do this" :-)

548

nit: this function name is a bit hard to parse, "highlight" is a transitive verb but the rest of the name isn't its object.

Maybe highlightMutableReferenceArguments?

558

I feel like you'd be better off using the FunctionProtoType and iterating over argument types, rather than the argument declarations on a particular declaration of the function.

e.g. this code is legal in C:

int x(); // i suspect this is the canonical decl
int x(int); // but this one provides the type

We don't have references in C of course!, but maybe similar issues lurking...

562

Yeah this is the core of this modifier, worth being precise and explicit here. I think we want to match only reference types where the inner type is not top-level const.

I think we should also conservatively forbid the inner type from being *dependent*. Consider the following function:

template <typename X>
void foo(X& x) {
  foo(x); // this call
}

Locally, the recursive call looks like "mutable reference to dependent type". But when instantiated, X may be const int and is in fact very likely to be deduced that way in practice.

566

You may want to add an "unwrapping" step so that e.g. ArraySubscriptExpr a[i] can be unwrapped to a and then possibly highlighted (and its operator-overloaded form).

Wouldn't suggest doing that in this patch, but you could leave a note if you like.

567

And getLocation() will do the right thing for DeclRefExpr, MemberExpr, and others, so this can just be isa<DeclRefExpr, MemberExpr> with no need for dyn_cast.

clang-tools-extra/clangd/SemanticHighlighting.h

73

nit: no line break here. The line breaks separate normal modifiers vs scope modifiers (which are mutually exclusive) vs sentinels

74

nit: "mutable" is more natural than "non-const", and modifiers don't use heavily C++-specific names anyway

Please spell out "reference", we don't use other abbreviations.

I'd consider "UsedAsMutableReference" rather than "passed" as this seems to be the deeper semantic idea we're expressing, and we may want to generalize this in some way

[nridge]

clang-tools-extra/clangd/SemanticHighlighting.cpp

312

I don't have a strong opinion on the options here, just wanted to chime in and say I also highlight Unknown prominently for similar reasons :)

[tom-anders]

clang-tools-extra/clangd/SemanticHighlighting.cpp

567

I'm not sure which getLocation() you're talking about here. There's DeclRefExpr::getLocation(), but neither Expr::getLocation() nor MemberExpr::getLocation(). Am I missing something?

[sammccall]

clang-tools-extra/clangd/SemanticHighlighting.cpp

567

No, I think I'm just going mad (I was thinking of Decl::getLocation I guess).
Never mind and sorry!

[tom-anders]

clang-tools-extra/clangd/SemanticHighlighting.cpp

558

I'm not really sure how to get from the CallExpr to the FunctionProtoType, can you give me a hint?

567

np :D

[nridge]

clang-tools-extra/clangd/SemanticHighlighting.cpp

558

I think FD->getType()->getAs<FunctionProtoType>() should do it

[tom-anders]

clang-tools-extra/clangd/SemanticHighlighting.cpp

312

I switched to @sammccall's idea of having a map<Range, Modifier>, I agree that it feels more natural than the previous version

534

Agreed, added a FIXME

562

I adjusted the condition accordingly and added a few more test cases (including stuff like const int*&).

566

Added a FIXME.

567

Switched to getLocation() and added a test case.

[tom-anders]

• Apply suggested renaming and fix nits
• Use a map for extra modifiers instead of abusing conflict resolution
• Use ArrayRef instead of callback
• Add FIXME for handling CXXOperatorCallExpr
• Use getLocation() instead of getBeginLoc()
• Add FIXME for unwrapping operators
• Iterate over FunctionProtoType instead of FunctionDecl
• Adjust highlighting condition and revise test cases

[tom-anders]

• Remove accidentally added empty line

[tom-anders]

• clang-format once again

[tom-anders]

@sammccall @nridge Thanks for the positive feedback, let me know if I missed any of your suggestions!

[nridge]

Thanks for the update! Looks pretty good, just a few minor comments from me. Will leave approval to Sam.

clang-tools-extra/clangd/SemanticHighlighting.cpp

470

Looking at existing usage of associative containers in clangd code, llvm::DenseMap and llvm::DenseSet (which are hashtable-based) are more commonly used than std::map and std::set, though it may require some boilerplate to teach them to use new key types (example)

We could also consider making the value just a vector instead of a set (maybe even an llvm::SmallVector<HighlightingModifier, 1> given its anticipated usage), since even in the unlikely case that we get duplicates, addModifier() will handle them correctly

553

nit: declarations --> types

554

nit: parameters

560

I think we can relax the dependent type check here a bit, for example std::vector<T>& is a dependent type but it's definitely a mutable reference.

(Feel free to leave that for later and just add a FIXME.)

566

Maybe also add a // FIXME: Handle dependent expression types just so we don't forget

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

773

(test case needs uncommenting)

[tom-anders]

clang-tools-extra/clangd/SemanticHighlighting.cpp

470

Agreed, changed std::set to llvm::SmallVector. I can't really judge if it's worth using llvm::DenseMap here though.

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

773

Actually it needed to be removed, has been replaced by the test above

[tom-anders]

• Use llvm::SmallVector, add more FIXME, remove old commented out test

[nridge]

Thanks for the updates! LGTM from my side.

clang-tools-extra/clangd/SemanticHighlighting.cpp

555

nit: still a typo here ("paramteres")

562

nit: typo (idDependentType)

[sammccall]

Thanks! This looks good to me to land as is.
I'd like to see the std::map eliminated, but if you don't want to do this I'm happy to take a stab at it after.

Want someone to commit this for you?

clang-tools-extra/clangd/SemanticHighlighting.cpp

470

We do try to avoid std::map/std::set as they're pretty gratuitous performance sinks. And the map here may not be tiny.

I'd suggest either:

• make this a flat vector<pair<Location, HighlightingModifier>>, sort it after building, and use binary search to find the right place. This has the same big-O as std::map but without all the allocations and pointer chasing
• Specializing DenseMapInfo<Location> in protocol.h and using DenseMap<Location, SmallVector<HighlightingModifier,1>>

We don't really need the full Range in the key, as the start defines it.

We could also consider making the value just a vector instead of a set

In practice these sets of modifiers end up being a bitmask, so if using a map, you could just store a uint32_t of 1<<Modifier and add an addModifiers(uint32_t) to HighlightingToken. I'm too really worried about preserving that encapsulation.

[tom-anders]

Yeah, I‘d need someone to commit this for me.

About the std::map thing: let’s proceed as suggested and have @sammccall take look at it himself after :)

[nridge]

I'm happy to commit it. Did you want to make the SmallVector<1> --> uint32_t change, or should I just commit it as is?

[tom-anders]

I’d say commit as is and have a look at the SmallVector -> unit32_t thing together with the std::map issue afterwards

[nridge]

Apologies, forgot about this. Commited now :)

clang-tools-extra/clangd/SemanticHighlighting.h

enum class HighlightingModifier {
   Deprecated,
   Deduced,
   Readonly,
   Static,
   Abstract,
   Virtual,
   DependentName,
   DefaultLibrary,
+  UsedAsMutableReference,
 
   FunctionScope,
   ClassScope,
   FileScope,
   GlobalScope,
 
   LastModifier = GlobalScope
 };
 static_assert(static_cast<unsigned>(HighlightingModifier::LastModifier) < 32,
               "Increase width of modifiers bitfield!");

clang-tools-extra/clangd/SemanticHighlighting.cpp

 // - macro arguments are expanded multiple times and have different roles
 // - broken code recovery produces several AST nodes at the same location
 //
 // We should either resolve these to a single token, or drop them all.
 // Our heuristics are:
 //
 // - token kinds that come with "dependent-name" modifiers are less reliable
 //   (these tend to be vague, like Type or Unknown)
 // - if we have multiple equally reliable kinds, drop token rather than guess
 // - take the union of modifiers from all tokens
 //
 // In particular, heuristically resolved dependent names get their heuristic
 // kind, plus the dependent modifier.
 llvm::Optional<HighlightingToken>
 resolveConflict(ArrayRef<HighlightingToken> Tokens) {
   if (Tokens.size() == 1)
     return Tokens[0];

 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
   HighlightingsBuilder(const ParsedAST &AST)
       : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
         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) {
     HighlightingToken HT;
     HT.R = std::move(R);
     HT.Kind = Kind;
     Tokens.push_back(std::move(HT));
     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.
     llvm::sort(Tokens);
     auto Last = std::unique(Tokens.begin(), Tokens.end());
     Tokens.erase(Last, Tokens.end());
 
     // Macros can give tokens that have the same source range but conflicting
     // kinds. In this case all tokens sharing this source range should be
     // removed.
     std::vector<HighlightingToken> NonConflicting;
     NonConflicting.reserve(Tokens.size());
     for (ArrayRef<HighlightingToken> TokRef = Tokens; !TokRef.empty();) {
       ArrayRef<HighlightingToken> Conflicting =
           TokRef.take_while([&](const HighlightingToken &T) {
             // TokRef is guaranteed at least one element here because otherwise
             // 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();
 
     // Merge token stream with "inactive line" markers.
     std::vector<HighlightingToken> WithInactiveLines;
     auto SortedSkippedRanges = AST.getMacros().SkippedRanges;
     llvm::sort(SortedSkippedRanges);
     auto It = NonConflicting.begin();
std::vector<HighlightingToken> collect(ParsedAST &AST) && {
     for (; It != NonConflicting.end(); ++It)
       WithInactiveLines.push_back(std::move(*It));
     return WithInactiveLines;
   }
 
   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;
 };
 
 llvm::Optional<HighlightingModifier> scopeModifier(const NamedDecl *D) {
   const DeclContext *DC = D->getDeclContext();
   // Injected "Foo" within the class "Foo" has file scope, not class scope.

 
 /// Produces highlightings, which are not captured by findExplicitReferences,
 /// e.g. highlights dependent names and 'auto' as the underlying type.
 class CollectExtraHighlightings
     : public RecursiveASTVisitor<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)
                       .addModifier(HighlightingModifier::Deduced);
       if (auto Mod = scopeModifier(L.getTypePtr()))
         Tok.addModifier(*Mod);
       if (isDefaultLibrary(L.getTypePtr()))
         Tok.addModifier(HighlightingModifier::DefaultLibrary);
llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier) {
   case HighlightingModifier::Abstract:
     return "abstract";
   case HighlightingModifier::Virtual:
     return "virtual";
   case HighlightingModifier::DependentName:
     return "dependentName"; // nonstandard
   case HighlightingModifier::DefaultLibrary:
     return "defaultLibrary";
+  case HighlightingModifier::UsedAsMutableReference:
+    return "usedAsMutableReference"; // 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:            "deprecated",
 # 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:            "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:      4097
+# CHECK-NEXT:      8193
 # 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:          4097
+# CHECK-NEXT:          8193
 # 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:      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:  }
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

)cpp",
         struct $Class_decl[[Derived]] : $Class[[Base]]<$TemplateParameter[[T]]> {
           using $Class[[Base]]<$TemplateParameter[[T]]>::$Field_dependentName[[member]];
 
           void $Method_decl[[method]]() {
             (void)$Field_dependentName[[member]];
           }
         };
       )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.
     // They're tested separately.
     checkHighlightings(TestCase, {}, ~ScopeModifierMask);
 
   checkHighlightings(R"cpp(
     class $Class_decl[[A]] {
       #include "imp.h"