Add new lint: std_wildcard_imports
#14868
Conversation
alanbondarun
force-pushed
the
std_wildcard_imports
branch
4 times, most recently
from
3a0a721 to
5f8b8e3
Compare
rustbot
added
the
S-waiting-on-review
alanbondarun
changed the title
std_wildcard_importsstd_wildcard_imports
This comment has been minimized.
This comment has been minimized.
alanbondarun
force-pushed
the
std_wildcard_imports
branch
from
5f8b8e3 to
80ac147
Compare
There was a problem hiding this comment.
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.
This belongs into the Example section.
| /// ``` | ||
| #[clippy::version = "1.89.0"] | ||
| pub STD_WILDCARD_IMPORTS, | ||
| style, |
There was a problem hiding this comment.
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.
(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.
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.
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.
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.
It would be shorter and clearer to replace the "std crates" with the actual crate name.
rustbot
added
S-waiting-on-author
|
☔ 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.
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.
alanbondarun
force-pushed
the
std_wildcard_imports
branch
from
80ac147 to
de87094
Compare
alanbondarun
force-pushed
the
std_wildcard_imports
branch
from
de87094 to
4c4c928
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
changelog: [
std_wildcard_imports]: Initial implementationThis fixes #13961.
Please let me know if there any missing test cases and/or changes!
TODO