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

Regression in the Debug formatter #47619

Closed
pietroalbini opened this issue Jan 20, 2018 · 12 comments
Closed

Regression in the Debug formatter #47619

pietroalbini opened this issue Jan 20, 2018 · 12 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@pietroalbini
Copy link
Member

pietroalbini commented Jan 20, 2018

The Debug formatter output was recently changed in #30967 to always include a decimal point, and apparently some crates' test suites depends on the exact output of the formatter. I don't know if it's worth to revert the change, but I reported it just in case.

cc @dtolnay @Luthaf @rodolf0

@pietroalbini pietroalbini added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jan 20, 2018
@pietroalbini pietroalbini added this to the 1.24 milestone Jan 20, 2018
@pietroalbini
Copy link
Member Author

euclid and expecttest are also affected (but euclid now it's fixed on master).

cc @zummenix

@sfackler
Copy link
Member

We make zero guarantees as to what Debug implementations look like.

@Luthaf
Copy link

Luthaf commented Jan 21, 2018

I'm fine with this, I'll change my tests =)

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 21, 2018
@zummenix
Copy link

I'll change my tests too. @pietroalbini thanks for cc me!

@pietroalbini
Copy link
Member Author

pietroalbini commented Jan 21, 2018

mkv is also affected. cc @vi

@pietroalbini
Copy link
Member Author

postgis is also affected. cc @andelf @pka
pretty_assertions is also affected. cc @colin-kiegel
scanlex is also affected. cc @stevedonovan

colin-kiegel added a commit to rust-pretty-assertions/rust-pretty-assertions that referenced this issue Jan 21, 2018
@colin-kiegel
Copy link

Thanks for the heads up!

If you want to restore "consistent" behaviour across old and future rust versions, you can just replace {:?} with either {:.0?} or {:.1?}. This way you can still write testcases that depend on the exact debug output for floats:

print!("{:?}",   0.0); // stable: "0",   nightly "0.0"
print!("{:.0?}", 0.0); // stable: "0",   nightly "0"
print!("{:.1?}", 0.0); // stable: "0.0", nightly "0.0"

@vi
Copy link
Contributor

vi commented Jan 23, 2018

Shall there be a [clippy] lint against using Debug output for assert_eq!?

@pietroalbini
Copy link
Member Author

This also affects spectral (crater log). cc @cfrancia
This also affects vectors (crater log). cc @deepthought

@stevedonovan
Copy link

Yes, it's foolish to depend on exact Debug representations, and I will fix accordingly. There was a similar situation in Lua 5.3 when the default formating for floats changed - broke a lot of naive code.

@Mark-Simulacrum
Copy link
Member

Closing as not a bug; we do not expect to make any changes here.

pka added a commit to t-rex-tileserver/t-rex that referenced this issue Feb 8, 2018
@sunjay
Copy link
Member

sunjay commented Feb 20, 2018

Broke the tests in the turtle crate too. Any of our tests using should_panic broke because they were relying on the entire message which included a formatted struct.

#[test]
#[should_panic(expected = "Invalid color: Color { red: NaN, green: 0, blue: 0, alpha: 0 }. See the color module documentation for more information.")]
fn rejects_invalid_background_color() {
    // ...
}

Not a huge deal this time because there were just a few tests like that, but this really shouldn't be taken lightly either. Any breaking change, no matter how small for you can be really frustrating for someone else who doesn't see it coming.

Yes, it's foolish to depend on exact Debug representations, and I will fix accordingly. There was a similar situation in Lua 5.3 when the default formating for floats changed - broke a lot of naive code.

In this case, for should_panic, we only get to pass in a naive string, so of course changes like this will break a test like that. We don't even get to set formatting options so any of the workarounds suggested will not work. I'm including the debug representation in what I am testing because I want to make sure that its values match what I expect. Changing is a big deal and it broke my code in a way I really couldn't have prevented.

We make zero guarantees as to what Debug implementations look like.

I'm fine with this, because it makes sense to me too, but as I said earlier I don't think any breaking change should be taken too lightly just because no explicit guarantees have been made. It's clear from this issue that a lot of people were relying on that being a certain way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants