Don't abort compilation after giving a lint error

[jyn514]

The only reason to use abort_if_errors is when the program is so broken that either:

1. later passes get confused and ICE
2. any diagnostics from later passes would be noise

This is never the case for lints, because the compiler has to be able to deal with allow-ed lints.
So it can continue to lint and compile even if there are lint errors.

Closes #82761.

[rust-highfive]

r? @LeSeulArtichaut

(rust-highfive has picked a reviewer for you, use r? to override)

[flip1995]

Copying my comment about this from Zulip:

If you don't want the compiler to abort on a deny-by-default lint, then you can just use --cap-lints=warn. Preserving the lint levels and keep going even if there is an error makes lint levels completely nonsensical.

IMO not aborting after an error, even with lints, is a bad idea. Here's why:

The compiler works in stages / IRs and so do lints. So there is the AST, where source-level checks are done, the (T)HIR where type checking and similar things happen and the MIR, where borrow checking and the like happen. If you already have erroneous code at the source-level (AST), then you should fix those first, before the linter tries to check type information or borrowing stuff.

Making the compiler not stop after an error would render different lint levels useless. The only difference then would be that they have a different color in the terminal.

So I'm against this change, especially since there already is the --cap-lints flag where you can tell the compiler to ignore all lint levels above a specific level.

---

This will definitely fail the Clippy test suite, since our UI tests make the assumption that an error in an EarlyPass lint will prevent LatePassLints from running. And so does rustc:

test result: FAILED. 11623 passed; 424 failed;

[SNCPlay42]

Making the compiler not stop after an error would render different lint levels useless. The only difference then would be that they have a different color in the terminal.

What I imagined when filing #82761 was not that the compiler would not abort on error-level lints, but that it would not abort early. That preserves an important difference between error and warning lint levels: a lint being error-level would still cause the compiler to not emit compilation artifacts and exit with nonzero status (resulting in e.g. a CI run reporting failure instead of success), unlike a warning. I don't think anyone is advocating that compilation should actually succeed if there are error level lints* (in situations where that is desirable, as you note, --cap-lints exists); I think it would be a bug in this PR if it causes that to happen.

(*EDIT: Actually @jyn514 did talk about having cargo builds keep going into dependent crates in the case of lint errors, which I guess requires this to some extent, but I think we would want to preserve a sense that the build overall has failed, e.g. cargo would not exit with a zero status.)

If you already have erroneous code at the source-level (AST), then you should fix those first, before the linter tries to check type information or borrowing stuff.

This is reasonable for hard errors (and sometimes completely necessary - for instance the compiler can't typecheck code that it can't parse into an AST), but I'm not convinced this is the case for lints, especially those that come from external tools like clippy - it seems less likely that a problem that would trigger a clippy lint, but otherwise would still allow the code to be compiled, would cause that kind of cascade of errors where the later diagnostics don't add value.

[flip1995]

but I'm not convinced this is the case for lints, especially those that come from external tools like clippy

There are lints in Clippy that don't check for specific things, since they will be caught by earlier (rustc) lints. I don't know if this will lead to ICEs (worst case) or just some contradicting lint output, or if it just doesn't matter.

not that the compiler would not abort on error-level lints, but that it would not abort early.

Thinking about this, my concerns may be a bit unwarranted.

Since to get this PR through, many allows will have to be added to the UI tests, I think we'd find if lints really depend on earlier lints. So I'd be happy with this, if my concerns will be proven wrong in the test suite(s).

[jyn514]

@flip1995 oh, I was just going to bless the tests, so you can see exactly what changed (I've already done that locally but I got distracted by preserving the error status). Would you prefer that I use allows instead? That seems less helpful, you wouldn't see contradictory lints.

[flip1995]

The Clippy test suite is designed so that one test (mostly named after the lint) tests exactly one lint. If that file accidentally triggers another lint, we just allow that lint instead of carefully rewriting the test. Either way, we definitely don't want to clobber the stderr with output from unrelated lints.

[jyn514]

FWIW, another use case for this is when deny(warnings) is set - I've found it frustrating when working on rustc itself that I only see the first couple errors before the others show up.

[flip1995]

I just ran the Clippy test suite for those changes:

test result: FAILED. 632 passed; 16 failed; 3 ignored; 0 measured; 0 filtered out; finished in 9.83s

So only 16 tests are effected. That's a surprisingly low number.

[LeSeulArtichaut]

Would this change warrant a MCP?

[jyn514]

@LeSeulArtichaut I'm happy to open an MCP, I'll do that this afternoon. I don't think it's necessarily required, but it doesn't hurt.

[LeSeulArtichaut]

So only 16 tests are effected. That's a surprisingly low number.

I'd guess that is because most tests in Clippy only test one lint?

[jyn514]

@bors r=oli-obk,flip1995 rollup=iffy

[bors]

📌 Commit ebf8966 has been approved by oli-obk,flip1995

[bors]

⌛ Testing commit ebf8966 with merge 214cd1f...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk,flip1995
Pushing 214cd1f to master...

[rust-timer]

Finished benchmarking commit (214cd1f): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression