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

fix(rust_analyzer): revert is_library regression #3198

Conversation

kulkalkul
Copy link

In #2995, root check is replaced with is_descendant. Author of the PR then figured out issue was unrelated to the way root is checked, but kept the new implementation because it was descriptive on what it does. But on Windows, this causes the issue that #2645 is_library fixes.

So the issue is, is_descendant function calls traverse_parents, which converts the path using:

path = uv.fs_realpath(path)

Which, in a Windows environment, this converts UNIX paths to Windows paths. So this requires items in line 27 to be also converted. BUT that doesn't work because dirname is not able to work with Windows paths. So I guess this is a bigger issue on Windows support, but I don't know the codebase enough to change how utils work and have that responsibility.

So, if that's it, why isn't this an issue and a PR instead. Because I think is_descendant is wrong tool here. For valid cases of items there, substring check is exact and does it in a way simpler way. I also added a comment above to make it descriptive on what it does. So this essentially reverts part of #2995 that changes how descendant is checked and adds a comment.

@kulkalkul kulkalkul requested a review from glepnir as a code owner June 9, 2024 13:03
@glepnir
Copy link
Member

glepnir commented Jun 9, 2024

The original question was just to consider reusing the lib for an already open client. At the time I fixed it on 0.9x. Now we have 0.10. This version has the vim.fs module. maybe we can use vim.fs.normalize it first ?

@kulkalkul
Copy link
Author

kulkalkul commented Jun 9, 2024

Hmhmh, I also figured this only solved one part of the issue. So:

  • Issue I had:
    Diagnostics work as expected when project is loaded, but after I use "go to declaration", it was reindexing that library and adding its diagnostics.

  • Issue I now have, with the change above:
    "go to declaration" is instant, no more reindexing. But now it shows diagnostic errors from libraries. I'm using nightly rust, but libraries I'm using are on stable rust. So diagnostic errors are complaining "you are using nightly rust in a stable crate".

Later today or tomorrow I can create a reproducible example for it to show those two different issues. Do you want me to post it here, or close this PR and open an issue for it?

@glepnir
Copy link
Member

glepnir commented Jun 10, 2024

more like an issue tracker :)

@glepnir glepnir closed this Jun 10, 2024
@kulkalkul
Copy link
Author

Hey! So sorry for the late response, while working on this reproducable repo, I accidentally rm -rf'd my local project's .git repo which I had no backup of. It was mostly a self destructive week after the incident (unrelated, personal), I'm back on the track now. I reinvestigated the issue, looks like the issue I had was unrelated to the changes in the PR, so this PR actually solves the issue; my bad!

I think in this case continuing with this PR suits better. I'll see if vim.fs.normalize is useful, though my config is still 0.9 and I have to get back to the rhythm before messing around with my config 😅 So it might take several days before I tackle this

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.

None yet

2 participants