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

bug: imports_granularity = "Module"/"Crate"/"One" crash code when de-duplicate dependencies #5131

Closed
Sunt-ing opened this issue Dec 11, 2021 · 8 comments · Fixed by #5209
Closed
Labels
a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce

Comments

@Sunt-ing
Copy link

Sunt-ing commented Dec 11, 2021

Recently, I enabled a rustfmt rule:

imports_granularity = "Module"

After I re-fmt, the code failed to be compiled. And thus I found an unexpected rustfmt behaviour:

before fmt:

use risingwave_pb::data::DataType;
use risingwave_pb::data::DataType as DataTypeProst;

after fmt:

use risingwave_pb::data::DataType as DataTypeProst;

But the codes that use "DataType" are not changed.

I don't think that rustfmt should turn the correct code into incorrect in any condition.

@TennyZhuang
Copy link

Can you provide a reproducible example?

@Sunt-ing
Copy link
Author

Can you provide a reproducible example?

Ok, will do it soon.

@Sunt-ing
Copy link
Author

Can you provide a reproducible example?

I created a repo to reproduce this bug: https://github.com/Sunt-ing/rustfmt_bug_example

@skyzh
Copy link

skyzh commented Dec 11, 2021

Can you provide a reproducible example?

I created a repo to reproduce this bug: https://github.com/Sunt-ing/rustfmt_bug_example

Consider creating a single file for this issue, which is easier to test and reproduce. e.g.,

https://github.com/rust-lang/rustfmt/blob/master/tests/source/imports_granularity_crate.rs
https://github.com/rust-lang/rustfmt/blob/master/tests/target/imports_granularity_crate.rs

@Sunt-ing
Copy link
Author

@skyzh Good advice. I will try to do that.

@Sunt-ing
Copy link
Author

@skyzh Since the bug occurs when imports_granularity = "Module", I think it's hard to reproduce it within a single file. I'd like to leave it to the one who wants to fix it after the bug is verified. He or she could create such a file as a test.

@TennyZhuang
Copy link

@skyzh Since the bug occurs when imports_granularity = "Module", I think it's hard to reproduce it within a single file. I'd like to leave it to the one who wants to fix it after the bug is verified. He or she could create such a file as a test.

In fact, rustfmt always accept a single file as input, and cargo fmt call rustfmt on all files. So it should always reproduce on a single file.

@Sunt-ing
Copy link
Author

Sunt-ing commented Dec 11, 2021

OK, I created one. This code snippet will crash if you format it with any one of the following options:

  • imports_granularity = "Module"
  • imports_granularity = "One"
  • imports_granularity = "Crate"

Yes, imports_granularity = "Item" will not crash it.

#![allow(dead_code)]
mod a {
    pub mod b {
        pub struct Data {
            pub a: i32,
        }
    }

    use crate::a::b::Data;
    use crate::a::b::Data as Data2;

    pub fn data(a: i32) -> Data {
        Data { a }
    }

    pub fn data2(a: i32) -> Data2 {
        Data2 { a }
    }

    #[cfg(test)]
    mod tests {
        use super::*;

        #[test]
        pub fn test() {
            data(1);
            data2(1);
        }
    }
}

@Sunt-ing Sunt-ing changed the title bug: import rules crash code when de-duplicate dependencies bug: imports_granularity = "Module"/"Crate"/"One" crash code when de-duplicate dependencies Dec 11, 2021
@ytmimi ytmimi added only-with-option requires a non-default option value to reproduce a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. labels Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants