LSP: fix "call hierarchy" across files #26040
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jabraham17
approved these changes
Oct 4, 2024
| @@ -1128,19 +1164,34 @@ def get_file_info( | |||
| creating one if it doesn't exist. If do_update is set to True, | |||
| then the FileInfo's index is rebuilt even if it has already been | |||
| computed. This is useful if the underlying file has changed. | |||
|
|
|||
| Most of the tiem, we will create a new context for a given URI. When | |||
| ): | ||
| self.show_message( | ||
| "Call hierarchy item contains missing or invalid additional data", | ||
| MessageType.Error, | ||
| ) | ||
| return None | ||
| uid, idx = item.data | ||
| uid, idx, ctx = item.data |
DanilaFe
force-pushed
the
michael-bugfix
branch
from
04d782b
to
4b6d07f
Compare
Signed-off-by: Danila Fedorin <[email protected]>
…l hierarchy Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
No Jade, did not hit CI warning this time :) Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
DanilaFe
force-pushed
the
michael-bugfix
branch
from
f3bf535
to
00aee9e
Compare
Signed-off-by: Danila Fedorin <[email protected]>
jabraham17
approved these changes
Oct 14, 2024
| @@ -0,0 +1,215 @@ | |||
| """ | |||
| Tests basic functionality, including autocompletion, go-to-definition, hover, | |||
Signed-off-by: Danila Fedorin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the following program, trying to get the call hierarchy for
writelndid not work onmain.There were a few issues with this. The first issue is that the the "index" that was fed through a
CallHierarchyItemto LSP and back toCLSwas an index into a single file's list of instantiations. Thus, if the function was instantiated in one place, but defined in another, the index would not be valid (or worse, be incorrect). This PR changes this by changing the way typed signatures are mapped to JSON-compatible identifiers; they are instead registered with aContextContainerobject, getting a unique ID. The unique ID is then used as JSON data, and can be used to access the sameTypedSignatureobject when the JSON is being deserialized.This, however, was not enough. The next problem was that we currently create a new
ContextContainerper file (to handle the case in which multiple files with the same name / module name exist). As a result of this,writelnthat is called in moduleAand defined in moduleBwill have its instantiation computed usingA's context, but its AST and other information will useB's context. This is troublesome. This PR adjusts the situation by changing thefile -> FileInfomapping to be a(file, context id) -> FileInfomapping. This way, the same file can have several contexts (e.g., if a standard library file is loaded from two distinct files, but both namedA.chpl). By default (e.g., when opening a file), the(file, "None")key is used (since no particular context was requested). However, if a file A needs to show an instantiation (via file info) from file B, we create a new mapping(fileB, fileA.context.id) -> fileInfoB, which is created from the same context. This ensures that there is a file information object for file B with the same context as that of file A.When a call hierarchy item is constructed, it now specifies what context was used to create it. Thus, if other CallHierarchyItems point to other files, these other files are opened with the same context. To communicate the context over JSON, I employ the same approach as I did with typed signatures (register a context object with unique ID, use the ID to marshal the context to and from JSON).
To fix the case ofThis was separately merged in #26078.writelnin particular, I had to relax an assertion from the_owned/_shared_resolution PR #25617. This led to some simplified code in any case, which is great. I believe @arezaii is fine with this change.A bug that I'm surprised we didn't find prior to this, which is fixed in this PR, is that vector unmarshalling for Python interop allocates a vector of the Python list's size, then pushes additional elements instead of setting the already-allocated locations. This ended up e.g., pushing an empty string for each "real" string a user passed in a list. This led to some odd interactions with Dyno's module searching (an empty string as an input file led to "." being added to the module search path, which we do not want).
Reviewed by @jabraham17 -- thanks!
Testing