Cleanup error handlers some more by nnethercote · Pull Request #118587 · 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

Conversation8 Commits13 Checks0 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 }})

nnethercote

@nnethercote

Currently, Handler::fatal returns FatalError. But Session::fatal returns !, because it calls Handler::fatal and then calls raise on the result. This inconsistency is unfortunate.

This commit changes Handler::fatal to do the raise itself, changing its return type to !. This is safe because there are only two calls to Handler::fatal, one in rustc_session and one in rustc_codegen_cranelift, and they both call raise on the result.

HandlerInner::fatal still returns FatalError, so I renamed it fatal_no_raise to emphasise the return type difference.

@nnethercote

They each have a single call site.

@rustbot

⚠️ Warning ⚠️

@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

Dec 4, 2023

@rustbot

Some changes occurred in src/tools/cargo

cc @ehuss

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@nnethercote

There are more improvements to be made, but this is enough changes for one PR. In general, the error handling code has lots of different ways to do things, and this is steps towards cutting things back so there are fewer ways. As always, best reviewed one commit at a time.

I don't know why the src/tools/cargo changes are in there, I've tried doing git submodule update --recursive a bunch of times, but the changes keep finding a way to come back.

@nnethercote

They each have a single call site.

Note: the make_diagnostic_builder calls in lib.rs will be replaced in a subsequent commit.

@nnethercote

sess is a terribly misleading name for a Handler! This confused me for a bit.

@nnethercote

That's what is mostly used. This commit changes a few EM and E and T type variables to G.

@nnethercote

Diagnostic::new can be used instead.

@nnethercote

@nnethercote

These impls are all needed for just a single IntoDiagnostic type, not a family of them.

Note that ErrorGuaranteed is the default type parameter for IntoDiagnostic.

@nnethercote

By making it generic, instead of only for EmissionGuarantee = (), we can use it everywhere.

@nnethercote

Handler is a wrapper around HanderInner. Some functions on on Handler just forward to the samed-named functions on HandlerInner.

This commit removes as many of those as possible, implementing functions on Handler where possible, to avoid the boilerplate required for forwarding. The commit is moderately large but it's very mechanical.

@nnethercote

This is weird: HandlerInner::emit calls HandlerInner::emit_diagnostic, but only after doing a treat-err-as-bug check. Which is fine, except that there are multiple others paths for an Error or Fatal diagnostic to be passed to HandlerInner::emit_diagnostic without going through HandlerInner::emit, e.g. Handler::span_err call Handler::emit_diag_at_span, which calls emit_diagnostic. So that suggests that the coverage for treat-err-as-bug is incomplete.

This commit removes HandlerInner::emit and moves the treat-err-as-bug check to HandlerInner::emit_diagnostic, so it cannot by bypassed.

@nnethercote

@nnethercote

This makes Handler::fatal more like Handler::{err,warn,bug,note}.

@nnethercote

I managed to eliminate the src/tools/cargo changes.

@compiler-errors

@bors

📌 Commit 7811c97 has been approved by compiler-errors

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

Dec 5, 2023

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Dec 5, 2023

@compiler-errors

…-2, r=compiler-errors

Cleanup error handlers some more

A sequel to rust-lang#118470.

r? @compiler-errors

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Dec 5, 2023

@compiler-errors

…-2, r=compiler-errors

Cleanup error handlers some more

A sequel to rust-lang#118470.

r? @compiler-errors

This was referenced

Dec 5, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 5, 2023

@bors

…mpiler-errors

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 5, 2023

@bors

…mpiler-errors

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 6, 2023

@bors

…mpiler-errors

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 6, 2023

@bors

…mpiler-errors

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Dec 6, 2023

@rust-timer

Rollup merge of rust-lang#118587 - nnethercote:cleanup-error-handlers-2, r=compiler-errors

Cleanup error handlers some more

A sequel to rust-lang#118470.

r? @compiler-errors

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Dec 15, 2023

@workingjubilee

…re, r=compiler-errors

Cleanup errors handlers even more

A sequel to rust-lang#118587.

r? @compiler-errors

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Dec 15, 2023

@rust-timer

Rollup merge of rust-lang#118933 - nnethercote:cleanup-errors-even-more, r=compiler-errors

Cleanup errors handlers even more

A sequel to rust-lang#118587.

r? @compiler-errors

oli-obk

Comment on lines +684 to +688

if matches!(diag.level, Level::Error { lint: true }) {
inner.lint_err_count += 1;
} else {
inner.err_count += 1;
}

Choose a reason for hiding this comment

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

This breaks -Ztreat-err-as-bug, as the counter changes, even though panic_if_treat_err_as_bug is not invoked here.

Choose a reason for hiding this comment

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

This change was in the "Move some HandlerInner functions to Handler." commit. It just moved code around. The code added here was from HandlerInner::stash, which was inlined and removed in the commit. There were no functional changes.

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.