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.

Sign up for GitHub

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

Jump to bottom

Don't stop evaluating due to errors before borrow checking #60125

Merged
merged 3 commits into from
Apr 23, 2019
Merged

Don't stop evaluating due to errors before borrow checking #60125

merged 3 commits into from
Apr 23, 2019

Conversation

estebank
Copy link
Contributor

r? @oli-obk

Fix #60005. Follow up to #59903. Blocked on #53708, fixing the ICE in src/test/ui/consts/match_ice.rs.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2019
@estebank estebank changed the title Never stop due to errors before borrow checking Don't stop evaluating due to errors before borrow checking Apr 19, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
oli-obk
oli-obk reviewed Apr 20, 2019
View reviewed changes
src/librustc_mir/hair/pattern/mod.rs Show resolved Hide resolved
src/test/ui/error-codes/E0007.stderr Outdated
| ^^^^^^^^^^^^^^^^^-^
| | |
| | value moved here
| value used here after move
Copy link
Contributor

Choose a reason for hiding this comment

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

this span isn't quite right. I think it should point to op_string

src/test/ui/error-codes/E0301.stderr Outdated Show resolved Hide resolved
src/test/ui/error-codes/E0302.stderr Show resolved Hide resolved
src/test/ui/rfc-2005-default-binding-mode/for.stderr
@@ -6,6 +6,16 @@ LL | for (n, mut m) in &tups {
| |
| both by-ref and by-move used

error: aborting due to previous error
error[E0507]: cannot move out of borrowed content
Copy link
Contributor

Choose a reason for hiding this comment

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

this diagnostic is actually better than the first one.

@bors

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

@oli-obk It seems to me that with the change made in this PR, we can safely disable everything in check_match and no currently compiling code will stop compiling, and a subset of currently rejected code would now be accepted (as tested locally), which seems like it could be problematic because people could write code that compiles in latest stable but wouldn't compile in older rustc.

@estebank estebank mentioned this pull request Apr 22, 2019
Closed
oli-obk
oli-obk reviewed Apr 23, 2019
View reviewed changes
src/test/ui/borrowck/borrowck-mutate-in-guard.rs
@@ -9,9 +9,15 @@ fn foo() -> isize {
match x {
Enum::A(_) if { x = Enum::B(false); false } => 1,
//~^ ERROR cannot assign in a pattern guard
//~| WARN cannot assign `x` in match guard
//~| WARN this error has been downgraded to a warning for backwards compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

as a next step we should make this a deny-by-default lint.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2019

a subset of currently rejected code would now be accepted (as tested locally)
which seems like it could be problematic because people could write code that compiles in latest stable but wouldn't compile in older rustc.

That's fine. We do this all the time. I mean, every new syntactical feature is basically that.

we can safely disable everything in check_match

There's also code for the exhaustiveness checks in match checking, we can't remove that.

Let's merge this PR and then have a look at what effect killing the match borrow check duplication has (in a new PR)

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2019

📌 Commit 87ef96d has been approved by oli-obk

@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 Apr 23, 2019
@bors
Copy link
Contributor

bors commented Apr 23, 2019

⌛ Testing commit 87ef96d with merge 31f5d69...

bors added a commit that referenced this pull request Apr 23, 2019
@bors
Auto merge of #60125 - estebank:continue-evaluating, r=oli-obk
31f5d69 
Don't stop evaluating due to errors before borrow checking

r? @oli-obk

Fix #60005. Follow up to #59903. Blocked on #53708, fixing the ICE in `src/test/ui/consts/match_ice.rs`.
@bors
Copy link
Contributor

bors commented Apr 23, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 31f5d69 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2019
@bors bors merged commit 87ef96d into rust-lang:master Apr 23, 2019
@tesuji tesuji mentioned this pull request Oct 14, 2020
Closed
@estebank estebank deleted the continue-evaluating branch November 9, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove abort_if_errors before borrow checking
4 participants
@estebank @rust-highfive @bors @oli-obk