Remove `DiagCtxt` API duplication #119146

nnethercote · 2023-12-20T00:12:58Z

DiagCtxt defines the internal API for creating and emitting diagnostics: methods like struct_err, struct_span_warn, note, create_fatal, emit_bug. There are over 50 methods.

Some of these methods are then duplicated across several other types: Session, ParseSess, Parser, ExtCtxt, and MirBorrowckCtxt. Session duplicates the most, though half the ones it does are unused. Each duplicated method just calls forward to the corresponding method in DiagCtxt. So this duplication exists to (in the best case) shorten chains like ecx.tcx.sess.parse_sess.dcx.emit_err() to ecx.emit_err().

This API duplication is ugly and has been bugging me for a while. And it's inconsistent: there's no real logic about which methods are duplicated, and the use of #[rustc_lint_diagnostic] and #[track_caller] attributes vary across the duplicates.

This PR removes the duplicated API methods and makes all diagnostic creation and emission go through DiagCtxt. It also adds dcx getter methods to several types to shorten chains. This approach scales much better than API duplication; indeed, the PR adds dcx() to numerous types that didn't have API duplication: TyCtxt, LoweringCtxt, ConstCx, FnCtxt, TypeErrCtxt, InferCtxt, CrateLoader, CheckAttrVisitor, and Resolver. These result in a lot of changes from foo.tcx.sess.emit_err() to foo.dcx().emit_err(). (You could do this with more types, but it gets into diminishing returns territory for types that don't emit many diagnostics.)

After all these changes, some call sites are more verbose, some are less verbose, and many are the same. The total number of lines is reduced, mostly because of the removed API duplication. And consistency is increased, because calls to emit_err and friends are always preceded with .dcx() or .dcx.

r? @compiler-errors

rustbot · 2023-12-20T00:13:09Z

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Type relation code was changed

cc @compiler-errors, @lcnr

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

nnethercote · 2023-12-20T00:13:35Z

Here's a table showing the full API and the existing duplication.

DC: DiagCtxt
S:  Session
PS: ParseSess
P:  Parser
EC: ExtCtxt
MB: MirBorrowckCtxt

func                              DC S PS P EC MB
----                              ---------------
struct_diagnostic                 DC     

struct_bug                        DC     

struct_fatal                      DC S PS
struct_span_fatal                 DC S   
struct_span_fatal_with_code       DC S     

struct_almost_fatal               DC       

struct_err                        DC S PS  
struct_span_err                   DC S    P EC
struct_err_with_code              DC S     
struct_span_err_with_code         DC S         MB  

struct_err_lint                   DC       

struct_warn                       DC S PS 
struct_span_warn                  DC S    
struct_warn_with_code             DC S    
struct_span_warn_with_code        DC S   

struct_warn_with_expectation      DC S      
struct_span_warn_with_expectation DC S     

struct_note                       DC S    
struct_span_note                  DC     

struct_help                       DC     

struct_allow                      DC S   
struct_span_allow                 DC S   

struct_expect                     DC S   

----

bug                               DC        EC  
span_bug                          DC      P EC 
span_bug_no_panic                 DC         

span_delayed_bug                  DC S       
good_path_delayed_bug             DC S       

fatal                             DC S       
span_fatal                        DC S       
span_fatal_with_code              DC S       

err                               DC S       
span_err                          DC S      EC    
span_err_with_code                DC S       

warn                              DC S      
span_warn                         DC S      
span_warn_with_code               DC S      

note                              DC S     
span_note                         DC S     

----

create_err                        DC S PS   EC  
create_warning                    DC S PS    
create_almost_fatal               DC        
create_fatal                      DC S PS   
create_bug                        DC        
create_note                       DC S PS   

----

emit_err                          DC S PS   EC 
emit_warning                      DC S PS    
emit_almost_fatal                 DC         
emit_fatal                        DC S PS   
emit_bug                          DC        
emit_note                         DC S PS

nnethercote · 2023-12-20T00:14:58Z

And here's a table showing all the possible chaining combinations.

ParseSess
- old
  - parse_sess.emit_err()
  - parse_sess.dcx.emit_err()
- new
  - parse_sess.dcx.emit_err()

Session
- old
  - sess.emit_err(e)
  - sess.parse_sess.emit_err(e)
  - sess.parse_sess.dcx.emit_err(e)
- new
  - sess.dcx().emit_err()
  - sess.parse_sess.dcx.emit_err(e)

Parser [note: `sess` here is a `ParseSess`]
- old
  - parser.emit_err()
  - parser.sess.emit_err()
  - parser.sess.dcx.emit_err()
- new
  - parser.dcx().emit_err()
  - parser.sess.dcx.emit_err()

TyCtxt (no current API duplication)
- old
  - tcx.sess.emit_err()
  - tcx.sess.parse_sess.emit_err(e)
  - tcx.sess.parse_sess.dcx.emit_err(e)
- new
  - tcx.dcx().emit_err()
  - tcx.sess.dcx().emit_err()
  - tcx.sess.parse_sess.dcx.emit_err()

ExtCtxt
- old
  - ecx.emit_err()
  - ecx.tcx.sess.emit_err()
  - ecx.tcx.sess.parse_sess.emit_err()
  - ecx.tcx.sess.parse_sess.dcx.emit_err()
- new
  - ecx.dcx().emit_err()
  - ecx.tcx.dcx().emit_err()
  - ecx.tcx.sess.dcx().emit_err()
  - ecx.tcx.sess.parse_sess.dcx.emit_err()

Other TyCtxt wrappers without a dcx() getter
- old
  - foo.tcx.sess.emit_err()
  - foo.tcx.sess.parse_sess.emit_err()
  - foo.tcx.sess.parse_sess.dcx.emit_err()
- new (if you don't add a dcx() getter)
  - foo.tcx.dcx().emit_err()
  - foo.tcx.sess.dcx().emit_err()
  - foo.tcx.sess.parse_sess.dcx.emit_err()
- new (if you add a dcx() getter)
  - foo.dcx().emit_err()
  - foo.tcx.dcx().emit_err()
  - foo.tcx.sess.dcx().emit_err()
  - foo.tcx.sess.parse_sess.dcx.emit_err()

The shortest chains are always preferred, of course.

compiler/rustc_const_eval/src/interpret/eval_context.rs

nnethercote · 2023-12-20T10:39:26Z

Not expected to affect perf, but let's check:

@bors try @rust-timer queue

bors · 2023-12-20T10:40:37Z

⌛ Trying commit 8a1a0de with merge 917190f...

…ion, r=<try> Remove `DiagCtxt` API duplication `DiagCtxt` defines the internal API for creating and emitting diagnostics: methods like `struct_err`, `struct_span_warn`, `note`, `create_fatal`, `emit_bug`. There are over 50 methods. Some of these methods are then duplicated across several other types: `Session`, `ParseSess`, `Parser`, `ExtCtxt`, and `MirBorrowckCtxt`. `Session` duplicates the most, though half the ones it does are unused. Each duplicated method just calls forward to the corresponding method in `DiagCtxt`. So this duplication exists to (in the best case) shorten chains like `ecx.tcx.sess.parse_sess.dcx.emit_err()` to `ecx.emit_err()`. This API duplication is ugly and has been bugging me for a while. And it's inconsistent: there's no real logic about which methods are duplicated, and the use of `#[rustc_lint_diagnostic]` and `#[track_caller]` attributes vary across the duplicates. This PR removes the duplicated API methods and makes all diagnostic creation and emission go through `DiagCtxt`. It also adds `dcx` getter methods to several types to shorten chains. This approach scales *much* better than API duplication; indeed, the PR adds `dcx()` to numerous types that didn't have API duplication: `TyCtxt`, `LoweringCtxt`, `ConstCx`, `FnCtxt`, `TypeErrCtxt`, `InferCtxt`, `CrateLoader`, `CheckAttrVisitor`, and `Resolver`. These result in a lot of changes from `foo.tcx.sess.emit_err()` to `foo.dcx().emit_err()`. (You could do this with more types, but it gets into diminishing returns territory for types that don't emit many diagnostics.) After all these changes, some call sites are more verbose, some are less verbose, and many are the same. The total number of lines is reduced, mostly because of the removed API duplication. And consistency is increased, because calls to `emit_err` and friends are always preceded with `.dcx()` or `.dcx`. r? `@compiler-errors`

bors · 2023-12-20T12:07:42Z

☀️ Try build successful - checks-actions
Build commit: 917190f (917190f61708fc9aa5ffeabf09fa56c678e43d18)

rust-timer · 2023-12-20T15:52:11Z

Finished benchmarking commit (917190f): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.2%, 1.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.5%, 2.0%]	3
Regressions ❌ (secondary)	3.4%	[2.1%, 4.4%]	3
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-2.9%, 2.0%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.7%	[6.7%, 6.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.674s -> 672.581s (-0.01%)
Artifact size: 312.84 MiB -> 312.70 MiB (-0.05%)

bors · 2023-12-21T23:46:50Z

☔ The latest upstream changes (presumably #119190) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2023-12-22T03:40:27Z

I fixed the conflicts.

bors · 2023-12-22T05:04:36Z

☔ The latest upstream changes (presumably #119163) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2023-12-22T22:09:35Z

I figure I won't fix the conflicts again until/unless this gets r+, because it's so conflict-prone. The conflicts won't affect reviewing in any significant way.

bors · 2023-12-26T02:24:43Z

⌛ Testing commit 8a9db25 with merge 2271c26...

bors · 2023-12-26T04:24:46Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 2271c26 to master...

rust-timer · 2023-12-26T07:27:45Z

Finished benchmarking commit (2271c26): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.7%	[4.7%, 4.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.724s -> 672.287s (0.38%)
Artifact size: 312.56 MiB -> 312.50 MiB (-0.02%)

…ion, r=compiler-errors Remove `DiagCtxt` API duplication `DiagCtxt` defines the internal API for creating and emitting diagnostics: methods like `struct_err`, `struct_span_warn`, `note`, `create_fatal`, `emit_bug`. There are over 50 methods. Some of these methods are then duplicated across several other types: `Session`, `ParseSess`, `Parser`, `ExtCtxt`, and `MirBorrowckCtxt`. `Session` duplicates the most, though half the ones it does are unused. Each duplicated method just calls forward to the corresponding method in `DiagCtxt`. So this duplication exists to (in the best case) shorten chains like `ecx.tcx.sess.parse_sess.dcx.emit_err()` to `ecx.emit_err()`. This API duplication is ugly and has been bugging me for a while. And it's inconsistent: there's no real logic about which methods are duplicated, and the use of `#[rustc_lint_diagnostic]` and `#[track_caller]` attributes vary across the duplicates. This PR removes the duplicated API methods and makes all diagnostic creation and emission go through `DiagCtxt`. It also adds `dcx` getter methods to several types to shorten chains. This approach scales *much* better than API duplication; indeed, the PR adds `dcx()` to numerous types that didn't have API duplication: `TyCtxt`, `LoweringCtxt`, `ConstCx`, `FnCtxt`, `TypeErrCtxt`, `InferCtxt`, `CrateLoader`, `CheckAttrVisitor`, and `Resolver`. These result in a lot of changes from `foo.tcx.sess.emit_err()` to `foo.dcx().emit_err()`. (You could do this with more types, but it gets into diminishing returns territory for types that don't emit many diagnostics.) After all these changes, some call sites are more verbose, some are less verbose, and many are the same. The total number of lines is reduced, mostly because of the removed API duplication. And consistency is increased, because calls to `emit_err` and friends are always preceded with `.dcx()` or `.dcx`. r? `@compiler-errors`

Relevant PRs: - rust-lang/rust#119601 - rust-lang/rust#119146 - rust-lang/rust#119174 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

rustbot assigned compiler-errors Dec 20, 2023