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

Avoid ICES after reporting errors on erroneous patterns #126320

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 12, 2024

fixes #109812
fixes #125914
fixes #124004

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jun 12, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 12, 2024

pulled out of #126316

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

👍 sgtm, though I would love to see some slightly more involved cleanup here while you're already at it 😊

compiler/rustc_hir_typeck/src/pat.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Jun 12, 2024

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned cjgillot Jun 12, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

very nice ❤️

r=me after nits

Ok(ty) => ty,
Err(err) => {
err.emit();
expected
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that we return expected instead of Ty::new_error here 🤔 if someone wants to experiment with this, try changing it and run our UI test suite.

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 did have that, the result is not great. I should document it here

let expected = self.resolve_vars_with_obligations(expected);

let e = match self.coerce(expr, checked_ty, expected, allow_two_phase, None) {
Ok(ty) => return (ty, None),
Ok(ty) => return Ok(ty),
Copy link
Contributor

Choose a reason for hiding this comment

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

we set_tainted_by_errors in line 263 using a span_delayed_bug and then emit the error further down in line 270. Feel like we should just flip the order and use the ErrorGuaranteed from actually emitting something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, we didn't emit the error initially, so I didn't change it, good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nevermind, we're not actually emitting the error here, we're just creating the diagnostic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those functions don't have a great name, but that's a very common pattern throughout rustc. Maybe we should add an internal lint for methods with report in their name that also return a Diag.

//! Instead of actually analyzing the erroneous patterns,
//! we instead stop after typeck where errors are already
//! reported.
//!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//!

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 13, 2024

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Jun 13, 2024

📌 Commit 7566307 has been approved by lcnr

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 Jun 13, 2024
fmease added a commit to fmease/rust that referenced this pull request Jun 13, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#122613 (Don't build a broken/untested profiler runtime on mingw targets)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)

Failed merges:

 - rust-lang#126388 (const-eval: make lint scope computation consistent)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126285 (`UniqueRc`: support allocators and `T: ?Sized`.)
 - rust-lang#126315 (Add pub struct with allow(dead_code) into worklist)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126285 (`UniqueRc`: support allocators and `T: ?Sized`.)
 - rust-lang#126315 (Add pub struct with allow(dead_code) into worklist)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126285 (`UniqueRc`: support allocators and `T: ?Sized`.)
 - rust-lang#126315 (Add pub struct with allow(dead_code) into worklist)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)
 - rust-lang#126436 (Reduce rustdoc GUI tests flakyness)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121216 (Always emit `native-static-libs` note, even if it is empty)
 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126285 (`UniqueRc`: support allocators and `T: ?Sized`.)
 - rust-lang#126315 (Add pub struct with allow(dead_code) into worklist)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126399 (extend the check for LLVM build)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#124884 (place explicit lifetime bound after generic param)
 - rust-lang#126244 (Update fuchsia commit, and SDK to 21.20240610.2.1)
 - rust-lang#126270 (Migrate run make const fn mir)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126343 (Remove some msys2 utils)
 - rust-lang#126351 (std::unix::fs::link using direct linkat call for Solaris.)
 - rust-lang#126368 (Remove some unnecessary crate dependencies.)
 - rust-lang#126386 (Migrate `run-make/allow-non-lint-warnings-cmdline` to `rmake.rs`)
 - rust-lang#126449 (Fill out missing Windows support information)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#123962 (change method resolution to constrain hidden types instead of rejecting method candidates)
 - rust-lang#126244 (Update fuchsia commit, and SDK to 21.20240610.2.1)
 - rust-lang#126270 (Migrate run make const fn mir)
 - rust-lang#126320 (Avoid ICES after reporting errors on erroneous patterns)
 - rust-lang#126449 (Fill out missing Windows support information)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b3fb62 into rust-lang:master Jun 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
Rollup merge of rust-lang#126320 - oli-obk:pat_ice, r=lcnr

Avoid ICES after reporting errors on erroneous patterns

fixes rust-lang#109812
fixes rust-lang#125914
fixes rust-lang#124004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
6 participants