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

[single_component_path_imports] to include super:: and crate::; … #7190

Closed
wants to merge 2 commits into from

Conversation

mautamu
Copy link

@mautamu mautamu commented May 8, 2021

Howdy,

This is my attempt to fix #7168.

I added a tracker variable to [single_component_path_imports] for what I'm referring to as "root paths," i.e. crate::* and super::*. With this, we add another check to filter out those prior to emitting warnings.

Let me know what could be done better!

Best,
Mautamu

r? @djc @camsteffen
fixes: #7168
changelog: Don't trigger [single_component_path_imports] on an import if crate::import or super::import are invoked.

@camsteffen
Copy link
Contributor

r? @camsteffen

@giraffate giraffate added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 8, 2021
@camsteffen
Copy link
Contributor

Thanks for contributing to clippy!

We should make sure we don't introduce false negatives where super refers to something at a different level. If I'm not mistaken, we need a more robust solution to detect theses cases (and we should have tests for some negative cases).

use x; // this should lint

mod a {
    use x; // this should not lint
	mod b {
	      fn f() {
	          super::x::..;
	      }
	}
}

Also I'm concerned about performance with collecting uses for the entire crate in a Vec and then using Vec::contains.

This is a little difficult for a first-time contribution, but you are certainly welcome to stick to it!

Here is one idea.
  1. Add imports: Vec<FxHashMap<Symbol, Span>> to SingleComponentPathImports
  2. When you visit use x, add it to imports.last()
  3. When you visit a nested mod (in check_item if ItemKind::Mod) push a new set onto imports
  4. When you visit super::x::.. remove x from imports[imports.len() - number_of_supers - 1]
  5. At the end of a module (check_item_post), imports.pop() and lint every element in the set.

Perhaps all of the Vecs currently being used could be removed and just use imports. But not required.

@bors
Copy link
Contributor

bors commented May 21, 2021

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

@giraffate
Copy link
Contributor

ping from triage @mautamu. Can you have any updates on this? If you have any questions, feel free to ask us!

@mautamu
Copy link
Author

mautamu commented Jun 7, 2021

ping from triage @mautamu. Can you have any updates on this? If you have any questions, feel free to ask us!

Oh thanks for checking back! Sorry about the delay, I’ll see what I can get done tomorrow.

@giraffate
Copy link
Contributor

ping from triage @mautamu. Can you have any updates on this?

@giraffate
Copy link
Contributor

ping from triage @mautamu. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this Jul 8, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FP single_component_path_imports
4 participants