[PATCH] D108320: Add semantic token modifier for non-const reference parameter

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:314
 //   (these tend to be vague, like Type or Unknown)
+// - Resolved tokens (i.e. without the "dependent-name" modifier) with kind
+//   "Unknown" are less reliable than resolved tokens with other kinds
----------------
nridge wrote:
> 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?
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 :-)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:514
+  bool VisitCallExpr(CallExpr *E) {
+    // Highlighting parameters passed by non-const reference does not really
+    // make sence for these
----------------
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" :-)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:528
+
+  void highlightPassedByNonConstReference(
+      const FunctionDecl *FD,
----------------
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`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:538
+    for (size_t I = 0; I < FD->getNumParams(); ++I) {
+      if (const auto *Param = FD->getParamDecl(I)) {
+        auto T = Param->getType();
----------------
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...


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:542
+        // Is this parameter passed by non-const reference?
+        if (T->isLValueReferenceType() && !isConst(T)) {
+          if (auto *Arg = GetArgExprAtIndex(I)) {
----------------
nridge wrote:
> 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.)
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:546
+
+            if (isa<DeclRefExpr>(Arg)) {
+              Location = Arg->getBeginLoc();
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:547
+            if (isa<DeclRefExpr>(Arg)) {
+              Location = Arg->getBeginLoc();
+            } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
----------------
nridge wrote:
> 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.)
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:72
   DefaultLibrary,
 
+  PassedByNonConstRef,
----------------
nit: no line break here. The line breaks separate normal modifiers vs scope modifiers (which are mutually exclusive) vs sentinels


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:73
 
+  PassedByNonConstRef,
+
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108320/new/

https://reviews.llvm.org/D108320