-
-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
semantic tokens: add infix operator #4030
Conversation
@@ -66,7 +77,7 @@ tyThingSemantic ty = case ty of | |||
| isTyVar vid -> Just TTypeVariable | |||
| isRecordSelector vid -> Just TRecordField | |||
| isClassOpId vid -> Just TClassMethod | |||
| isFunVar vid -> Just TFunction | |||
| isFunVar vid -> Just TFunction <> (nameInfixOperator $ getName vid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're going to merge them all with <>
later, perhaps tyThingSemantic
should just return [HsSemanticTokenType]
, since in general we might allocate several types? That would simplify some things, e.g. here you're implicitly duplicating the fact that TClassMethod
is preferred over TFunction
by putting the match for isClassOpId
first. (So if you ever changed the preference in the Semigroup instance you would still get the old answer!).
So you could do something like:
AnId vid -> [TTypeVariable | isTyVar vid] ++ ... ++ [TFunction | isFunVar vid] ++ [TOperator | isSymId vid]
and then let the semigroup instance sort it all out. Less important for the other cases where we don't have overlapping token types, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, let semigroup instance sort it all out would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you would actually have to do this if you wanted to make the priorities configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and it is going to be more tricky when priorities is configurable. Since we need to make sure the token type consistant between the one from hieAst and the one from typeThing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that should work out okay with the model where we just collect (possibly many) token types and then pick the "best". We'll have to have a "best" function that is derived from the configuration, instead of just the hardcoded one based on the Semigroup instance, but that seems okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we might want to switch to list or set eventually with priorities configurations. Since by then, we might need to add test to ensure the all the results coming from imported or local names would have the same set of token types(instead of the current strategy to have just the same token type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test to ensure the all the results coming from imported or local names would have the same set of token types
You mean if I have
module A where
foo = ...
module B where
import A
bar = foo
then foo
should have the same tokens in both cases? That does seem like a nice property!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, we would have to have that property or the configuration would not work well.
($$$$) = b | ||
x = 1 $$$$ 2 | ||
data a :+: b = Add a b | ||
-- type take precedence over operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think people might prefer to see type operators highlighted differently? In "normal" Haskell syntax highlighting they are!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've been thinking about this, may be it's better for us to mark all the infix thing as operator.
Since infix does change the semantic of normal function application ordering, visually showing this would be more preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel reasonably sure that we want "operator" to take precedence over "type". I'm still unsure about "operator" vs "class method"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me neither, maybe we can make a pool or something to collect people's opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I think it's fine for you to just make a decision for now, we can adjust based on feedback or add configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, lets stick to the simplest way for now. Make the infix on top of all.
We can sort things out in configuration PR that switching to the [HsSemanticTokenType]
.
Trying to implement #3969
I am still thinking by default, we should take infix operator or class method(we have infix class method) or type(we have type operator) as precedent.After discussion, I made a decision, temporally operator have its priority over them by default.