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

repair Ord for UseSegment #6375

Merged
merged 4 commits into from
Nov 4, 2024
Merged

repair Ord for UseSegment #6375

merged 4 commits into from
Nov 4, 2024

Conversation

ajewellamz
Copy link
Contributor

@ajewellamz ajewellamz commented Oct 24, 2024

Fixes #6333

Ord for UseSegment was not Transitive, causing user-provided comparison function does not correctly implement a total order panic.

e.g. for

let A = DafnyType
let B = _System
let C = truncate

A < B
B < C
but
A > C

@ytmimi
Copy link
Contributor

ytmimi commented Oct 24, 2024

@ajewellamz Thanks for looking into this! Can you briefly explain how your fix addresses the issue?

@ajewellamz
Copy link
Contributor Author

In the example above, the problem was that (A, C) was compared with the special case logic, but (A,B) and (B,C) were not. This is why the comparison operator was not transitive.

That is, segments that began with a character that was neither lower nor upper fell into an inconsistent gray area.

Said another way, A was CamelCase when compared to C but snake_case when compared to B.

Switching from is_lower to !is_upper changes that, A is always CamelCase, regardless of what it's being compared to.

@ytmimi
Copy link
Contributor

ytmimi commented Oct 28, 2024

So it was mostly an issue with special characters like _ that are neither uppercase nor lowercase? It doesn't look like any of the existing test cases changed, but I'm wondering if this change has the potential to change the current sort behavior?

@ajewellamz
Copy link
Contributor Author

If all segments start with a letter, then there is no change.

If there is a segment that does not start with a letter, then the current behavior is indeterminate, and often results in a panic! (because Ord is not transitive) so we need a change.

Yes, it has the potential to change the current sort behavior, but with the current code, running rustfmt in a slightly different context also has potential to change the current sort behavior.

@ajewellamz
Copy link
Contributor Author

So.... is there something more I can do?
Our build and deployment processes are kinda blocked, because rustfmt crashes.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 2, 2024

Yes, it has the potential to change the current sort behavior, but with the current code, running rustfmt in a slightly different context also has potential to change the current sort behavior.

Could you elaborate on which cases would potentially be changed? Also, what do you mean when you say running rustfmt in different contexts could change things?

So.... is there something more I can do?

Adding a test case would be great. Especially if you can think of cases where the new sort behavior differs from what we used to have. You can add a new test case by creating a .rs file to tests/target.

@ajewellamz
Copy link
Contributor Author

If there are segments that don't begin with a letter, the current behavior is undefined, because the comparator is not transitive. This is what I mean by "could change", and why rustfmt will panic on some input, like that in #6333

I haven't found a case where rustfmt produces unexpected results but doesn't crash.

I've added tests/target/issue-6333.rs, but I might be confused about something, because cargo test passes but cargo run --bin rustfmt -- --check tests/target/issue-6333.rs fails.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 2, 2024

You may need to add a config comment to the top of your test file to specify style_edition=2015 like // rustfmt-style_edition: 2015. In rustfmt's own rustfmt.toml file it's configured to uses the newer and still unstable style_edition=2024 by default, which I don't think has the same transitive issues.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 2, 2024

We changed the sort order in style_edition=2024 to match the new sorting rules established by the style guide.

@ajewellamz
Copy link
Contributor Author

Even with
// rustfmt-style_edition: 2015
at the top of the file, I can't get cargo test and cargo run --bin rustfmt to agree.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 2, 2024

// rustfmt-style_edition: 2015 will only work in the context of rustfmt tests. Outside of the test suite it's treated as a normal comment.

Can you try running cargo run --bin rustfmt -- --config=style_edition=2015

@ajewellamz
Copy link
Contributor Author

Thanks. With --config=style_edition=2015, cargo test and cargo run --bin rustfmt agree.

@@ -0,0 +1,22 @@
pub use crate::r#_StructUtil_Compile::CanonCryptoItem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be useful to add the // rustfmt-style_edition: 2015 comment to the start of the file. Also, it might be useful to include a separate test for // rustfmt-style_edition: 2024.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@ytmimi ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-not-reviewed labels Nov 4, 2024
@ytmimi ytmimi merged commit 777e25a into rust-lang:master Nov 4, 2024
26 checks passed
@ajewellamz ajewellamz mentioned this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo fmt crashes in Rust 1.81.0
3 participants