Fix some of the rustfmt fallout in Miri #67833

RalfJung · 2020-01-03T12:38:18Z

I tried to coerce rustfmt into reasonable formatting of matches, but that didn't always work...

RalfJung · 2020-01-03T12:41:11Z

src/librustc_mir/interpret/cast.rs

-                    src.layout.ty
-                )
-            }
+            _ => assert!(


It is really disappointing that I cannot stop rustfmt from doing silly things like this. :( Cc rust-lang/rustfmt#4004

src/librustc_mir/interpret/memory.rs

Centril · 2020-01-03T15:32:26Z

src/librustc/mir/interpret/error.rs

@@ -438,139 +438,165 @@ pub enum UnsupportedOpInfo<'tcx> {
 impl fmt::Debug for UnsupportedOpInfo<'tcx> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        use UnsupportedOpInfo::*;
+        #[rustfmt::skip] // rustfmt and long matches do not go well together


Imo the diff doesn't seem like that much of an improvement to justify this... I'm concerned about people starting to use this in various places throughout the compiler to fit their personal tastes rather than having a uniform style.

I tend to agree with @Centril here that I would like to avoid #[rustfmt::skip]. I'd rather find ways to structure the code that make rustfmt happy, or else open issues on rustfmt for extreme cases.

(Tweaking rustfmt.toml seems ok too, though I'd prefer to avoid even that when we can.)

Uh... for me this diff is the difference between "unreadable garbage" and "reasonable code". Like, without this PR, the write! is formatted in one of three different ways depending on the length of the message and the phase of the moon. Sometimes it's entirely in the same line as the pattern, sometimes it is in a line on its own, and sometimes the write! is on the line of the pattern but its arguments are in separate lines. On top of that, some arms have curly braces and some don't, even though all just contain a single write!. How is that not just bad style? The style should be consistent across all arms of this match. But what rustfmt does is inconsistent, it's hard to parse visually. The entire point of rustfmt, I thought, is to get consistent formatting, but clearly it is doing the opposite here (and for matches in general). The code is very symmetric, but visually in rustfmt style, there's no symmetry at all.

I opened a rustfmt issue at rust-lang/rustfmt#3995. But until that gets fixed, I don't see any good reason to let a tool's deficiency (rustfmt overall works fairly poorly on match arms) dictate the format.

Also Cc @Mark-Simulacrum, whose suggestion it was to use rustfmt::skip on this file.

it's just useful to be able to format-on-save anywhere

I appreciate that's useful for folks who went through the hoops of setting that up. Even as someone who didn't, I appreciate the consistent formatting of function signatures, the sorted imports, stuff like that.

But when considering matches like this, I honestly don't understand how you can call this a "uniform reading experience" -- there's nothing uniform about how rustfmt formats this match. As far as I am concerned, the formatting of this match after rustfmt is literally worse than anything I ever saw anywhere in the rustc codebase before rustfmt. The way it formats matches will make it much harder for me to work on and review this code-base, due to how it degrades readability. I appreciate that is subjective, but the part where it is very inconsistent and using different styles to do the exact same thing is objectively neither uniform nor consistent.

I think the style in this diff is by far egregious enough to justify rustfmt::skip. I also don't get the principled opposition to that: we don't usually make ourselves slaves to our own tools, do we? Tidy has had exceptions that were and are applied where reasonable. Not allowing reasonable exceptions would be quite unprecedented. In fact, I count around 30 occurrences of rustfmt::skip in the rustc repo already (excluding tools and tests). Many of them are for some sort of table or another (but in some sense, this match is a table), but this one is not. And that makes sense -- rustfmt is a great tool, but like all tools it has its limitations. We should apply the tool where it makes sense, but not mindlessly submit ourselves to whatever the tool happens to do.

I'll just note that sometimes (sometimes) I find let val; ...; val = expr more readable.

(Not when its mutable state; that's almost never more readable. I'm talking about lazily initialized state here.)

Why do I find it more readable? Well, sometimes the `let val = { ... }; ends up with something where the result expression is so far from the binding that it doesn't fit in my editor's view at once, which means I've effectively lost the relationship (or at least have to keep in in my mental cache).

Why do I find it more readable? Well, sometimes the `let val = { ... }; ends up with something where the result expression is so far from the binding that it doesn't fit in my editor's view at once, which means I've effectively lost the relationship (or at least have to keep in in my mental cache).

Split your functions Felix, so that "doesn't fit" doesn't happen :P

Split your functions Felix, so that "doesn't fit" doesn't happen :P

Sure, when that's an option, it can work well. And I suppose you might also suggest that I ensure that the enums I process never have more than four or five variants.

but sometimes you have to play the hand you are dealt, and I'm glad that I have let x; ... x = ...; as a tool in my back pocket, even you all aren't willing to use it yourself.

And I suppose you might also suggest that I ensure that the enums I process never have more than four or five variants.

Not really, but you can split match arms into separate functions when the match gets too long, e.g. as in https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_typeck/check/expr.rs.html#213-289 or https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_typeck/check/pat.rs.html#119-219 which were unreadable messes before I split those functions apart.

src/librustc_mir/interpret/place.rs

src/librustc_mir/interpret/validity.rs

RalfJung · 2020-01-07T11:59:09Z

Another match that got badly hit is this: there are multiple match arms that have patterns like A | B | C, but some of them get formatted with one pattern per line, while others get formatted with all patterns on the same line. Visually scanning for a particular arm becomes impossible.

RalfJung · 2020-01-08T10:46:16Z

I guess ultimately this is up to the compiler team, so maybe we should nominate? The higher-level point here is to come up with some kind of a policy for when rustfmt::skip is acceptable. Also see the discussion at #67833 (comment).

I just picked that one match as I think it is the most obvious and worst case of match-formatting gone wrong. But also see this commit of rustfmt edits that decrease match consistency (except for the last two), so if this was my codebase I'd not accept a PR landing such code; and this commit manually formatting two more matches.

Mark-Simulacrum · 2020-01-08T12:04:33Z

I would personally be in favor of landing the skips as a stopgap and seeing what we can do to upstream fixes into rustfmt, even if those are gated behind some option.

In particular, I find that I much prefer to enable people to contribute and we should have a policy that skip can be applied anywhere but should be tagged with a rustfmt issue associated with it. If it becomes more widespread, we can move to trying to limit the cases, but my hope is that it touches 1% of the code and we can mostly not care.

pnkfelix · 2020-01-13T14:46:01Z

Hey @RalfJung , @nikomatsakis and I were musing that if rustfmt had a config option that told it to not rewrite the brace-styling on match arms (i.e. don't replace PAT => { EXPR } with PAT => EXPR,) then that might address the resulting formatting issues you have here, as long as you were willing to write your own code here to use braces. (But you would not, I believe, need to add assignment statements to your code in the manner I outlined earlier.)

Would that be a reasonable way to address this, from your point of view, if it does work?

RalfJung · 2020-01-13T14:53:48Z

It would certainly be a big improvement, yes. :)

I was hoping to also lobby for match_arm_blocks = false to make rustfmt less aggressive about adding braces, not sure how these options would end up behaving together.

pnkfelix · 2020-01-13T16:32:26Z

@RalfJung oh, right; my intent was that the config flag would apply in both directions. I.e., if the flag is set, then rustfmt should err on the side of leaving the brace-style as is. So if they're present, leave them there, and if they're absent, don't insert them.

oli-obk · 2020-01-16T16:00:58Z

Discussed at the compiler team meeting. There isn't much opposition to #[rustfmt::skip] addings, but also noone really in favour. So we won't be accepting this PR to prevent churn. Changes to rustfmt.toml causing widespread changes would still be ok.

RalfJung · 2020-01-16T17:31:28Z

Well, that is disappointing, but I guess there is little I can do here. :(

I'll reopen the PR without the rustfmt::skip part -- and I will try to never open that error.rs file ever again so I do not have to bear witness to the mess there...

@oli-obk

Fix some of the rustfmt fallout in Miri re-post of rust-lang#67833 without the `rustfmt::skip` r? @oli-obk

RalfJung added 3 commits January 3, 2020 13:31

fix rustfmt fallout

0581605

let rustfmt undo most of my edits :(

ff5b102

manually format Miri error message matches

e44f3e7

rust-highfive assigned oli-obk Jan 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2020

RalfJung commented Jan 3, 2020

View reviewed changes

oli-obk reviewed Jan 3, 2020

View reviewed changes

src/librustc_mir/interpret/memory.rs Show resolved Hide resolved

Centril reviewed Jan 3, 2020

View reviewed changes

oli-obk reviewed Jan 3, 2020

View reviewed changes

src/librustc_mir/interpret/place.rs Show resolved Hide resolved

oli-obk reviewed Jan 3, 2020

View reviewed changes

src/librustc_mir/interpret/validity.rs Outdated Show resolved Hide resolved

adjust Deref comment

bc902c4

add FIXME for rustfmt::skip

7a4a08f

oli-obk approved these changes Jan 8, 2020

View reviewed changes

Centril added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 8, 2020

oli-obk removed the I-nominated label Jan 16, 2020

oli-obk closed this Jan 16, 2020

RalfJung mentioned this pull request Jan 16, 2020

Fix some of the rustfmt fallout in Miri #68288

Merged

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 17, 2020

Rollup merge of rust-lang#68288 - RalfJung:fmt, r=oli-obk

c411c22

Fix some of the rustfmt fallout in Miri re-post of rust-lang#67833 without the `rustfmt::skip` r? @oli-obk

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some of the rustfmt fallout in Miri #67833

Fix some of the rustfmt fallout in Miri #67833

RalfJung commented Jan 3, 2020

RalfJung Jan 3, 2020 •

edited

Loading

Centril Jan 3, 2020

nikomatsakis Jan 3, 2020

nikomatsakis Jan 3, 2020

RalfJung Jan 3, 2020 •

edited

Loading

RalfJung Jan 6, 2020

RalfJung Jan 8, 2020

pnkfelix Jan 9, 2020 •

edited

Loading

Centril Jan 9, 2020

pnkfelix Jan 16, 2020

Centril Jan 16, 2020

RalfJung commented Jan 7, 2020

RalfJung commented Jan 8, 2020 •

edited

Loading

Mark-Simulacrum commented Jan 8, 2020

pnkfelix commented Jan 13, 2020

RalfJung commented Jan 13, 2020

pnkfelix commented Jan 13, 2020

oli-obk commented Jan 16, 2020

RalfJung commented Jan 16, 2020 •

edited

Loading

Fix some of the rustfmt fallout in Miri #67833

Fix some of the rustfmt fallout in Miri #67833

Conversation

RalfJung commented Jan 3, 2020

RalfJung Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jan 7, 2020

RalfJung commented Jan 8, 2020 • edited Loading

Mark-Simulacrum commented Jan 8, 2020

pnkfelix commented Jan 13, 2020

RalfJung commented Jan 13, 2020

pnkfelix commented Jan 13, 2020

oli-obk commented Jan 16, 2020

RalfJung commented Jan 16, 2020 • edited Loading

RalfJung Jan 3, 2020 •

edited

Loading

RalfJung Jan 3, 2020 •

edited

Loading

pnkfelix Jan 9, 2020 •

edited

Loading

RalfJung commented Jan 8, 2020 •

edited

Loading

RalfJung commented Jan 16, 2020 •

edited

Loading