Fix rascal hovers showing random types #262
Conversation
|
Indeed the summary should filter locations outside current module. |
|
Which tree are we walking? I can't find that code in the source. Can you point me to it @DavyLandman ? Typically in the eclipse version of this functionality, we'd use the cursor offset in the file to find a path in the tree to all relevant source locations, in |
| // search all the way to the "bottom" of the tree to see if we are | ||
| // contained in something larger than the closest key | ||
| var previousKeys = data.headMap(from, true).descendingMap(); | ||
| for (var candidate : previousKeys.entrySet()) { |
There was a problem hiding this comment.
I'd rather read the type of candidate here instead of var. I'm getting lost. Same for previousKeys.
There was a problem hiding this comment.
Hmm, I don't really see the value, descendingMap becomes a NavigatableMap<Range, T> and candidated Entry<Range, T>. But for me the readability doesn't improve by it, but that might be because I wrote the code. I would prefer to give better names to the variables.
The |
|
I think we might separate the (fuzzy) search from the translation; if we use the parse Tree to provide a (list of) candidate source locations, then these ranges can be translated one-to-one to the LSP Range concept. There is no need to walk over the entire data-structure anymore, and the lookup would be twice in log time, once log of the size of the input file and once log the size of the lookup cache. WDYT? |
|
Sounds like a new PR :) This is fixing the bug we have right now. I don't exactly understand your proposal. We get a summary from the rascal-core with just a few rels in there, do you want to put it back in a parse tree? I was thinking we could build a specific RB tree to do a some better mapping of this logic, but for now using TreeMaps are good enough. It just could be faster. |
|
The eclipse mirror of the summary feature gets a current parse tree and the cursor position, and the current summary. First the tree is used to extract which ranges are relevant for the current lookup. This is done by traversing a path in the tree that contains the cursor position. The rest of the tree can be ignored, naturally. This gives it the worst case log complexity behavior. Then the target ranges are identified, which can be searched in the summary, using the index of the relation. Then the output can be composed out of the answers of the previous and translated to ranges in LSP format. |
|
Ah, thanks for explaining that in a bit more detail. |
… check if it still works
|
Kudos, SonarCloud Quality Gate passed! |
|
I've added a random test that tries to fill the TreeMap with all kinds of ranges (trying to cram lots of overlapping ranges in there) and then made sure the specific range would still be found. I propose we go ahead with this, and think of a different approach at a later point. As in, it's primarily a performance discussion after this right? |
|
I'm testing the fixes:
|
|
Ok, I'm retracting the jump-to-definition bug:
So this was an older .tpl file in my target folder that got unzipped. It is an instance of #242 ; at least if we would have used
|
|
It really works a lot better now. |
Fixes #260
As discussed, there were 3 bugs:
This PR fixes all 3, where I'm a bit sad I have to walk the whole tree, but that seems inevitable.