Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix more ICEs in diagnostic::on_unimplemented #124875

Merged
merged 2 commits into from
May 9, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented May 8, 2024

There were 8 other calls to expect_local left in on_unimplemented.rs -- all of which (afaict) could be turned into ICEs.

I would really like to see validation of on_unimplemented separated from parsing, so we only emit errors here:

pub(super) fn check_on_unimplemented(tcx: TyCtxt<'_>, def_id: LocalDefId) {
// an error would be reported if this fails.
let _ = OnUnimplementedDirective::of_item(tcx, def_id.to_def_id());
}

...And gracefully fail instead when emitting trait predicate failures, not ever even trying to emit an error or a lint. But that's left for a separate PR.

r? @estebank

@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 May 8, 2024
@compiler-errors
Copy link
Member Author

This should be backported together with #124683.

@compiler-errors compiler-errors added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels May 8, 2024
@@ -1,17 +0,0 @@
//@ edition:2021
Copy link
Member Author

Choose a reason for hiding this comment

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

Github didn't recognize the file being moved, but I gave it a better name. Also, there's no reason for us to be using #[test] here -- it's not necessary to reproduce the ICE; all that is necessary is a foreign crate.

@estebank
Copy link
Contributor

estebank commented May 8, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented May 8, 2024

📌 Commit 7dbdbaa has been approved by estebank

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 May 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#124777 (Fix Error Messages for `break` Inside Coroutines)
 - rust-lang#124837 (Migrate `run-make/rustdoc-map-file` to rmake)
 - rust-lang#124875 (Fix more ICEs in `diagnostic::on_unimplemented`)
 - rust-lang#124908 (Handle field projections like slice indexing in invalid_reference_casting)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c49436d into rust-lang:master May 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2024
Rollup merge of rust-lang#124875 - compiler-errors:more-diagnostics-ices, r=estebank

Fix more ICEs in `diagnostic::on_unimplemented`

There were 8 other calls to `expect_local` left in `on_unimplemented.rs` -- all of which (afaict) could be turned into ICEs.

I would really like to see validation of `on_unimplemented` separated from parsing, so we only emit errors here:
https://github.com/rust-lang/rust/blob/a60f077c38fe66d05449919842d3d73e3299bbab/compiler/rustc_hir_analysis/src/check/check.rs#L836-L839
...And gracefully fail instead when emitting trait predicate failures, not *ever* even trying to emit an error or a lint. But that's left for a separate PR.

r? `@estebank`
@apiraino
Copy link
Contributor

Beta backports approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

Stable backport: same reasoning as for #124683: does not justify a dot release but this backport could jump on that train, if a dot release is planned for other reasons

@rustbot label +beta-accepted +stable-accepted

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels May 10, 2024
@cuviper cuviper mentioned this pull request May 16, 2024
@cuviper cuviper modified the milestones: 1.80.0, 1.79.0 May 16, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 16, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 17, 2024
[beta] backports

- Do not ICE on foreign malformed `diagnostic::on_unimplemented` rust-lang#124683
- Fix more ICEs in `diagnostic::on_unimplemented` rust-lang#124875
- rustdoc: use stability, instead of features, to decide what to show rust-lang#124864
- Don't do post-method-probe error reporting steps if we're in a suggestion rust-lang#125100
-  Make `non-local-def` lint Allow by default rust-lang#124950

r? cuviper
@cuviper cuviper removed stable-nominated Nominated for backporting to the compiler in the stable channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jun 21, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Development

Successfully merging this pull request may close these issues.

6 participants