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

Match ergonomics 2024: implement mutable by-reference bindings #123080

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

Jules-Bertholet
Copy link
Contributor

Implements the mutable by-reference bindings portion of match ergonomics 2024 (#123076), with the mut ref/mut ref mut syntax, under feature gate mut_ref.

r? @Nadrieril

@rustbot label A-patterns A-edition-2024

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2024

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in match lowering

cc @Nadrieril

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-edition-2024 Area: The 2024 edition A-patterns Relating to patterns and pattern matching labels Mar 26, 2024
@Nadrieril
Copy link
Member

Nadrieril commented Mar 26, 2024

From a process perspective, we don't have an accepted RFC for this so I think we need a lang team liaison. Either that or write a precise proposal that can be approved (the original proposal was positively received but didn't get proposer "we agree to do this" kind of approval)

Copy link
Member

@Nadrieril Nadrieril left a comment

Choose a reason for hiding this comment

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

First of all, thanks for doing this work!

This PR is doing a bunch of different things at once; for future PRs I would have found it much easier to review a series of commits each doing a single change, e.g. the first commit would have changed how ByRef/BindingAnnotation work, a second one would have removed BindingMode etc and the last one would have added the new mut ref mut/mut ref possibility.

No need to change that now though, I've read through it. I like what you did with BindingAnnotation; this looks good. r=me once the feature gate is marked "incomplete" and we get a liaison.

| -
| |
| first assignment to `a`
| help: consider making this binding mutable: `mut a`
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice to extend this help message to suggest mut ref as well when the feature is active. I'lll let you decide whether to include in this PR or do it separately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it separately, I'd rather the BindingAnnotation/ByRef changes land first so later PRs can make use of them

compiler/rustc_feature/src/unstable.rs Outdated Show resolved Hide resolved
@Nadrieril Nadrieril added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2024
@bors
Copy link
Contributor

bors commented Mar 27, 2024

☔ The latest upstream changes (presumably #123121) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet
Copy link
Contributor Author

This should be ready to go, now that we have a liaison (#123076 (comment)).

@rustbot label -S-waiting-on-team S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 27, 2024
@Nadrieril
Copy link
Member

Let's goooo

@bors r+

@bors
Copy link
Contributor

bors commented Mar 27, 2024

📌 Commit e931595 has been approved by Nadrieril

It is now in the queue for this repository.

@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 Mar 27, 2024
@bors
Copy link
Contributor

bors commented Mar 28, 2024

⌛ Testing commit e931595 with merge 170598b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
…eril

Match ergonomics 2024: implement mutable by-reference bindings

Implements the mutable by-reference bindings portion of match ergonomics 2024 (rust-lang#123076), with the `mut ref`/`mut ref mut` syntax, under feature gate `mut_ref`.

r? `@Nadrieril`

`@rustbot` label A-patterns A-edition-2024
@bors
Copy link
Contributor

bors commented Mar 28, 2024

💔 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 Mar 28, 2024
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
-- Configuring done (29.2s)
-- Generating done (2.5s)
-- Build files have been written to: C:/a/rust/rust/build/x86_64-pc-windows-msvc/llvm/build
running: "cmake" "--build" "." "--target" "install" "--config" "Release" "--" "-j" "8"
ninja: error: build.ninja:3780: multiple outputs aren't (yet?) supported by depslog; bring this up on the mailing list if it affects you
thread 'main' panicked at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.48\src\lib.rs:975:5:

command did not execute successfully, got: exit code: 1

@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Mar 28, 2024

@Nadrieril
Copy link
Member

@bors retry

@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 Mar 28, 2024
@bors
Copy link
Contributor

bors commented Mar 29, 2024

⌛ Testing commit e931595 with merge 45796d1...

@bors
Copy link
Contributor

bors commented Mar 29, 2024

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 45796d1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2024
@bors bors merged commit 45796d1 into rust-lang:master Mar 29, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 29, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (45796d1): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.769s -> 668.043s (-0.11%)
Artifact size: 315.81 MiB -> 315.85 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-patterns Relating to patterns and pattern matching 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants