Skip to content

fix: Make rust-analyzer.files.excludeDirs work, actually #18998

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
Feb 11, 2025

Conversation

ChayimFriedman2
Copy link
Contributor

I have no idea what the original writer of the code thought (CC @matklad, I don't expect you to remember but it was you), but the logic just seems backwards. We should not exclude a file/directory if it is equal to an include! This also meant that we had to add a root == path check so this stuff will actually work, which in turn meant excludes (of root files) no longer worked...

Also rename if to rust-analyzer.files.exclude, because it can exclude files as well.

Fixes #14734.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2025
@ChayimFriedman2 ChayimFriedman2 marked this pull request as draft January 21, 2025 23:59
@matklad
Copy link
Member

matklad commented Jan 22, 2025

The original logic still looks correct to me!

It handles nested entries. If root B is nested under A, and C is excluded, then, while scanning A:

  • you should ignore C as it is explicitly excluded
  • But you also should ignore B, as you get to scanning the contents of B separately

That is, there’s an outer loop over A and B anyway. Without this exclusion, B will be scanned twice

@ChayimFriedman2
Copy link
Contributor Author

Okay, so the logic needs to be:

dirs.exclude.iter().all(|it| it != path) && (root == path || dirs.include.iter().all(|it| it != path))

@matklad
Copy link
Member

matklad commented Jan 22, 2025

To make sure I understand correctly, what happens here is that the same path ends up in exclude (explicitly configured by the user) and also in the include (I am guessing through auto-discovery mechanism)?

🤔

Should be perhaps push the if up here? In VFS, we can assert that neither path is simultaneously in the inclusion and exclusion list.

In the project-discovery code, we can explicitly exclude the excluded directories.


Unrelated, but, due to a bug in GitHub, it is best not to include @-mentions in the commit messages. GitHub incorrectly sends a email-notification every time such a commit is pushed, including rebases and forks.

Pings in PR descriptions are totally fine and encouraged!

@ChayimFriedman2
Copy link
Contributor Author

To make sure I understand correctly, what happens here is that the same path ends up in exclude (explicitly configured by the user) and also in the include (I am guessing through auto-discovery mechanism)?

Yes, exactly.

Should be perhaps push the if up here? In VFS, we can assert that neither path is simultaneously in the inclusion and exclusion list.

I don't think, I think it's fine as it's now.

Unrelated, but, due to a bug in GitHub, it is best not to include @-mentions in the commit messages. GitHub incorrectly sends a email-notification every time such a commit is pushed, including rebases and forks.

Didn't know, sorry! I edited the commit message.

I have no idea what the original writer of the code thought but the logic just seems backwards. We should not exclude a file/directory if it is equal to an include! This also meant that we had to add a `root == path` check so this stuff will actually work, which in turn meant excludes (of root files) no longer worked...

Also rename if to `rust-analyzer.files.exclude`, because it can exclude files as well.
@ChayimFriedman2 ChayimFriedman2 marked this pull request as ready for review February 4, 2025 15:54
@ChayimFriedman2
Copy link
Contributor Author

Okay this is now ready for review.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Unfortunate that we have to encode this specially in the VFS (and then effectively handle in every request handler), but it does make sense as to why (and I can't think of a different way to do this either)

This require a pretty big modification, because this is a new kind of file: exists - but ignore it.
@futile
Copy link

futile commented Feb 11, 2025

Ping, kind of waiting for this (I think it'll solve my "Roots scanned" issue), and wanted to ask when it can/will be merged? thanks! :)

@Veykril
Copy link
Member

Veykril commented Feb 11, 2025

Forgot about this, should be good to merge :)

@Veykril Veykril added this pull request to the merge queue Feb 11, 2025
@futile
Copy link

futile commented Feb 11, 2025

Awesome, thank you! 🚀

Merged via the queue into rust-lang:master with commit 78e7515 Feb 11, 2025
9 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the exclude branch February 11, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-analyzer.files.excludeDirs is (still) not working
5 participants