Skip to content

Prevent ICE with broken borrow in closure #108705

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
Merged
merged 1 commit into from
May 11, 2023
Merged

Prevent ICE with broken borrow in closure #108705

merged 1 commit into from
May 11, 2023

Conversation

@clubby789
Copy link
Contributor

r? @Nilstrieb
Fixes #108683

This solution isn't ideal, I'm hoping to find a way to continue compilation without ICEing.

@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. labels Mar 3, 2023
@Noratrieb
Copy link
Member

Yeah, I don't like this much. Ideally we'd just find a way to get typeck and MIR building to agree on this without anyone else involved.
But this is probably quite hard to fix in typeck, as typeck doesn't know about exhaustiveness, so getting it to emit a capture for "non-exhaustive" patterns sounds impossible.
MIR building is as usual a can of worms though.... and especially for this here. We'd have to reproduce the typeck logic in MIR building for this which is uhhhh.. yeah. Sounds hard and subtle, even just on a recovery path. Maybe this hack isn't so bad after all.

✨ future thoughts and fix ideas section✨
we really shouldn't be building MIR for broken bodies. that can only go wrong, I don't think this is the first time it went wrong. But just doing abort_if_errors before MIR building is also pretty bad (and impossible, some MIR for constants is built earlier after all!). Ideally we would have some way to return dummy MIR from MIR building when earlier passes failed, something that still keeps the compilation doing but doesn't produce nonsense errors or ICEs like here.

@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch 2 times, most recently from bd4e277 to ac39bbf Compare March 3, 2023 20:31
@clubby789
Copy link
Contributor Author

Here's another solution - walking the pattern to look for any literals, ranges or paths to constants which could be refutable, and inserting a borrow for them. Also pretty hacky, and probably a less complete version of maybe_read_scrutinee, but at least doesn't regress diagnostics by bailing out early

@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch 2 times, most recently from 5268cf3 to 3f607b6 Compare March 3, 2023 21:53
@clubby789
Copy link
Contributor Author

I'm a bit happier with this. If we fail to resolve the place, we just emit a goto with one of the possible target blocks to allow MIR to continue building, and a delay_span_bug to make sure we don't end up emitting this now-incorrect code.

@clubby789 clubby789 marked this pull request as ready for review March 4, 2023 16:01
@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from 3f607b6 to 95f6058 Compare March 7, 2023 11:41
@cjgillot
Copy link
Contributor

cjgillot commented Mar 8, 2023

We should probably just bail out from constructing MIR if match checking fails.
At the beginning of mir_built, we exit early of thir_body returns Err(ErrorGuaranteed). The check_match query could return a Result<(), ErrorGuaranteed>, and mir_built could early-exit based on that value too.
Calling check_match in mir_built is something we are planning to do anyway in #108118.

@cjgillot cjgillot mentioned this pull request Mar 8, 2023
Merged
@Noratrieb Noratrieb 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 Mar 11, 2023
@Noratrieb Noratrieb mentioned this pull request Mar 18, 2023
Closed
@clubby789
Copy link
Contributor Author

Is this blocked on #108118 then, or could this be done here without conflicting?

@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from 95f6058 to 73ea9e6 Compare April 30, 2023 15:34
@clubby789
Copy link
Contributor Author

clubby789 commented Apr 30, 2023 •
edited
Loading

Changed this to making the query and various functions return Result<(), ErrorGuarenteed>, bailing out in mir_built if match checking has errored

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2023
cjgillot
cjgillot reviewed Apr 30, 2023
View reviewed changes
@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from 73ea9e6 to f967076 Compare April 30, 2023 17:26
cjgillot
cjgillot reviewed Apr 30, 2023
View reviewed changes
@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from f967076 to 4ade156 Compare April 30, 2023 17:51
cjgillot
cjgillot reviewed Apr 30, 2023
View reviewed changes
@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from 4ade156 to 5792ab3 Compare April 30, 2023 18:11
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@clubby789 clubby789 force-pushed the refutable-let-closure-borrow branch from 5792ab3 to 2d5ca0e Compare April 30, 2023 18:17
@clubby789
Copy link
Contributor Author

#109523 is fixed by this too

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 10, 2023

📌 Commit 2d5ca0e has been approved by cjgillot

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 10, 2023

🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened.

@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 May 10, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request May 11, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 11, 2023
@bors
Auto merge of rust-lang#111452 - matthiaskrgr:rollup-uic8dgy, r=matth…
4d941cd 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#108705 (Prevent ICE with broken borrow in closure)
 - rust-lang#111292 (Fix mishandled `--check-cfg` arguments order)
 - rust-lang#111382 (Isolate coverage FFI type layouts from their underlying LLVM C++ types)
 - rust-lang#111385 (vec-shrink-panik: update expectations to work on LLVM 17)
 - rust-lang#111389 (Add esp-idf platform-support page)
 - rust-lang#111432 (Use visit_assign to detect SSA locals.)
 - rust-lang#111448 (Use proper impl self type for alias impl in rustdoc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 40d933a into rust-lang:master May 11, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 11, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request May 11, 2023
Closed
@dtolnay dtolnay mentioned this pull request Aug 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@Noratrieb Noratrieb

Labels

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

Milestone

1.71.0

Development

Successfully merging this pull request may close these issues.

ICE due to mapping f32 to same ident with const and let

6 participants

@clubby789 @Noratrieb @cjgillot @rust-log-analyzer @bors @rustbot