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

Lint for redundant imports #58805

Merged
merged 18 commits into from
Mar 31, 2019
Merged

Lint for redundant imports #58805

merged 18 commits into from
Mar 31, 2019

Conversation

fabric-and-ink
Copy link
Contributor

@fabric-and-ink fabric-and-ink commented Feb 28, 2019

Add lint for redundant imports. The changes are suggested by @petrochenkov.

Closes #10178.

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2019
@petrochenkov
Copy link
Contributor

r? @petrochenkov

Mark::root() means that we are not in a macro, so the correct condition is

if directive.parent_scope.expansion != Mark::root() {
    return;
}

@rust-highfive rust-highfive assigned petrochenkov and unassigned varkor Feb 28, 2019
@fabric-and-ink
Copy link
Contributor Author

With that change I get a different error:

error: redundant import                                                                                                     
  --> src/libstd/rt.rs:24:9
   |
24 |     use panic;
   |         ^^^^^
   |
note: the other one
  --> src/libstd/macros.rs:58:1
   |
58 | / macro_rules! panic {
59 | |     () => ({
60 | |         panic!("explicit panic")
61 | |     });
...  |
71 | |     });
72 | | }
   | |_^

error: aborting due to previous error

error: Could not compile `std`.

@petrochenkov
Copy link
Contributor

@fabric-and-ink
That's because the error only needs to be reported if the import is redundant in all namespaces, rather than in any namespace like this PR is currently doing.

@fabric-and-ink
Copy link
Contributor Author

I updated the check. Now it seems that my test file compiles fine, without a redundancy error reported.

@petrochenkov
Copy link
Contributor

By "all namespaces" I mean all namespaces in which resolution exists, i.e. if source_bindings[ns] is None (or perhaps if its def() is Def::Err) then we shouldn't consider it.

(Right now the error is not reported if the import is unresolved in any of the namespaces.)

@fabric-and-ink
Copy link
Contributor Author

I updated the check. Now I get the false positive again. 😄

error: redundant import                                                                                                     
  --> src/libstd/rt.rs:24:9
   |
24 |     use panic;
   |         ^^^^^
   |
note: the other one
  --> src/libstd/macros.rs:58:1
   |
58 | / macro_rules! panic {
59 | |     () => ({
60 | |         panic!("explicit panic")
61 | |     });
...  |
71 | |     });
72 | | }
   | |_^

error: aborting due to previous error

error: Could not compile `std`.

@petrochenkov
Copy link
Contributor

I don't see an obvious error, try to use logging to see what happens in each individual namespace.

@petrochenkov petrochenkov 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 Feb 28, 2019
@fabric-and-ink
Copy link
Contributor Author

Have some trouble getting helpful logs. I will try a few things and will get back to you if they don't seem to work out.

@fabric-and-ink fabric-and-ink changed the title WIP: Lint for redundant imports Lint for redundant imports Mar 10, 2019
@fabric-and-ink
Copy link
Contributor Author

fabric-and-ink commented Mar 10, 2019

Ok, I think this is more or less ready to go :) Too early...

@fabric-and-ink
Copy link
Contributor Author

Strange... why don't I see these locally?

@fabric-and-ink
Copy link
Contributor Author

fabric-and-ink commented Mar 10, 2019

Ok, compilation should be fine now. But I see a few errors in the ui test suite. I could add #[allow(redundant_imports)] to the failing tests. What do you think?

@Centril
Copy link
Contributor

Centril commented Mar 16, 2019

@Mark-Simulacrum Once this is implemented wdyt about integrating into tidy?

@petrochenkov
Copy link
Contributor

@Centril
What do you mean? Enabling this warning in bootstrap?
It's already enabled by default currently.

@Centril
Copy link
Contributor

Centril commented Mar 16, 2019

@petrochenkov Oh, we reject all warnings? That should do it then, cool.

@petrochenkov
Copy link
Contributor

@Centril
Yes

if env::var_os("RUSTC_DENY_WARNINGS").is_some() && env::var_os("RUSTC_EXTERNAL_TOOL").is_none()
{
cmd.arg("-Dwarnings");
cmd.arg("-Dbare_trait_objects");
}

@bors
Copy link
Contributor

bors commented Mar 31, 2019

📌 Commit c1d5314 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2019
@petrochenkov
Copy link
Contributor

@bors p=1

@bors
Copy link
Contributor

bors commented Mar 31, 2019

⌛ Testing commit c1d5314 with merge cc77f27ad5253e79b795b0c48aafba0daf0b7d96...

Centril added a commit to Centril/rust that referenced this pull request Mar 31, 2019
…petrochenkov

Lint for redundant imports

Add lint for redundant imports. The changes are suggested by @petrochenkov.

Closes rust-lang#10178.
@Centril
Copy link
Contributor

Centril commented Mar 31, 2019

@bors retry

Yielding to rollup that this PR is included in.

bors added a commit that referenced this pull request Mar 31, 2019
Rollup of 7 pull requests

Successful merges:

 - #58805 (Lint for redundant imports)
 - #59506 (Use platform dependent mcount function)
 - #59519 (rustc_target: factor out common fields of non-Single Variants.)
 - #59580 (Allow closure to unsafe fn coercion)
 - #59581 (Stabilize refcell_replace_swap feature)
 - #59583 (match match match match match)
 - #59587 (Remove #[doc(hidden)] from Error::type_id)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Mar 31, 2019

⌛ Testing commit c1d5314 with merge 4fac5c9...

@bors bors merged commit c1d5314 into rust-lang:master Mar 31, 2019
@petrochenkov
Copy link
Contributor

@fabric-and-ink
Congrats.
This certainly wasn't an easy fix.

@fabric-and-ink
Copy link
Contributor Author

fabric-and-ink commented Mar 31, 2019

@petrochenkov thanks for helping me! That was fun :) I will try to contribute more often.

phansch added a commit to phansch/rust-clippy that referenced this pull request Apr 1, 2019
flip1995 added a commit to rust-lang/rust-clippy that referenced this pull request Apr 1, 2019
bors added a commit to rust-lang/cargo that referenced this pull request Apr 1, 2019
Fix unused import warning.

2019-04-01 nightly introduced a new warning (presumably rust-lang/rust#58805).
flip1995 pushed a commit to rust-lang/rust-clippy that referenced this pull request Apr 1, 2019
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 9, 2019
Changes:
````
Remove force-host and explain no-prefer-dynamic
Escape a single quote in single_char_pattern hint
cargo fmt
Re-add tmp feature to compiletest
Remove libtest from deps
Re-allow clippy::identity_conversion in compiletest
Use latest compiletest-rs
Revert tests/compile-test.rs to 61aa5c9
Fix ICE in suspicious_else_formatting
use a multispan for MANY_SINGLE_CHAR_NAMES
Add missing `// run-pass` annotations to ICE tests
Remove clippy_dev as dev-dependency
NFC: fix typos
rustup rust-lang#59657
Add TransmutingNull Lint * Late Lint pass, catches:   * One liner: 0 -> null -> transmute   * One liner: std:null() -> transmute   * Const (which resolves to null) -> transmute * UI Test case for Lint * Updated test for issue 3849, because now the lint that code generated is in Clippy. * Expanded `const.rs` miri-based Constant Folding code, to cover   raw pointers
Run rustfmt
Set level of identity_conversion FP to warn
Rustup to rust-lang#58805
rustup 41316f0
Updated source to match with recent rustc `master` toolchain changes
Fix dogfood error of question_mark lint fix
Fix question_mark lint+test
use `span_lint_and_sugg` in `explicit_counter_loop`
Fix some test failures
Hacky rustup
run cargo fmt
rustup rust-lang#59096
Change explicit_counter_loop's message to add parentheses if necessary
Change explicit_counter_loop's message to reflect original variable name
cargo fmt
Add rustfix tests for mistyped_literal_suffix lint
Move some `unreadable_literal` ui tests to correct file
Add implementation for the EarlyLintPass trait into the Adding Lints documentation.
Add rust-toolchain for clippy_dev
````
@fabric-and-ink fabric-and-ink deleted the redundant_import branch April 11, 2019 16:42
@est31 est31 mentioned this pull request Apr 15, 2019
est31 added a commit to est31/glium that referenced this pull request Apr 15, 2019
glium reexports glutin and I think this
point needs to be stressed. So using glutin
from the reexport instead of the possibly
available extern crate is useful at least
in the examples otherwise users might not
realize that they don't have to add glutin
to their Cargo.toml.

Therefore, the lint is silenced instead of
being followed.

Link to the lint change that likely caused this:

rust-lang/rust#58805
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
Remove force-host and explain no-prefer-dynamic
Escape a single quote in single_char_pattern hint
cargo fmt
Re-add tmp feature to compiletest
Remove libtest from deps
Re-allow clippy::identity_conversion in compiletest
Use latest compiletest-rs
Revert tests/compile-test.rs to 61aa5c9
Fix ICE in suspicious_else_formatting
use a multispan for MANY_SINGLE_CHAR_NAMES
Add missing `// run-pass` annotations to ICE tests
Remove clippy_dev as dev-dependency
NFC: fix typos
rustup rust-lang/rust#59657
Add TransmutingNull Lint * Late Lint pass, catches:   * One liner: 0 -> null -> transmute   * One liner: std:null() -> transmute   * Const (which resolves to null) -> transmute * UI Test case for Lint * Updated test for issue 3849, because now the lint that code generated is in Clippy. * Expanded `const.rs` miri-based Constant Folding code, to cover   raw pointers
Run rustfmt
Set level of identity_conversion FP to warn
Rustup to rust-lang/rust#58805
rustup 41316f0
Updated source to match with recent rustc `master` toolchain changes
Fix dogfood error of question_mark lint fix
Fix question_mark lint+test
use `span_lint_and_sugg` in `explicit_counter_loop`
Fix some test failures
Hacky rustup
run cargo fmt
rustup rust-lang/rust#59096
Change explicit_counter_loop's message to add parentheses if necessary
Change explicit_counter_loop's message to reflect original variable name
cargo fmt
Add rustfix tests for mistyped_literal_suffix lint
Move some `unreadable_literal` ui tests to correct file
Add implementation for the EarlyLintPass trait into the Adding Lints documentation.
Add rust-toolchain for clippy_dev
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants