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

Tweak if let suggestion to be more liberal with suggestion and to not ICE #77283

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

estebank
Copy link
Contributor

Fix #77218. Fix #77238.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Sep 28, 2020
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2020
@camelid camelid added the C-bug Category: This is a bug. label Oct 3, 2020
@Dylan-DPC-zz
Copy link

r? @Mark-Simulacrum

Applicability::MaybeIncorrect,
);
}
let (mut msg, applicability, eq) = if self.can_coerce(rhs_ty, lhs_ty) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, so the ordering switched from left/right to right/left here, which (IIRC) with coercion is quite significant. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the expression we want to take as "ground truth" to detect if foo = bar when if let foo = bar was meant is the rhs.

@estebank estebank added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 20, 2020
@estebank
Copy link
Contributor Author

The ICE is in the beta now.

@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

Of course once approved and merged into master.

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 22, 2020
@Mark-Simulacrum
Copy link
Member

Niko indicated that the flip in coercion order was likely fine during the last meeting -- @estebank, would you be up for squashing the commits here a bit? Ideally I'd prefer to avoid 4 commits for such a small and dense change :)

r=me though.

@estebank
Copy link
Contributor Author

@Mark-Simulacrum done.

@Mark-Simulacrum
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Oct 25, 2020

📌 Commit cabf6d0 has been approved by Mark-Simulacrum

@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 Oct 25, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 26, 2020
…crum

Tweak `if let` suggestion to be more liberal with suggestion and to not ICE

Fix rust-lang#77218. Fix rust-lang#77238.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 26, 2020
…crum

Tweak `if let` suggestion to be more liberal with suggestion and to not ICE

Fix rust-lang#77218. Fix rust-lang#77238.
@bors
Copy link
Contributor

bors commented Oct 26, 2020

⌛ Testing commit cabf6d0 with merge 16e9ed0...

@bors
Copy link
Contributor

bors commented Oct 26, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 16e9ed0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2020
@bors bors merged commit 16e9ed0 into rust-lang:master Oct 26, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 26, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 5, 2020
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.49.0, 1.48.0 Nov 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2020
…ulacrum

[beta] backports

This backports a number of PRs to beta:

* Add delay_span_bug to no longer ICE rust-lang#78645
* Do not ICE on invalid input rust-lang#78422
* revert rust-lang#75443, update mir validator rust-lang#78410
* Do not try to report on closures to avoid ICE rust-lang#78268
* Disable "optimization to avoid load of address" in InstCombine rust-lang#78195
* Disable MatchBranchSimplification rust-lang#78151
* Do not ICE with TraitPredicates containing [type error] rust-lang#77930
* Tweak `if let` suggestion to be more liberal with suggestion and to not ICE rust-lang#77283
* Use different mirror for linux headers in musl-toolchain CI script. rust-lang#78316
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If `@petrochenkov` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to `@varkor` who helped with the implementation, particularly around the struct rest changes.

r? `@petrochenkov`
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If ``@petrochenkov`` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to ``@varkor`` who helped with the implementation, particularly around the struct rest changes.

r? ``@petrochenkov``
@fanzier
Copy link
Contributor

fanzier commented Nov 13, 2020

The diagnostics implemented here regressed slightly due to my PR #78836. In the file src/test/ui/issues/issue-77218.rs,

if Some(3) = foo {}

is lowered to

if { let Some(_x) = foo; 3 = _x } {}

Hence the code of this PR doesn't recognize it as something where == or if let should be suggested.

I'm not sure how to best fix this, so pinging @estebank in case he wants to look into this.

@estebank
Copy link
Contributor Author

@fanzier thank you for the heads up :(

flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If ``@petrochenkov`` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to ``@varkor`` who helped with the implementation, particularly around the struct rest changes.

r? ``@petrochenkov``
@estebank estebank deleted the if-let-sugg branch November 9, 2023 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` beta-accepted Accepted for backporting to the compiler in the beta channel. C-bug Category: This is a bug. 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