Implement basic semantic highlighting (method calls, local vars) #32
Conversation
| @@ -77,13 +77,20 @@ def add_token(location, classification) | |||
| line = @parser.line_counts[location.start_line - 1] | |||
| column = location.start_char - line.start | |||
|
|
|||
| if row < @current_row | |||
| raise InvalidTokenRowError, "Invalid token row detected: "\ | |||
There was a problem hiding this comment.
I didn't add a test case for this because... this method is private, and well, it's a little hard to unit test this particular case.
|
|
||
| def visit_assign(node) | ||
| case node.target | ||
| when SyntaxTree::ARefField |
There was a problem hiding this comment.
Why not implement visit_a_ref_field and whatever the visit_ method correspond to the else branch?
There was a problem hiding this comment.
✅ Done in 73d376f
| private | ||
|
|
||
| def add_token(location, classification) | ||
| length = location.end_char - location.start_char |
There was a problem hiding this comment.
Can we copy part of the explanation at https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_semanticTokens to explain what's going on here please? 🙏
There was a problem hiding this comment.
✅ Added the comment in d22c414
I didn't add how each delta is calculated but I just linked the doc instead - is that okay?
Note that this change only covers the basic cases and requires more refinement to special cases like instance variables.
…methods Also, make delta column calculation more readable
wildmaples
force-pushed
the
add_semantic_highlighting
branch
from
fba3b1a
to
d22c414
Compare
|
Thanks everyone for the review! For full disclosure: in this current PR, instance variable reference and fields are both being counted as a local variable. We initially intended to try to fix it before merging. However, I did some digging to realize that it will be a larger change. It's because VarField and VarRef are both being counted as local variables but in reality the nodes could be the parent of a variety of other nodes aside from local variables, one of them being instance variables. I chatted with @vinistock and we are ok with merging this for now and working on instance variables next. 👍 |
…lint-import-resolver-typescript-2.7.0 Bump eslint-import-resolver-typescript from 2.5.0 to 2.7.0
Issue
Some of: https://github.com/Shopify/sorbet-issues/issues/518 (Add semantic highlighting)
Changes
Add semantic highlighting for local variables and methods - f call, v call and normal calls (see unit test cases for a better picture on what each of those calls are) ➡️ This is primarily @vinistock work 🙏 😄
Cover the case where a method call receiver is a variable (e.g.
var = "yo"; var.upcase)Modified tests to be more human readable - namely just putting the deltas in a hash with readable key, then translating them before asserting
Add error to ensure we don't get bitten by the same issue where children nodes were being added after the parents. Consider the following:
syntax_treevisits the call#upcasebefore the receiver"hello". However, we want to add"hello"before#upcase, which is the in-order traversal VSCode API requires. (I think that makes sense because that's how the code is presented in view.) If we add the call token before the receiver token, we would come up with the wrong delta calculations and we'd like to avoid that by raising an error.Real time testing
I wrote a bit if people are curious how to test this in real time: https://github.com/Shopify/sorbet-issues/issues/518#issuecomment-1083391665