Skip to content

fix missing diagnostics if urls mismatch #73

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

Merged
merged 2 commits into from
Aug 5, 2025
Merged

Conversation

Leandros
Copy link
Collaborator

When the Uri passed to the LSP server is, for example, file:///home/leandros/testdiagtest/src/main_tests.rs, and the file_name from the invocation to cargo clippy is /home/leandros/testdiagtest/src/./main_tests.rs, bacon-ls would've discarded all diagnostics and showed none.

This is fixed by "canonicalizing" the path, that is, removing all instances of . or .. and returning a full path.

@Leandros Leandros requested a review from crisidev July 31, 2025 16:26
@Leandros Leandros self-assigned this Jul 31, 2025
@Leandros Leandros added the bug Something isn't working label Jul 31, 2025
@Leandros Leandros marked this pull request as draft August 1, 2025 07:18
@crisidev
Copy link
Owner

crisidev commented Aug 1, 2025

Nice find!

@crisidev
Copy link
Owner

crisidev commented Aug 1, 2025

When this is merged, I'll release new version.

@Leandros
Copy link
Collaborator Author

Leandros commented Aug 4, 2025

The change has unearthed another oddity of the LSP protocol, if the authority of a file_name in a span is empty, the path is absolute. This was occurring for me, and it made bacon panic because it couldn't find the file.

This behavior is now encoded into maybe_add_diagnostics.

I've also extended the debug logging slightly so these errors are easier to catch in the future (also, when receiving reports from users).

@Leandros Leandros marked this pull request as ready for review August 4, 2025 09:56
@crisidev
Copy link
Owner

crisidev commented Aug 5, 2025

CI is failing because you need to run cargo fmt. After that, feel free to merge.

I experienced the same oddity you found, it's great you had time to find the root cause.

@Leandros
Copy link
Collaborator Author

Leandros commented Aug 5, 2025

Huh. Interesting. My local cargo fmt didn't reorder the imports. However, I fixed it manually.

Will merge once CI succeeds.

@Leandros Leandros merged commit 9c47c7c into crisidev:main Aug 5, 2025
3 of 4 checks passed
@crisidev
Copy link
Owner

crisidev commented Aug 5, 2025

I am a bit flooded at work, I'll release a new version over the weekend probably. Thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants