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 autoimport does nothing when importing trait that is as _ imports #15587

Merged
merged 2 commits into from Sep 22, 2023

Conversation

dfireBird
Copy link
Contributor

Potentially fixes #15128

There are two cases of imports:

  1. With simple path
  2. With use tree list (or say complex path).

On deeper inspection, the recursive_merge function (called by try_merge_trees_mut) is meaningful only in the case of complex path (i.e when the UseTree contains a UseTreeList).

The recursive_merge function has match with Ok arm, that is only executed when both LHS and RHS has PathSegment with same NameRef. The removal of underscore is implemented in this arm in the case of complex path.

For simple paths, the underscore is removed by checking if both LHS and RHS are simple paths and if their Path is same (the check is done here) and remove the underscore if one is found (I made an assumption here that RHS will always be what rust-analyzer suggests to import, because at this point I'm not sure how to remove underscore with help of ted::replace).

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2023
@dfireBird
Copy link
Contributor Author

++ @Veykril I described the logic as descriptive as possible. I hope it's understandable and you could relate it with the diff. Please give any feedback if you have any?

@Veykril
Copy link
Member

Veykril commented Sep 11, 2023

Can you add a test for this here -> https://github.com/dfireBird/rust-analyzer/blob/fix-15128/crates/ide-db/src/imports/insert_use/tests.rs?

Or ideally two, one for each of those changes

@dfireBird
Copy link
Contributor Author

dfireBird commented Sep 11, 2023

Regarding tests, I forgot to mention. But this only happens when the rust-analyzer.imports.prefix is set to "self", but for these tests the setting was set to "plain". I didn't see a way to override it for just single test or two. So I was not sure how to proceed with tests.

P.S: I actually have the tests in my local tree with modified config such rust-analyzer.imports.prefix is set "self" instead of "plain". It breaks other tests in the module (not all but some) but works on my tests

@Veykril
Copy link
Member

Veykril commented Sep 11, 2023

There is a check_with_config function that lets you pass a corresponding config to test with, see the insert_not_group test for example (there you can set the prefix kind)

@dfireBird
Copy link
Contributor Author

I was adding the tests in tests module of https://github.com/dfireBird/rust-analyzer/blob/fix-15128/crates/ide-assists/src/handlers/auto_import.rs . That's why I couldn't find the check_with_config. I didn't realize the tests should be added to the module you noted. Sorry, my bad.

@Veykril
Copy link
Member

Veykril commented Sep 22, 2023

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Sep 22, 2023

📌 Commit df1239b has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 22, 2023

⌛ Testing commit df1239b with merge 5855bd8...

@bors
Copy link
Collaborator

bors commented Sep 22, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 5855bd8 to master...

@bors bors merged commit 5855bd8 into rust-lang:master Sep 22, 2023
10 checks passed
@dfireBird dfireBird deleted the fix-15128 branch December 5, 2023 16:18
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.

autoimport does nothing when importing trait that is as _ imports
4 participants