Skip to content
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

[relay-lsp] Calculate LSP locations from cached JavaScriptSourceFeatures #4509

Closed

Conversation

tobias-tengler
Copy link
Contributor

@tobias-tengler tobias-tengler commented Oct 27, 2023

Currently, when transforming a location to an LSP location, JavaScriptSourceFeatures are extracted from the corresponding file and then used to calculate the LSP location.
The problem is that what's on disk doesn't necessarily match the current document state. If you have unsaved changes in your editor, locations will be calculated on the older (persisted-to-disk) file content.

This PR tries to get the JavaScriptSourceFeatures from the synced sources first (which represent the current document state) and if it can't, falls back to reading from disk.

@tobias-tengler
Copy link
Contributor Author

@captbaritone I'm working on some refactorings and it would be great to get this one in beforehand.

@captbaritone
Copy link
Contributor

Thanks so much for this. Sorry I've let it languish. There is still one wrinkle, which is that for most of these LSP features the location information is computed from the program IRs which I believe are derived from the on-disk representation of the file, not from the potentially unsaved state.

I guess if you have unsaved changes and we report a location in that file that we've derived from on-disk state we're going to show. you the wrong location anyway so this is no worse? Or put another way, there's no way at that point that we can derive a correct location so it's not a problem if its still wrong.

@tobias-tengler
Copy link
Contributor Author

Yes, that's true. I like the approach you took in your inlay hint PR with having both options available and making their behavior explicit through naming. I assume your PR is more likely to land, so I'll just leave this open for now and will close it once yours is merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants