Skip to content

Incorrect span for unused import when nested #46576

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

Closed
Badel2 opened this issue Dec 8, 2017 · 6 comments
Closed

Incorrect span for unused import when nested #46576

Badel2 opened this issue Dec 8, 2017 · 6 comments
Assignees
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@Badel2
Copy link
Contributor

Badel2 commented Dec 8, 2017

Consider this import:

use std::io::{BufRead, BufReader, Read};

When BufRead is not used in the code, but BufReader and Read are used, one would expect this error:

warning: unused import: `BufRead`
 --> src/main.rs:3:15
  |
3 | use std::io::{BufRead, BufReader, Read};
  |               ^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

Which is what happens on stable. On nighly, however, that does not happen, sometimes we got this error instead:

warning: unused import: `std::io::{BufRead, BufReader, Read}`
 --> src/main.rs:3:5
  |
3 | use std::io::{BufRead, BufReader, Read};
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

But sometimes it shows the correct error, and sometimes it shows both errors.

Playground link with minimal example
If you comment out the first function it shows both errors, and if you comment out the second function it shows the stable error.

@shssoichiro
Copy link
Contributor

Confirmed here on 2017-12-17 nightly.

@arielb1 arielb1 added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Dec 19, 2017
@Badel2
Copy link
Contributor Author

Badel2 commented Dec 20, 2017

Run a bisect on this: 91ba8b4 is the first bad commit

@shssoichiro
Copy link
Contributor

I'll take a shot at fixing this one. Thanks for bisecting the commit @Badel2!

@arielb1
Copy link
Contributor

arielb1 commented Dec 21, 2017

Hi @petrochenkov - could you try to help @shssoichiro debug this? This looks pretty bad.

@petrochenkov petrochenkov self-assigned this Dec 23, 2017
@petrochenkov
Copy link
Contributor

Slightly minimized:

use std::fs::File;
use std::io::{BufRead, BufReader, Read};

pub fn read_from_file(path: &str) {
    let file = File::open(&path).unwrap();
    let mut reader = BufReader::new(file);
    let mut s = String::new();
    reader.read_to_string(&mut s).unwrap();
    
    // comment out this line to solve bug
    s.lines();
}

fn main() {}

This is a fun issue. "Unused import" warnings are reported either during name resolution (normally) or during type checking (for traits that are potentially implicitly used through method calls).

Trait BufRead has method lines, so having any call to a method named lines in code shifts the "unused import" check from resolve to typeck.
All "unused import" warnings reported during typeck have bad spans because spans of imports in HIR are set to spans of whole import trees during lowering from AST to HIR, they need to use spans of final path segments instead.
@shssoichiro The span that needs to be changed is here

span: use_tree.span,

@shssoichiro
Copy link
Contributor

Thanks! I'm attempting to test a fix, however the compiler seems to be taking forever to compile stage1 since I rebased off of master. I'll let everyone know how it goes when (if?) compilation ever finishes.

shssoichiro pushed a commit to shssoichiro/rust that referenced this issue Dec 26, 2017
Solves incorrect diagnostics for unused or deprecated imports. Closes rust-lang#46576.

Deprecated imports had an existing test which asserted the incorrect span.
That test has been corrected as part of this commit.
bors added a commit that referenced this issue Dec 28, 2017
…tebank

Pass correct span when lowering grouped imports

Solves incorrect diagnostics for unused or deprecated imports. Closes #46576.

Deprecated imports had an existing test which asserted the incorrect span.
That test has been corrected as part of this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

4 participants