[PATCH] D136594: [clangd] Add support for semantic token type "operator"

Fri Nov 25 04:51:54 PST 2022

ckandeler added a comment.

================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,

----------------
sammccall wrote:
> sammccall wrote:
> > nridge wrote:
> > > ckandeler wrote:
> > > > ckandeler wrote:
> > > > > sammccall wrote:
> > > > > > sammccall wrote:
> > > > > > > sammccall wrote:
> > > > > > > > Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
> > > > > > > > (Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)
> > > > > > > as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5
> > > > > > > 
> > > > > > > If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.
> > > > > > Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> > > > > > (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)
> > > > > > as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5
> > > > > > 
> > > > > > If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.
> > > > > 
> > > > > I use "userProvided" here in the sense of "not built-in" or "overloaded". I do not cling to the term, and have also suggested the opposite default of using "builtin" in another comment.
> > > > > Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> > > > 
> > > > True, I have not considered this.
> > > > 
> > > > > (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)
> > > > 
> > > > Intuitively, it seems we should be conservative and not claim the operator is overloaded unless we know it is. So "built-in" might then mean "we can't prove it's not a built-in". It's probably not worth to introduce a modifier CouldBeEitherWay just to explicitly express ambiguity ;)
> > > > Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
> > > > (If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)
> > > 
> > > In my mind, "go-to-definition on this operator symbol will take me to a function declaration/definition" is a good match for "I want this colored differently". (Which would imply treating dependent operator calls where we can't figure out an overloaded operator target even heuristically, as "built-in".)
> > > Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
> > > (Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)
> > 
> > This was one of the biggest questions I had about this patch - just hoping it doesn't get missed.
> > Intuitively, it seems we should be conservative and not claim the operator is overloaded unless we know it is. 
> 
> This feels a bit circular, if we agree we're not going to introduce a `CouldBeEitherWay` then why is "built-in" a more conservative claim than "overloaded"?
> 
> I'm inclined towards `builtin` as a modifier because I think for language entities as a whole (types, functions etc, not just operators) it's the exception. It also seems easier to name and define.
> 
> > In my mind, "go-to-definition on this operator symbol will take me to a function declaration/definition" is a good match for "I want this colored differently".
> 
> This mostly makes sense to me, but:
>  - I don't think we should actually run all the heuristics logic
>  - if there's probably a definition available but we can't resolve it due to templates, I'd still like to know something's up
> 
> I think my internal question is more like "is this a trivial arithmetic shift, or something potentially complicated"? And I think depending on template resolution is "potentially complicated". (Maybe trivial in the end, but so might be an overloaded operator)
> > Intuitively, it seems we should be conservative and not claim the operator is overloaded unless we know it is. 
> 
> This feels a bit circular, if we agree we're not going to introduce a `CouldBeEitherWay` then why is "built-in" a more conservative claim than "overloaded"?
> 
> I'm inclined towards `builtin` as a modifier because I think for language entities as a whole (types, functions etc, not just operators) it's the exception. It also seems easier to name and define.
> 
> > In my mind, "go-to-definition on this operator symbol will take me to a function declaration/definition" is a good match for "I want this colored differently".
> 
> This mostly makes sense to me, but:
>  - I don't think we should actually run all the heuristics logic
>  - if there's probably a definition available but we can't resolve it due to templates, I'd still like to know something's up
> 
> I think my internal question is more like "is this a trivial arithmetic shift, or something potentially complicated"? And I think depending on template resolution is "potentially complicated". (Maybe trivial in the end, but so might be an overloaded operator)

The documentation for BinaryOperator says:  
/// Within a C++ template, whether BinaryOperator or CXXOperatorCallExpr is
/// used to store an expression "x + y" depends on the subexpressions
/// for x and y. If neither x or y is type-dependent, and the "+"
/// operator resolves to a built-in operation, BinaryOperator will be
/// used to express the computation (x and y may still be
/// value-dependent). If either x or y is type-dependent, or if the
/// "+" resolves to an overloaded operator, CXXOperatorCallExpr will
/// be used to express the computation.
With the patch as-is (possibly with the UserProvided/BuiltIn switch) , this should result exactly in what you want (if I understand you correctly). However, it does not match my observation: I always see the normal operator types. Am I missing something or is the documentation wrong?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136594