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

Replace qualified path with use replaces with a different path #9537

Closed
jbr opened this issue Jul 8, 2021 · 3 comments · Fixed by #9551
Closed

Replace qualified path with use replaces with a different path #9537

jbr opened this issue Jul 8, 2021 · 3 comments · Fixed by #9551
Assignees
Labels
A-assists Broken Window Bugs / technical debt to be addressed immediately S-actionable Someone could pick this issue up and work on it right now

Comments

@jbr
Copy link
Contributor

jbr commented Jul 8, 2021

I tried to use "replace qualified path with use" on std::time::Duration and got use tokio::time::Duration;, which is a reexport of the same type. I've also run into this when I replace qualified path with use on std::future::Future and get another crate's reexport of Future. This isn't a major issue, but is surprising behavior that is not immediately obvious if the top of the file is not visible — the code compiles just fine, but is not what the author intended.

If for some obscure-to-me reason there's no way to use the exact qualified path that was typed (std::time in this case), it would be nice to special case std::* when picking the right use statement.

Happy to help resolve this with guidance. Thanks for rust-analyzer!

Edit: This is as of current HEAD, 80f193e

@Veykril
Copy link
Member

Veykril commented Jul 9, 2021

So what we are doing here currently is that we remove the qualifier of the path, then create an import path for the item we removed the qualifier for, which unfortunately finds the re-exports first in this case. Ideally we would search for an import of the first segment of the to-be-replaced path instead and then stick the found import for that and the to-be-replaced path together as an import for the item instead. This doesn't completely avoid the problem in all cases, but it should in the majority of them.

An example walk-through of the idea:
Given std::time::$0Duration($0 being the cursor) we would look for an import for std(the first segment of the path), giving us back just std in this case(this step is necessary for the import prefix logic to work even if a no-op in this case), then we merge the found import (std) and the to be replaced path without its leading segments (time::Duration) together and insert that import, giving us an import for (std::time::Duration). How simple this is to realize in the code base I'm not sure but I believe it shouldn't be too bad.

So basically the main reason for this "dance" would be so that we honor the import prefix settings still while trying to keep the path that was used to qualify the item, the rust-analyzer.assist.importPrefix setting.

So all of that should be done instead of simply calling this function here
https://github.com/rust-analyzer/rust-analyzer/blob/80f193e3f86e38e1a683f5557900740af855b333/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs#L50-L54

@jonas-schievink
Copy link
Contributor

Ah, I just hit this as well #9540

@jonas-schievink jonas-schievink added A-assists Broken Window Bugs / technical debt to be addressed immediately S-actionable Someone could pick this issue up and work on it right now labels Jul 9, 2021
@Veykril
Copy link
Member

Veykril commented Jul 9, 2021

RA really does not enjoy renamed item imports/exports...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists Broken Window Bugs / technical debt to be addressed immediately S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants