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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked 12 inline comments as done.
tom-anders added inline comments.


================
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:
> sammccall wrote:
> > 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 :-)
> 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 :)
I switched to @sammccall's idea of having a map<Range, Modifier>, I agree that it feels more natural than the previous version


================
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
----------------
sammccall wrote:
> 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" :-)
Agreed, added a FIXME


================
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)) {
----------------
sammccall wrote:
> 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.
I adjusted the condition accordingly and added a few more test cases (including stuff like `const int*&`). 


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


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:547
+            if (isa<DeclRefExpr>(Arg)) {
+              Location = Arg->getBeginLoc();
+            } else if (auto *M = dyn_cast<MemberExpr>(Arg)) {
----------------
tom-anders wrote:
> sammccall wrote:
> > tom-anders wrote:
> > > sammccall wrote:
> > > > 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.
> > > 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?
> > No, I think I'm just going mad (I was thinking of Decl::getLocation I guess).
> > Never mind and sorry!
> np :D 
Switched to `getLocation()` and added a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108320