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

sync subtree #5944

Merged
Merged

Conversation

calebcartwright
Copy link
Member

@ytmimi I'll be intermittently away from my machine today so if you're around it'd be helpful if you could keep an eye on status and ping me/merge as necessary

(though remember that for sync PRs we never squash nor rebase the merge commits)

WaffleLapkin and others added 29 commits June 19, 2023 12:54
Syntactically accept `become` expressions (explicit tail calls experiment)

This adds `ast::ExprKind::Become`, implements parsing and properly gates the feature.

cc `@scottmcm`
Implement rust-lang/compiler-team#578.

When an ICE is encountered on nightly releases, the new rustc panic
handler will also write the contents of the backtrace to disk. If any
`delay_span_bug`s are encountered, their backtrace is also added to the
file. The platform and rustc version will also be collected.
Currently, Clippy, Miri, Rustfmt, and rustc all use an environment variable to
indicate that output should be blessed, but they use different variable names.
In order to improve consistency, this patch applies the following changes:

- Emit `RUSTC_BLESS` within `prepare_cargo_test` so it is always
  available
- Change usage of `MIRI_BLESS` in the Miri subtree to use `RUSTC_BLESS`
- Change usage of `BLESS` in the Clippy subtree to `RUSTC_BLESS`
- Change usage of `BLESS` in the Rustfmt subtree to `RUSTC_BLESS`
- Adjust the blessable test in `rustc_errors` to use this same
  convention
- Update documentation where applicable

Any tools that uses `RUSTC_BLESS` should check that it is set to any value
other than `"0"`.
Token tree cloning is only needed in one place.
…henkov

Less `TokenTree` cloning

`TokenTreeCursor` has this comment on it:
```
// FIXME: Many uses of this can be replaced with by-reference iterator to avoid clones.
```
This PR completes that FIXME. It doesn't have much perf effect, but at least we now know that.

