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

Bump rustfmt version #80843

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Bump rustfmt version #80843

merged 1 commit into from
Feb 2, 2021

Conversation

Mark-Simulacrum
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2021
| AnnotationType::MultilineEnd(_))
| AnnotationType::MultilineStart(_)
| AnnotationType::MultilineLine(_)
| AnnotationType::MultilineEnd(_)
Copy link
Contributor

Choose a reason for hiding this comment

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

@calebcartwright
This looks like a regression, | patterns outside of matches! are not formatted with an indent like this.

Copy link
Member

Choose a reason for hiding this comment

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

This one requires a much more thorough response. The short answer is that no it's not a regression, though you're not the first person to raise the question and I can certainly understand why.

For the longer answer, there's two core parts.

First, there was an issue in rustfmt where in cases with multi pats and at least one _ rustfmt was entirely unable to process the arg and thus wasn't applying any formatting at all against the mac call expr and just leaving the original formatting in place; rustfmt was just keeping whatever was here before as-is, not successfully applying the formatting rules.

An easy way to see this in practice is by running the older versions (I guess x.py fmt from master will work) with a wildly misformatted version of this, and you'll see that wild formatting remains untouched:

        matches!(self.annotation_type,AnnotationType::Multiline(_)
| AnnotationType::MultilineStart(_)                      | AnnotationType::MultilineLine(_)
              | AnnotationType::MultilineEnd(_)
        )

On the rustfmt side we've been particularly benefiting from the recent parser/tokens/etc. work that folks like yourself and Aaron have been working on, and now that we've pulled those rustc changes into rustfmt it's finally able to actually format these matches! calls producing the formatting seen here.

The second part relates to how we have to attempt to format macro call args in rustfmt. Obviously we're working with the calls in the AST pre-expansion, so when we encounter a maccall expression, we'll use the token streams to instantiate a parser and attempt to parse the args individually so that we can then apply the corresponding formatting rules when possible (e.g. an expr or type).

In the case of matches!, the tokens for that second arg actually ends up getting successfully parsed as a standard binop expression, and so rustfmt applies the corresponding formatting rules for binops which is why it gets indented this way.

Eventually I'd like to introduce some new special case process for matches! mac calls since we'll know definitively that second arg isn't actually a binop expr, and then be able to apply more pattern-like formatting rules instead.

Unfortunately though, for the foreseeable future rustfmt is going to continue to be formatting matches like any other mac call even though the emitted formatting is less than desireable

Type = Self,
DynExistential = Self,
Const = Self,
> + fmt::Write
Copy link
Contributor

Choose a reason for hiding this comment

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

@calebcartwright
Is this change intended? Looks like a regression.

AFAIK, the situation like

    HEADER BRACKET_OPEN
BRACKET_CLOSE

(with BRACKET_CLOSE being less indented than HEADER) is not supposed to happen in general.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping! This is definitely a bug that was introduced in v1.4.28, I'm guessing rust-lang/rustfmt#4474 but will dig into it

Copy link
Member

Choose a reason for hiding this comment

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

this has been fixed in source, should available within the next couple days

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
…k-Simulacrum

bump rustfmt to v1.4.32

Fixes an indentation bug with bounds reported in rust-lang#80843 (comment)

r? `@Mark-Simulacrum`
@Mark-Simulacrum
Copy link
Member Author

r? @petrochenkov

I've rebased with the latest rustfmt.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit 063b427 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2021
henryboisdequin added a commit to henryboisdequin/rust that referenced this pull request Jan 29, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 30, 2021
henryboisdequin pushed a commit to henryboisdequin/rust that referenced this pull request Feb 1, 2021
@bors
Copy link
Contributor

bors commented Feb 2, 2021

⌛ Testing commit 063b427 with merge 1f598ec78c7bee462b86200c62366c5ede08051c...

@bors
Copy link
Contributor

bors commented Feb 2, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 2, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@petrochenkov
Copy link
Contributor

Needs a rebase due to formatting changes in rustc_mir_build.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2021
Also switches on formatting of the mir build module
@Mark-Simulacrum
Copy link
Member Author

@bors r=petrochenkov p=1 (likely prone to bitrot)

@bors
Copy link
Contributor

bors commented Feb 2, 2021

📌 Commit d5b760b has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2021
@bors
Copy link
Contributor

bors commented Feb 2, 2021

⌛ Testing commit d5b760b with merge b81f581...

@bors
Copy link
Contributor

bors commented Feb 2, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b81f581 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2021
@bors bors merged commit b81f581 into rust-lang:master Feb 2, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants