Remove various `has_errors` or `err_count` uses by oli-obk · Pull Request #120342 · rust-lang/rust

oli-obk · 2024-01-25T17:14:37Z

follow up to #119895

r? @nnethercote since you recently did something similar.

There are so many more of these, but I wanted to get a PR out instead of growing the commit list indefinitely. The commits all work on their own and can be reviewed commit by commit.

All errors are local anyway, so we can track them directly

rustbot · 2024-01-25T17:14:49Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

nnethercote · 2024-01-25T20:59:02Z

compiler/rustc_expand/src/mbe/macro_rules.rs

-    let err = sess.dcx().err_count();
-    check_matcher_core(sess, def, &first_sets, matcher, &empty_suffix);
-    err == sess.dcx().err_count()
+    check_matcher_core(sess, def, &first_sets, matcher, &empty_suffix)?;


You can drop the trailing ?; and the Ok(()).

This function returns Result<()> while the _core function returns an ok value

nnethercote · 2024-01-25T21:06:22Z

compiler/rustc_expand/src/mbe/macro_rules.rs

-                    valid &= check_lhs_nt_follows(sess, def, &tt);
+                    // We don't handle errors here, the driver will abort
+                    // after parsing/expansion. we can report every error in every macro this way.
+                    valid &= check_lhs_nt_follows(sess, def, &tt).is_ok();


Pre-existing, but updating valid here is pointless when it's immediately followed by return.

Which means check_lhs_nt_follows doesn't need to return a bool.

It would be good to know how/why the driver will abort after parsing/expansion, bonus points if you understand that and can augment this comment appropriately.

No, this return is in a closure

nnethercote · 2024-01-25T21:11:22Z

src/tools/clippy/clippy_lints/src/transmute/utils.rs

-    assert!(
-        !fn_ctxt.errors_reported_since_creation(),
-        "Newly created FnCtxt contained errors"
-    );


Do you think this assertion just isn't that important? Likewise with the similar one below?

Yea, one of them just checked that constructing a FnCtxt doesn't have errors. The other was just unusual to me. One could check the error count manually if such a check was desired. I don't think these checks actually caught anything

nnethercote · 2024-01-25T21:23:47Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

-        // FIXME: statically guarantee this by tainting after the diagnostic is emitted
-        self.set_tainted_by_errors(
-            tcx.dcx().span_delayed_bug(span, "`report_selection_error` did not emit an error"),
-        );


This was weird, good to see it removed.

nnethercote · 2024-01-25T21:26:06Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

-        if predicate.references_error() {
-            return;
+        if let Err(e) = predicate.error_reported() {
+            return e;


Can you just predicate.error_reported()?;?

No, we return ErrorGuaranteed, not Result

nnethercote · 2024-01-25T21:26:36Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

-                if predicate.references_error() {
-                    return;
+                if let Err(e) = predicate.error_reported() {
+                    return e;


nnethercote · 2024-01-25T21:31:54Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                    return e;
+                }
+                if let Some(e) = self.dcx().has_errors() {
+                    return e;


This has_errors call is unfortunate, as is the one just below :/

I wonder if it's really necessary.

Preexisting. I'm not touching these in this PR

I will get rid of them in follow ups

nnethercote · 2024-01-25T21:32:53Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                    return e;
+                }
+                if let Some(e) = self.tainted_by_errors() {
+                    return e;


But this one doesn't have the has_errors call, nor does the following one. Interesting.

I think it was just haphazard bugfixing. We'll be able to fix that with sufficiently smart tainting, I'm certain

nnethercote · 2024-01-25T21:37:09Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

                .with_note("this may fail depending on what value the parameter takes")
                .emit();
-            return None;
+            return Err(guar);


No need for the guar local variable, just return directly.

But it's such a long method chain...

nnethercote · 2024-01-25T21:38:33Z

tests/ui/treat-err-as-bug/eagerly-emit.rs


 fn f() -> impl Foo {
    //~^ ERROR the trait bound `i32: Foo` is not satisfied
-    //~| ERROR `report_selection_error` did not emit an error


I guess this is an improvement? Yeah, seems so.

nnethercote · 2024-01-25T21:47:59Z

compiler/rustc_builtin_macros/src/format.rs

            _ => {
                let expr = p.parse_expr()?;
                if !args.named_args().is_empty() {
-                    ecx.dcx().emit_err(errors::PositionalAfterNamed {


This seems unconnected with the second half of this commit. Is it necessary?

Also, if necessary, it seems incomplete, because there are two other emit calls in this function. Seems like they should be converted to return created errors as well?

The others return. this one doesn't

I see one err.emit() and two emit_err() calls in parse_args. Prior to this change, none of them returned. After this change, one of them returns an Err without emitting, and the other two are unchanged.

Oh hmm. Need to rethink this one

The reason I did this is so that expand_format_args_impl (which is the only parse_args caller), doesn't cause follow-up warnings like

warning: named argument `foo` is not used by name --> $DIR/format-parse-errors.rs:9:9 | LL | "s {foo} {} {}", | -- this formatting argument uses named argument `foo` by position LL | foo = foo, | ^^^ this named argument is referred to by position in formatting string | = note: `#[warn(named_arguments_used_positionally)]` on by default help: use the named argument by name to avoid ambiguity | LL | "s {foo} {foo} {}", | +++

which are a bit nonsensical considering we already got a

error: positional arguments cannot follow named arguments --> $DIR/format-parse-errors.rs:10:9 | LL | foo = foo, | --------- named argument LL | bar, | ^^^ positional arguments must be before named arguments

and the fact that the diagnostic is likely always wrong. So I opted to just make the entire macro expansion go to a DummyResult::any if there were errors during it. This is already done for other errors, so...

I'm now not clear if you're intending to do more work here ("need to rethink this one") or if you are waiting for a response from me. Can you clarify?

There are no changes needed. this early return ensures that the macro expansion is poisoned instead of expanding to nonsense that may cause follow up nonsensical diagnostics.

Basically I think we should merge it as is, but I can also remove the entire commit and file it as a separate PR if you prefer

Ok, I'll accept that explanation, thanks.

nnethercote · 2024-01-25T22:36:51Z

These are nice improvements, and it all LGTM except for the fifth commit with the parse_args changes.

nnethercote · 2024-01-29T02:31:12Z

r=me if you want to exclude the 5th commit (containing the parse_args change).

nnethercote · 2024-01-30T04:49:02Z

@bors r+

bors · 2024-01-30T04:49:05Z

📌 Commit 054e1e3 has been approved by nnethercote

It is now in the queue for this repository.

…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#117906 (Improve display of crate name when hovered) - rust-lang#118533 (Suppress unhelpful diagnostics for unresolved top level attributes) - rust-lang#120293 (Deduplicate more sized errors on call exprs) - rust-lang#120295 (Remove `raw_os_nonzero` feature.) - rust-lang#120310 (adapt test for v0 symbol mangling) - rust-lang#120342 (Remove various `has_errors` or `err_count` uses) - rust-lang#120434 (Revert outdated version of "Add the wasm32-wasi-preview2 target") - rust-lang#120445 (Fix some `Arc` allocator leaks) - rust-lang#120475 (Improve error message when `cargo build` is used to build the compiler) - rust-lang#120476 (Remove some unnecessary check logic for lang items in HIR typeck) - rust-lang#120485 (add missing potential_query_instability for keys and values in hashmap) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120342 - oli-obk:track_errors6, r=nnethercote Remove various `has_errors` or `err_count` uses follow up to rust-lang#119895 r? `@nnethercote` since you recently did something similar. There are so many more of these, but I wanted to get a PR out instead of growing the commit list indefinitely. The commits all work on their own and can be reviewed commit by commit.

Remove various `has_errors` or `err_count` uses follow up to rust-lang#119895 r? `@nnethercote` since you recently did something similar. There are so many more of these, but I wanted to get a PR out instead of growing the commit list indefinitely. The commits all work on their own and can be reviewed commit by commit.

oli-obk added 5 commits January 25, 2024 11:57

Stop checking err_count in macro_rules validity checking

f68741b

All errors are local anyway, so we can track them directly

Remove an unused error count check

f6f0e04

Statically ensure an error is reported in report_arg_errors

646c8fc

Statically ensure report_selection_error actually reports an error

2b60e56

Remove has_errors check in builtin macro parsing

3042da0

rustbot assigned nnethercote Jan 25, 2024

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 Jan 25, 2024

oli-obk commented Jan 25, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs Outdated Show resolved Hide resolved

Track ErrorGuaranteed instead of conjuring it from thin air

054e1e3

nnethercote reviewed Jan 25, 2024

View reviewed changes

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 Jan 30, 2024

GuillaumeGomez mentioned this pull request Jan 30, 2024

Rollup of 11 pull requests #120496

Merged

bors merged commit b28e6f1 into rust-lang:master Jan 30, 2024

rustbot added this to the 1.77.0 milestone Jan 30, 2024

oli-obk deleted the track_errors6 branch January 31, 2024 09:44

rust-timer mentioned this pull request Feb 9, 2024

Rollup of 11 pull requests #120852

Merged

matthiaskrgr mentioned this pull request Feb 10, 2024

ICE: errors.is_empty() #120864

Closed

Uh oh!

Conversation

oli-obk commented Jan 25, 2024

Uh oh!

rustbot commented Jan 25, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Jan 25, 2024

Uh oh!

nnethercote commented Jan 29, 2024

Uh oh!

nnethercote commented Jan 30, 2024

Uh oh!

bors commented Jan 30, 2024

oli-obk Jan 29, 2024 •

edited

Loading