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: avoid hint items shadow local item from completions #18517

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CubeSugarCheese
Copy link

@CubeSugarCheese CubeSugarCheese commented Nov 16, 2024

Relevant Issues

#18398 Don't auto import items when one is already in scope
#17164 Block specific items from being auto-imported

Fixed Problem

Now completions will not hint item that could shadow local item.

Problems that May Arise

Unexpected behavior

Maybe we can auto rename or use long path to disambiguation, and add a option to control the behavior.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2024
@ChayimFriedman2
Copy link
Contributor

This makes the search N*M, I'd be much happier with a hash set.

@CubeSugarCheese
Copy link
Author

CubeSugarCheese commented Nov 16, 2024

This makes the search N*M, I'd be much happier with a hash set.

finish.
During refactoring, I notice that Rustc will treat prelude items and built in types different than normal items.
For prelude, allow shadow by use statement. Like std::result::Result::Ok shadow by use anyhow::Ok. This is not what we want.
For built in types, allow same name item. Like str and core::str, it also allow coexist with user defined item. This feature must support.
Finally, I choose to forbid same items and ignore all built in types. This rule may need to be improved.

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.

I feel like this should be the responsibility of the caller (doing the filtering), also this needs to be aware of namespaces, we do want to show importing completions for imports that bring a name into scope into a namespace that is currently "empty". (think of importing the Deserialize trait while the derive is already in scope)

@@ -251,7 +251,6 @@ impl ImportAssets {
*to_import = NameToImport::Exact(name, case_sensitive);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

restore this

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2024
@bors
Copy link
Contributor

bors commented Dec 16, 2024

☔ The latest upstream changes (presumably #18699) made this pull request unmergeable. Please resolve the merge conflicts.

@CubeSugarCheese CubeSugarCheese marked this pull request as draft December 22, 2024 13:39
@CubeSugarCheese
Copy link
Author

Apologies for the late reply. I've realized that the current approach is a bit too simplistic and rough. The specific rules and details still need further discussion. In light of that, I’ve decided to mark this PR as a draft so we can continue to discuss and refine the approach further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants