Skip to content

fix: "Go to definition" functionality for native LSP#1859

Merged
jbencin merged 3 commits intostx-labs:mainfrom
jbencin:fix/go-to-definition
Jun 12, 2025
Merged

fix: "Go to definition" functionality for native LSP#1859
jbencin merged 3 commits intostx-labs:mainfrom
jbencin:fix/go-to-definition

Conversation

@jbencin
Copy link
Contributor

@jbencin jbencin commented Jun 10, 2025

Description

Fixes: #1829

This PR fixes "Go to definition" functionality for native LSP, which is broken because the following code:

Url::parse(&definition_contract_location.to_string()).ok()

Always returns None for FileLocation::FileSystem paths (because the to_string() output is missing the file:// prefix). This is fixed by implementing TryInto<Url> for FileLocation to do the correct type conversion

Breaking change?

I don't think this will affect the VSCode extension (I'm guessing it always uses FileLocation::Url)

Notes

In a future PR I think we can get rid of FileLocation enum completely and just use Url directly

@jbencin jbencin requested review from hugoclrd and tippenein June 10, 2025 15:08
@codecov
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 37.50000% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
components/clarinet-files/src/lib.rs 46.15% 7 Missing ⚠️
components/clarity-lsp/src/common/state.rs 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@hugoclrd
Copy link
Contributor

hugoclrd commented Jun 11, 2025

I don't think this will affect the VSCode extension (I'm guessing it always uses FileLocation::Url)

Right, it shouldn't break.
You can give it a try. In component/clarity-vscode run npm i && npm run dev. It'll open a test project in vscode web.

In a future PR I think we can get rid of FileLocation enum completely and just use Url directly

Yes! I agree that the FileLocation is probably not necessary and adds useless complexity

@jbencin
Copy link
Contributor Author

jbencin commented Jun 11, 2025

Right, it shouldn't break

Tried this and goto functionality is still working in VSCode. I don't think it ever uses filesystem paths. Actually, the parts of the Url crate dealing with the filesystem don't even build for this target because it's a no_std environment

Copy link
Contributor

@tippenein tippenein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jbencin jbencin merged commit 93461dc into stx-labs:main Jun 12, 2025
24 checks passed
@jbencin jbencin deleted the fix/go-to-definition branch June 12, 2025 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native LSP: Go to definition/declaration/etc. doesn't work

3 participants