Avoid ICES after reporting errors on erroneous patterns by oli-obk · Pull Request #126320 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits5 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Contributor
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 😊
This comment has been minimized.
Contributor
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 |
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.
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), |
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
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
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
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. |
| //! |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📌 Commit 7566307 has been approved by lcnr
It is now in the queue for this repository.
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
fmease added a commit to fmease/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#121216 (Always emit
native-static-libsnote, 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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#121216 (Always emit
native-static-libsnote, 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 andT: ?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
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#121216 (Always emit
native-static-libsnote, 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 andT: ?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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#121216 (Always emit
native-static-libsnote, 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 andT: ?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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#121216 (Always emit
native-static-libsnote, 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 andT: ?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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…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-cmdlinetormake.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
…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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.