Skip to content

cargo fmt breaks pyo3 #2824

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
konstin opened this issue Jul 3, 2018 · 8 comments
Closed

cargo fmt breaks pyo3 #2824

konstin opened this issue Jul 3, 2018 · 8 comments
Labels
bug Panic, non-idempotency, invalid code, etc.

Comments

@konstin
Copy link

konstin commented Jul 3, 2018

rusfmt breaks pyo3 in the sense that it doesn't build after running cargo fmt.

Reproducing:

git clone https://github.com/pyo3/pyo3
cd pyo3
git checkout 68c14a5707dd92ceef7619c0a5444cefcc78a0f6 # Recent, but fixed commit
cargo build # Fine
cargo fmt
cargo build # Broken

The last command results in a conflicting implementations error, even though only the formatting should have changed:

error[E0119]: conflicting implementations of trait `python::IntoPyDictPointer` for type `()`:
   --> src/objects/dict.rs:247:1
    |
247 | / impl<K, V, I> IntoPyDictPointer for I
248 | | where
249 | |     K: ToPyObject,
250 | |     V: ToPyObject,
...   |
260 | |     }
261 | | }
    | |_^ conflicting implementation for `()`
    | 
   ::: src/noargs.rs:61:1
    |
61  |   impl IntoPyDictPointer for () {
    |   ----------------------------- first implementation here
    |
    = note: upstream crates may add new impl of trait `std::iter::Iterator` for type `()` in future versions

Versions:

  • cargo fmt --version: rustfmt 0.8.2-nightly (87edd75 2018-06-22)
  • rustc --version: rustc 1.28.0-nightly (e3bf634 2018-06-28)
@nrc nrc added the bug Panic, non-idempotency, invalid code, etc. label Jul 3, 2018
@nrc
Copy link
Member

nrc commented Jul 3, 2018

@konstin do you have the diff for the changes rustfmt made please? Even better would be if you could isolate any parts of it which look suspicious - i.e., where Rustfmt has changed more than just whitespace.

@konstin
Copy link
Author

konstin commented Jul 3, 2018

diff

The diff for the src alone is 154 files changed, 7786 insertions(+), 5578 deletions(-) and I couldn't find anything skimming through the cahnges. I also tried only formatting just the two files rustc complained about (rustfmt src/objects/dict.rs src/noargs.rs), but that didn't break the build.

@Rantanen
Copy link
Contributor

Rantanen commented Jul 4, 2018

Got curious about this so did some research into it.

This feels like a rustc bug. The offending file is src/objects/mod.rs and the cause is the reordering of the mod statements at the bottom.

After cargo fmt the mod import list is in the following order (highlights mine):

mod boolobject;
mod bytearray;
mod dict;  // <- dict
pub mod exc;
mod floatob;
mod iterator;
mod list;
mod module;  // <- module
mod sequence;
mod set;
mod slice;
mod stringdata;
mod stringutils;
mod tuple;
mod typeobject;  // <- typeobject

This causes the compilation error. I could fix this in two ways.

  • Raise the highlighted lines on top of all the imports and order them as typeobject, module and dict.
  • Drop the highlighted lines to the bottom of all the imports and order them as typeobject, dict and module.

Also once the crate has been built successfully, it will continue to compile just fine even with cargo fmt restoring the "broken" order. The build will fail only after deleting target/debug/incremental/pyo3* and target/debug/deps/pyo3*.

Also looking at the error message. That is an impl for I where I: IntoIterator<Item = (K, V)>. How is that a conflicting implementation for type () - is there some "empty tuple into any iterator" impl somewhere?

@nrc
Copy link
Member

nrc commented Jul 4, 2018

Thanks @Rantanen! I'll follow up with the compiler team.

@nikomatsakis
Copy link

The error message looks correct to me -- although there does not exist an impl of IntoIterator for (), the coherence system isn't allowed to assume we won't add one in the future. The inconsistent behavior around mod ordering, however, definitely does not sound correct!

It sounds like there is a bug around incremental compilation, to start.

Can you try something for me? If you have one of the configurations that does not error, and you run cargo clean and then cargo build, do you get errors?

@nikomatsakis
Copy link

Regardless, seems like something we should file an issue on rustc.

@nikomatsakis
Copy link

(Never mind my question, I have reproduced locally.)

@konstin konstin closed this as completed Jul 4, 2018
@konstin konstin reopened this Jul 4, 2018
@konstin
Copy link
Author

konstin commented Jul 30, 2018

Fixed through rust-lang/rust#52050

@konstin konstin closed this as completed Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

No branches or pull requests

4 participants