Unify "input" and "no input" paths in `run_compiler` #118002

nnethercote · 2023-11-17T06:28:28Z

A follow-up to #117649.

compiler/rustc_driver_impl/src/lib.rs

bjorn3 · 2023-11-17T08:51:41Z

compiler/rustc_driver_impl/src/lib.rs

@@ -293,7 +293,7 @@ fn run_compiler(
    >,
    using_internal_features: Arc<std::sync::atomic::AtomicBool>,
 ) -> interface::Result<()> {
-    let mut early_error_handler = EarlyErrorHandler::new(ErrorOutputType::default());
+    let mut default_handler = EarlyErrorHandler::new(ErrorOutputType::default());


I don't think the new name is any better. It is only supposed to be used for error handling very early on. After the Session is created it should't be used anymore.

Well, there is EarlyErrorHandler used after the session is created. Within the interface::run_compiler` call there is this:

let handler = EarlyErrorHandler::new(sess.opts.error_format);

The idea is that default_handler as a name contrasts more clearly with handler than early_error_handler does. Because they both have type EarlyErrorHandler.

All uses of let handler = EarlyErrorHandler::new(sess.opts.error_format); have Session passed in as argument and thus should work with sess.fatal("..."); just fine. I think it would make sense to remove all EarlyErrorHandler usage when sess is in scope and remove the error_format argument from EarlyErrorHandler.

nnethercote · 2023-11-17T09:09:55Z

I addressed two of the comments. I left default_handler unchanged. We have two EarlyErrorHandlers in this function, and I like naming them default_handler (for the one with the default error format) and handler (for the one with the error format from sess.opts). I think that's clearer than early_error_handler and handler, which don't give any clue about the difference between them.

bjorn3 · 2023-11-17T09:16:48Z

If the let handler = EarlyErrorHandler::new(sess.opts.error_format) is removed in favor of sess.fatal(), there is no need to distinguish between the two names anymore.

nnethercote · 2023-11-17T09:32:23Z

Do you mean a diff like this?

diff --git a/compiler/rustc_driver_impl/src/lib.rs b/compiler/rustc_driver_impl/src/lib.rs
index 555cfe4beab..a6ad6b6ef09 100644
--- a/compiler/rustc_driver_impl/src/lib.rs
+++ b/compiler/rustc_driver_impl/src/lib.rs
@@ -379,7 +379,9 @@ fn run_compiler(
         }

         if !has_input {
-            handler.early_error("no input filename given"); // this is fatal
+            #[allow(rustc::diagnostic_outside_of_impl)]
+            #[allow(rustc::untranslatable_diagnostic)]
+            sess.fatal("no input filename given");
         }

         if !sess.opts.unstable_opts.ls.is_empty() {

That changes the output on error slightly, e.g with rustc -g it goes from this:

error: no input filename given

to this:

error: no input filename given

error: aborting due to previous error

Is that expected/ok?

bjorn3 · 2023-11-17T09:43:56Z

Do you mean a diff like this?

Yes

That changes the output on error slightly

I see.

Is that expected/ok?

On one hand it is more consistent with almost all other errors, on the other hand it is a bit noisier. I think it is fine, but maybe ask on zulip what others think?

nnethercote · 2023-11-17T10:46:51Z

I asked on Zulip; not much response so far.

One possible way to look at this: the current PR preserves the existing behaviour. We could declare the conversion of the post-session EarlyErrorHandler errors as out of scope for this PR and suitable for a follow-up.

bjorn3 · 2023-11-17T10:50:25Z

Sure, r=me I don't know if this PR will conflict with your #117993.

nnethercote · 2023-11-17T11:09:49Z

It will conflict. I'll let that one merge first then fix this up.

bors · 2023-11-17T19:44:57Z

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

Yes, its type is `EarlyErrorHandler`, but there is another value of that type later on in the function called `handler` that is initialized with `sopts.error_format`. So `default_handler` is a better name because it clarifies that it is initialized with `ErrorOutputType::default()`.

`rustc_driver_impl::run_compiler` currently has two `interface::run_compiler` calls: one for the "no input" case, and one for the normal case. This commit merges the former into the latter, which makes the control flow easier to read and avoids some duplication. It also makes it clearer that the "no input" case will describe lints before printing crate info, while the normal case does it in the reverse order. Possibly a bug?

Currently we have an inconsistency between the "input" and "no input" cases: - no input: `rustc --print=sysroot -Whelp` prints the lint help. - input: `rustc --print=sysroot -Whelp a.rs` prints the sysroot. It makes sense to print the lint help in both cases, because that's what happens with `--help`/`-Zhelp`/`-Chelp`. In fact, the `describe_lints` in the "input" case happens amazingly late, after *parsing*. This is because, with plugins, lints used to be registered much later, when the global context was created. But rust-lang#117649 moved lint registration much earlier, during session construction. So this commit moves the `describe_lints` call to a single spot for both for both the "input" and "no input" cases, as early as possible. This is still not as early as `--help`/`-Zhelp`/`-Chelp`, because `-Whelp` must wait until the session is constructed.

I find `Compilation::and_then` hard to read. This commit removes it, simplifying the control flow in `run_compiler`, and reducing the number of lines of code. In particular, `list_metadata` and `process_try_link` (renamed `rlink`) are now only called if the relevant condition is true, rather than that condition being checked within the function.

nnethercote · 2023-11-17T22:27:56Z

I rebased.

@bors r=bjorn3

bors · 2023-11-17T22:27:59Z

📌 Commit 472f7c9 has been approved by bjorn3

It is now in the queue for this repository.

bors · 2023-11-18T16:43:51Z

⌛ Testing commit 472f7c9 with merge 28345f0...

bors · 2023-11-18T18:40:50Z

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 28345f0 to master...

rust-timer · 2023-11-18T20:03:09Z

Finished benchmarking commit (28345f0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.1%]	2
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

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)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.0%, -3.6%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 675.969s -> 675.681s (-0.04%)
Artifact size: 313.86 MiB -> 313.84 MiB (-0.01%)

rustbot assigned bjorn3 Nov 17, 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 Nov 17, 2023

bjorn3 reviewed Nov 17, 2023

View reviewed changes

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Nov 17, 2023

View reviewed changes

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Nov 17, 2023

View reviewed changes

nnethercote force-pushed the unify-input-no-input branch from 23e1b0e to 0645a6d Compare November 17, 2023 09:07

nnethercote added 5 commits November 18, 2023 07:17

Factor out two print_crate_info calls.

5659cc5

nnethercote force-pushed the unify-input-no-input branch from 0645a6d to 472f7c9 Compare November 17, 2023 22:27

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 Nov 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2023

bors merged commit 28345f0 into rust-lang:master Nov 18, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 18, 2023

bors mentioned this pull request Nov 18, 2023

Eagerly compute output_filenames #117584

Merged

nnethercote mentioned this pull request Nov 19, 2023

Remove support for compiler plugins. #116412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify "input" and "no input" paths in `run_compiler` #118002

Unify "input" and "no input" paths in `run_compiler` #118002

nnethercote commented Nov 17, 2023

bjorn3 Nov 17, 2023

nnethercote Nov 17, 2023

bjorn3 Nov 17, 2023

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bors commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bors commented Nov 17, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

Unify "input" and "no input" paths in run_compiler #118002

Unify "input" and "no input" paths in run_compiler #118002

Conversation

nnethercote commented Nov 17, 2023

bjorn3 Nov 17, 2023

Choose a reason for hiding this comment

nnethercote Nov 17, 2023

Choose a reason for hiding this comment

bjorn3 Nov 17, 2023

Choose a reason for hiding this comment

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bjorn3 commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bors commented Nov 17, 2023

nnethercote commented Nov 17, 2023

bors commented Nov 17, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Unify "input" and "no input" paths in `run_compiler` #118002

Unify "input" and "no input" paths in `run_compiler` #118002