Skip to content

WIP: Don't create self imports with merge_imports #3563

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

Conversation

jrobsonchase
Copy link
Contributor

Currently, rustfmt formats this:

use a::b;
use a::b::c;

into this:

use a::b::{self, c};

This changes the semantics of the imports since use a::b; could also be pulling in a function or macro named b, while use a::b::{self, c}; only imports the module named b.

Just have a failing test case for now, but I should have a fix before too long.

@jrobsonchase jrobsonchase force-pushed the dont_merge_imports_self branch from 37c0bd9 to f7e3d7c Compare May 18, 2019 21:01
@scampi
Copy link
Contributor

scampi commented May 23, 2019

I was able to reproduce what you described, thanks. Below is what I used

$ tree src
src
├── main.rs
├── moda
│   └── modb.rs
└── moda.rs

1 directory, 3 files

$ cat src/**/*.rs
     File: src/main.rs
   1 pub mod moda;
   2
   3 //use moda::modb;
   4 use moda::modb::{self, fnc};
   5
   6 fn main() {
   7     modb();
   8     fnc();
   9 }

     File: src/moda/modb.rs
   1 pub fn fnc() {
   2     println!("in fnc");
   3 }

     File: src/moda.rs
   1 pub mod modb;
   2
   3 pub fn modb() {
   4     println!("in modb function");
   5 }

This fails to compile:

error[E0423]: expected function, found module `modb`
 --> src/main.rs:6:5
  |
6 |     modb();
  |     ^^^^ not a function
help: possible better candidate is found in another module, you can import it into scope
  |
3 | use crate::moda::modb;
  |

Your build is failing https://travis-ci.com/rust-lang/rustfmt/jobs/201273582:

Mismatch at tests/source/merge_imports.rs:1:
 // rustfmt-merge_imports: true
 
-use a::{a, b, c, d, e, f, g};
+use a::{
+    a, e, g, {b, b}, {c, c}, {d, d}, {f, f},
+};
 
 #[doc(hidden)]
 use a::b;
Mismatch at tests/source/merge_imports.rs:8:
 
 #[doc(hidden)]
 use a::b;
-use a::{c, d, e};
+use a::{
+    c, e, {d, d},
+};
 
 use foo::{a, b, c};
 pub use foo::{bar, foobar};
Mismatch at tests/source/merge_imports.rs:15:
 
 use a::b::c::{d, xxx, yyy, zzz, *};
 
-use a::b;
-use a::b::c;
+use a::{b, b::c};
 
Mismatch at tests/source/configs/imports_layout/merge_mixed.rs:4:
 
 use std::{
     fmt, io,
-    str::{self, FromStr},
+    {str, str::FromStr},
 };
 
Ran 367 system tests.
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `0`: 2 system tests failed', src/test/mod.rs:153:9
test test::system_tests ... FAILED
failures:
---- imports::test::test_use_tree_merge stdout ----
thread 'imports::test::test_use_tree_merge' panicked at 'assertion failed: `(left == right)`
  left: `[a::{b, b}]`,
 right: `[a::b]`', src/imports.rs:994:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
---- test::idempotence_tests stdout ----
thread 'test::idempotence_tests' panicked at 'Failed to join a test thread: Any', src/libcore/result.rs:999:5
---- test::system_tests stdout ----
thread 'test::system_tests' panicked at 'Failed to join a test thread: Any', src/libcore/result.rs:999:5
failures:
    imports::test::test_use_tree_merge
    test::idempotence_tests
    test::system_tests

@jrobsonchase
Copy link
Contributor Author

Yeah, my fix isn't quite there yet, and right now I fear that it might take a more extensive rewrite of the merging algorithm than I'm up for 😕

The last mismatch is a false positive, the second is actually the correct behavior and I just missed the test case. The others are all a result of my half-fix that leaves duplicate imports.

@topecongiro
Copy link
Contributor

The issue seems to be fixed by #3753. As such, I am closing this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants