Cleanup error handlers some more #118587

nnethercote · 2023-12-04T06:14:02Z

A sequel to #118470.

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.

They each have a single call site.

rustbot · 2023-12-04T06:14:10Z

⚠️ Warning ⚠️

These commits modify submodules.

rustbot · 2023-12-04T06:14:12Z

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 · 2023-12-04T07:26:07Z

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.

They each have a single call site. Note: the `make_diagnostic_builder` calls in `lib.rs` will be replaced in a subsequent commit.

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

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

`Diagnostic::new` can be used instead.

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`.

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

`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.

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.

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

nnethercote · 2023-12-04T08:07:12Z

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

compiler-errors · 2023-12-05T15:55:51Z

@bors r+

bors · 2023-12-05T15:55:54Z

📌 Commit 7811c97 has been approved by compiler-errors

It is now in the queue for this repository.

…-2, r=compiler-errors Cleanup error handlers some more A sequel to rust-lang#118470. r? `@compiler-errors`

…-2, r=compiler-errors Cleanup error handlers some more A sequel to rust-lang#118470. r? ``@compiler-errors``

…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#117793 (Update variable name to fix `unused_variables` warning) - rust-lang#118123 (Add support for making lib features internal) - rust-lang#118268 (Pretty print `Fn<(..., ...)>` trait refs with parentheses (almost) always) - rust-lang#118346 (Add `deeply_normalize_for_diagnostics`, use it in coherence) - rust-lang#118350 (Simplify Default for tuples) - rust-lang#118450 (Use OnceCell in cell module documentation) - rust-lang#118585 (Fix parser ICE when recovering `dyn`/`impl` after `for<...>`) - rust-lang#118587 (Cleanup error handlers some more) - rust-lang#118642 (bootstrap(builder.rs): Don't explicitly warn against `semicolon_in_expressions_from_macros`) r? `@ghost` `@rustbot` modify labels: rollup

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```

…re, r=compiler-errors Cleanup errors handlers even more A sequel to rust-lang#118587. r? `@compiler-errors`

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 · 2024-02-05T08:49:14Z

compiler/rustc_errors/src/lib.rs

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


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

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.

nnethercote added 2 commits December 4, 2023 15:42

Inline and remove DiagnosticBuilder::new_diagnostic_* functions.

d4933aa

They each have a single call site.

rustbot assigned compiler-errors Dec 4, 2023

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

nnethercote added 11 commits December 4, 2023 18:57

Inline and remove more DiagnosticBuilder::new_diagnostic_* functions.

ab640ca

They each have a single call site. Note: the `make_diagnostic_builder` calls in `lib.rs` will be replaced in a subsequent commit.

Rename some arguments.

6a95dee

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

Always use G for EmissionGuarantee type variables.

ed95f39

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

Avoid Diagnostic::new_with_code(..., None, ...).

32dc78e

`Diagnostic::new` can be used instead.

Remove some unused code, and downgrade some pubs.

d51b3db

De-genericize some IntoDiagnostic impls.

b7e18ca

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`.

Use DiagnosticBuilder::new more.

8c20ad6

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

Make Handler::{err,bug} more like Handler::{warn,note}.

3ab05ca

Inline and remove fatal_no_raise.

7811c97

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

nnethercote force-pushed the cleanup-error-handlers-2 branch from 9010a90 to 7811c97 Compare December 4, 2023 08:06

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

Rollup merge of rust-lang#118587 - nnethercote:cleanup-error-handlers…

7c09391

…-2, r=compiler-errors Cleanup error handlers some more A sequel to rust-lang#118470. r? `@compiler-errors`

compiler-errors mentioned this pull request Dec 5, 2023

Rollup of 8 pull requests #118652

Closed

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 5, 2023

Rollup merge of rust-lang#118587 - nnethercote:cleanup-error-handlers…

0f0615c

…-2, r=compiler-errors Cleanup error handlers some more A sequel to rust-lang#118470. r? ``@compiler-errors``

This was referenced Dec 5, 2023

Rollup of 8 pull requests #118653

Closed

Rollup of 9 pull requests #118655

Merged

bors merged commit e813370 into rust-lang:master Dec 6, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Dec 6, 2023

nnethercote deleted the cleanup-error-handlers-2 branch December 7, 2023 04:58

nnethercote mentioned this pull request Dec 14, 2023

Cleanup errors handlers even more #118933

Merged

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 15, 2023

Rollup merge of rust-lang#118933 - nnethercote:cleanup-errors-even-mo…

9e872b7

…re, r=compiler-errors Cleanup errors handlers even more A sequel to rust-lang#118587. r? `@compiler-errors`

oli-obk reviewed Feb 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup error handlers some more #118587

Cleanup error handlers some more #118587

nnethercote commented Dec 4, 2023

rustbot commented Dec 4, 2023

rustbot commented Dec 4, 2023

nnethercote commented Dec 4, 2023

nnethercote commented Dec 4, 2023

compiler-errors commented Dec 5, 2023

bors commented Dec 5, 2023

oli-obk Feb 5, 2024

nnethercote Feb 5, 2024

Cleanup error handlers some more #118587

Cleanup error handlers some more #118587

Conversation

nnethercote commented Dec 4, 2023

rustbot commented Dec 4, 2023

rustbot commented Dec 4, 2023

nnethercote commented Dec 4, 2023

nnethercote commented Dec 4, 2023

compiler-errors commented Dec 5, 2023

bors commented Dec 5, 2023

oli-obk Feb 5, 2024

Choose a reason for hiding this comment

nnethercote Feb 5, 2024

Choose a reason for hiding this comment