repair Ord for UseSegment

[ajewellamz]

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]

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

[ajewellamz]

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]

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]

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]

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

[ytmimi]

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]

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]

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]

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

[ajewellamz]

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]

// 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]

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

[ytmimi]

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.

[ajewellamz]

Done.

[ytmimi]

Thanks for working on this!