-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add new lint: std_wildcard_imports
#14868
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
base: master
Are you sure you want to change the base?
Conversation
3a0a721 to
5f8b8e3
Compare
std_wildcard_importsstd_wildcard_imports
This comment has been minimized.
This comment has been minimized.
5f8b8e3 to
80ac147
Compare
llogiq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some duplication with the wildcard_imports lint. It probably makes sense to pull those lint passes together into one pass that implements both lints.
Otherwise I'm dubious about making this a warn-by-default lint, but we can discuss this during the final comment period.
| /// Wildcard imports can pollute the namespace. This is especially bad when importing from the | ||
| /// standard library through wildcards: | ||
| /// | ||
| /// ```no_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs into the Example section.
| /// ``` | ||
| #[clippy::version = "1.89.0"] | ||
| pub STD_WILDCARD_IMPORTS, | ||
| style, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the lint group. Yes, namespace pollution is bad. But there are four stdlib namespaces (of which three might be imported from in somewhat normal code), so this could lead to serious churn.
I also don't follow the argument that names imported from std are somehow worse than others. Why? The standard library moves slower than other crates if anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry for late reply) Then how about setting the lint group to pedantic, and denying this lint in Cargo.toml of standard libraries? I think it would solve the issue without editing a bunch of existing codes outside of stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some context, the libs team expressed interest in having some form of this builtin at some point rust-lang/rust#135672 (comment). But we likely need a smarter version for that - some discussion at rust-lang/rust#142448.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussions in the linked issues, I suggest we close this PR, clearly define what this lint should do, and then implement it accordingly. What do you think of this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I still think this is useful in its current state, just with some updated docs per @jieyouxu's comment. We probably eventually want some version of this as a rustc builtin lint and that will be a bit smarter, but having something in Clippy (regardless of whatever level the maintainers feel comfortable placing it at) definitely helps get us off the ground.
| cx, | ||
| STD_WILDCARD_IMPORTS, | ||
| span, | ||
| "usage of wildcard import from `std` crates", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be shorter and clearer to replace the "std crates" with the actual crate name.
|
☔ The latest upstream changes (possibly 84ef7fb) made this pull request unmergeable. Please resolve the merge conflicts. |
| /// Wildcard imports can pollute the namespace. This is especially bad when importing from the | ||
| /// standard library through wildcards: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: as far as I understand it, the motivation of #13961 is mostly not about namespace pollution, but about name resolution breakage (such as introduction of new ambiguities) from people glob-importing std modules, and then std introducing a new item whose name collides with that of an item defined locally. The very bad part about this situation is that even unstable std API editions can introduce name resolution ambiguities.
That is, the strongest motivation for this lint is the name resolution ambiguity related breakages, and not namespace pollution.
80ac147 to
de87094
Compare
de87094 to
4c4c928
Compare
|
@rustbot ready |
| segments.iter().any(|seg| allowed_segments.contains(seg.ident.as_str())) | ||
| } | ||
|
|
||
| // Returns the entire span for a given glob import statement, including the `*` symbol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Returns the entire span for a given glob import statement, including the `*` symbol. | |
| /// Returns the entire span for a given glob import statement, including the `*` symbol. |
| } | ||
| } | ||
|
|
||
| // Generates a suggestion for a glob import using only the actually used items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Generates a suggestion for a glob import using only the actually used items. | |
| /// Generates a suggestion for a glob import using only the actually used items. |
|
|
||
| struct X; | ||
| use std::collections::*; | ||
| use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine the suggestion was quite long in this case. Because of that, I think it would make sense to give a Diag::span_suggestion_verbose (inside span_lint_and_then)
| } else if is_std_import(use_path.segments) { | ||
| ( | ||
| STD_WILDCARD_IMPORTS, | ||
| format!("usage of wildcard import from `{}`", use_path.segments[0].ident), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could make these into Cows to avoid allocating a String in 2 out of 3 cases
| /// bar(); | ||
| /// let _ = Rc::new(5); | ||
| /// ``` | ||
| #[clippy::version = "1.89.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bit of churn^^
| #[clippy::version = "1.89.0"] | |
| #[clippy::version = "1.92.0"] |
changelog: [
std_wildcard_imports]: Initial implementationThis fixes #13961.
Please let me know if there any missing test cases and/or changes!
TODO