r? `@petrochenkov`
discard default auto trait impls if explicit ones exist (rebase of #85048)

Rebase of #85048
It's the same as `Delimiter`, minus the `Invisible` variant. I'm
generally in favour of using types to make impossible states
unrepresentable, but this one feels very low-value, and the conversions
between the two types are annoying and confusing.

Look at the change in `src/tools/rustfmt/src/expr.rs` for an example:
the old code converted from `MacDelimiter` to `Delimiter` and back
again, for no good reason. This suggests the author was confused about
the types.
Suggests turbofish in patterns

Fixes #114112

r? ```@estebank```
Indexing is similar to method calls in having an arbitrary
left-hand-side and then something on the right, which is the main part
of the expression. Method calls already have a span for that right part,
but indexing does not. This means that long method chains that use
indexing have really bad spans, especially when the indexing panics and
that span in coverted into a panic location.

This does the same thing as method calls for the AST and HIR, storing an
extra span which is then put into the `fn_span` field in THIR.
Lots of tiny incremental simplifications of `EmitterWriter` internals

ignore the first commit, it's rust-lang/rust#114088 squashed and rebased, but it's needed to use to use `derive_setters`, as they need a newer `syn` version.

Then this PR starts out with removing many arguments that are almost always defaulted to `None` or `false` and replace them with builder methods that can set these fields in the few cases that want to set them.

After that it's one commit after the other that removes or merges things until everything becomes some very simple trait objects
Improve spans for indexing expressions

fixes #114388

Indexing is similar to method calls in having an arbitrary left-hand-side and then something on the right, which is the main part of the expression. Method calls already have a span for that right part, but indexing does not. This means that long method chains that use indexing have really bad spans, especially when the indexing panics and that span in coverted into a panic location.

This does the same thing as method calls for the AST and HIR, storing an extra span which is then put into the `fn_span` field in THIR.

r? compiler-errors
Rollup of 9 pull requests

Successful merges:

 - #113945 (Fix wrong span for trait selection failure error reporting)
 - #114351 ([rustc_span][perf] Remove unnecessary string joins and allocs.)
 - #114418 (bump parking_lot to 0.12)
 - #114434 (Improve spans for indexing expressions)
 - #114450 (Fix ICE failed to get layout for ReferencesError)
 - #114461 (Fix unwrap on None)
 - #114462 (interpret: add mplace_to_ref helper method)
 - #114472 (Reword `confusable_idents` lint)
 - #114477 (Account for `Rc` and `Arc` when suggesting to clone)

r? `@ghost`
`@rustbot` modify labels: rollup
Anonymous structs or unions are only allowed in struct field
definitions.

Co-authored-by: carbotaniuman <[email protected]>
Comment on lines +84 to 89
<<<<<<< HEAD
[#5690]: (https://github.com/rust-lang/rustfmt/pulls/5690)
=======
[#5690]: https://github.com/rust-lang/rustfmt/pull/5690
>>>>>>> upstream/master
[#5684]: https://github.com/rust-lang/rustfmt/issues/5684
Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh 🤮

I clearly got tired of merge conflicts. Mercifully this is just noise in the changelog, so i'll clean this up later when I add the additional entries

Copy link
Contributor

@ytmimi ytmimi Oct 22, 2023

Choose a reason for hiding this comment

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

🤣 I know you mentioned that there were a lot of conflicts. No worries on this!

Choose a reason for hiding this comment

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

Cleaning up orphan processes

@calebcartwright
Copy link
Member Author

It seems that non-&& operators fail during parsing now as opposed to ast validation stage, so we'll need to update the let chains tests accordingly

Comment on lines +16 to +21
if aaaaaaaaaaaaaaaaaaaaa
&& aaaaaaaaaaaaaaa
&& aaaaaaaaa
&& let Some(x) = xxxxxxxxxxxx
&& aaaaaaa
&& let None = aaaaaaaaaa
Copy link
Member Author

Choose a reason for hiding this comment

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

I do have to step away now for a while, but wanted to flag this for your attention. Given that all the operators are now the same this does appear to be the correct formatting to my mind, do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is the formatting that I would have expected

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

Just for good measure I'm running the Diff Check Job

@calebcartwright
Copy link
Member Author

Just for good measure I'm running the Diff Check Job

Good, I'd been meaning to suggest we do so just to get a feel for the let chain results. Obviously we should be expecting some level of diff in just about every repo that has let chains in the code

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

I actually don't think there will be a change since let-chain support has already been merged, and we compare results between running the latest rustfmt from source and this PRs version of rustfmt. As long as both rustfmt binaries produce the same diff then we consider that a pass. That being said, I can run the diff check version from #5827 and inspect the diffs in the zipfile since we'll still capture those.

@calebcartwright
Copy link
Member Author

I'm going to go ahead and merge this then. If you have bandwidth now/within the next couple hours to do the version bump and changelog that'd also be a big help 🙏 (a single commit that adds the new entries to the changelog + another pass to fix some typos, and then does a patch version bump in the cargo.toml + lockfile files would do it)

@calebcartwright calebcartwright merged commit 0bb2acf into rust-lang:master Oct 22, 2023
27 checks passed
@calebcartwright calebcartwright deleted the subtree-sync-2023-10-22 branch October 22, 2023 19:08
@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

hmmm I guess 🤔 I spoke too soon: https://github.com/rust-lang/rustfmt/actions/runs/6605606043/job/17940945214

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

Going to look into this

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

@calebcartwright Okay, I think I know why the diff-check job failed, and I'm happy to say that it was a false positive. When I ran things locally, everything checked out 🙏🏼

You can inspect the zip if you'd like.

diff-check.zip

Here's the explanation for what went wrong.

When we're doing the subtree sync we're in an interesting scenario where rustfmt at origin/master and rustfmt from the feature branch (in this case subtree-sync-2023-10-22) need different LD_LIBRARY_PATHs to be set since the subtree sync modifies the rust-toolchain file.

I think the reason we see diffs in the output is because the binary we built from subtree-sync-2023-10-22 failed to run at all, and we silence those issues with set +e:

rustfmt/ci/check_diff.sh

Lines 149 to 152 in 0bb2acf

# rustfmt --check returns 1 if a diff was found
# Also check_diff returns 1 if there was a diff between master rustfmt and the feature branch
# so we want to ignore the exit status check
set +e

I couldn't re-run the CI job directly since you merged and deleted the subtree-sync-2023-10-22 branch, so I needed to work around that when running the script locally. Here are the steps you could take to reproduce the successful diff-check run:

  1. created a subtree-sync-2023-10-22 branch in my fork of the repo where the HEAD is in line with origin/master (0bb2acf)
  2. checked out ytmimi:diff-check-export
  3. applied this patch to modify how we set LD_LIBRARY_PATH and hard code 547577f as the master rustfmt:
    diff --git a/ci/check_diff.sh b/ci/check_diff.sh
    index b6ce62c2..c5ea2792 100755
    --- a/ci/check_diff.sh
    +++ b/ci/check_diff.sh
    @@ -2,9 +2,6 @@
    
     set -e
    
    -# https://github.com/rust-lang/rustfmt/issues/5675
    -export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH
    -
    function print_usage() {
         echo "usage check_diff REMOTE_REPO FEATURE_BRANCH [COMMIT_HASH] [OPTIONAL_RUSTFMT_CONFIGS]"
    }
    @@ -127,6 +124,13 @@ function compile_rustfmt() {
         echo -e "\ncompiling with $CARGO_VERSON\n"
    
         echo "Building rustfmt from src"
    +    # rustfmt commit before the latest subtree sync (https://github.com/rust-lang/rustfmt/pull/5944)
    +    echo "checking out commit hash 547577fa5d309d90292ca3a58fef1bf0d9325cc0"
    +    git fetch origin 547577fa5d309d90292ca3a58fef1bf0d9325cc0
    +    git checkout --detach 547577fa5d309d90292ca3a58fef1bf0d9325cc0
    +    # Add the LD_LIBRARY_PATH for master rustfmt
    +    export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH
    +    echo "LD_LIBRARY_PATH: $LD_LIBRARY_PATH"
         cargo build -q --release --bin rustfmt && cp target/release/rustfmt $1/rustfmt
    
         if [ -z "$OPTIONAL_COMMIT_HASH" ] || [ "$FEATURE_BRANCH" = "$OPTIONAL_COMMIT_HASH" ]; then
    @@ -136,6 +140,11 @@ function compile_rustfmt() {
         fi
    
         echo "Building feature rustfmt from src"
    +    # Add the `LD_LIBRARY_PATH` for feature rustfmt. In most cases the `LD_LIBRARY_PATH` should be the same,
    +    # for both binaries, however during subtree syncs we bump the `channel` in the rust-toolchain file, and
    +    # therefore the feature branch needs to use a different sysroot.
    +    export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib:$LD_LIBRARY_PATH
    +    echo "LD_LIBRARY_PATH: $LD_LIBRARY_PATH"
         cargo build -q --release --bin rustfmt && cp target/release/rustfmt $1/feature_rustfmt
    
         RUSFMT_BIN=$1/rustfmt
  4. run the script:
    ./ci/check_diff.sh https://github.com/ytmimi/rustfmt.git subtree-sync-2023-10-22 subtree-sync-2023-10-22
    

@calebcartwright
Copy link
Member Author

calebcartwright commented Oct 22, 2023

~~Would it help if I restored the branch? ~~

Nevermind, it's been too long and GitHub no longer allows me to restore. I really appreciate how deeply you dug into this.

I am still puzzled though why the sysroot location produces different formatting results? Or is it more that one of the two points of comparison didn't have any rustfmt execution (e.g. original_unformatted_input vs. output_from_actually_formatting_input)

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2023

I don't think restoring the branch would have made much of a difference anyway.

The issue was that only the master rustfmt binary ran successfully. When it ran it produced diffs, but the feature branch binary failed to run because of the runtime dependency on a different sysroot so no diffs were produced, and the diff-check script saw that discrepancy and marked the job as a failure.

I'll open up a PR with a similar patch to avoid these issues for future sub tree syncs moving forward.

@yorch2221
Copy link

2023-10-22T18:34:15.3750500Z Cleaning up orphan processes

@yorch2221
Copy link

@ytmimi I'll be intermittently away from my machine today so if you're around it'd be helpful if you could keep an eye on status and ping me/merge as necessary

(though remember that for sync PRs we never squash nor rebase the merge commits)

2023-10-22T18:34:15.3750500Z Cleaning up orphan processes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.