More accurate error for binop errors after identifying RHS type #90006

estebank · 2021-10-18T09:29:55Z

Evaluate binops a second time with the resolved RHS type so we can provide suggestions for errors caused by trait predicates that didn't fail when looking for the binop with a type variable in the RHS.

Provide more casting and borrowing suggestions in binops.

Fix #44695, partially address #40660, partially address #57621 (we provide bad suggestion spans when assert_eq! is involved), doesn't account for #52544.

Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Up.20to.20.255.20regression.20on.20kekkak.20with.20.2390006/near/258044068

rust-highfive · 2021-10-18T09:29:58Z

r? @petrochenkov

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

estebank · 2021-10-18T09:30:43Z

@bors try @rust-timer queue

This is doing more work on the happy path (evaluating the binop twice, with a ty var and with a concrete ty). Let's see if I need to rework this for perf's sake.

rust-timer · 2021-10-18T09:30:44Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-10-18T09:30:52Z

⌛ Trying commit c01107c2d7d7e2c47830a49fba5d724ba4ffe7ff with merge 393f6bcfbcbf8ba2089decd734dbf36b4081686c...

bors · 2021-10-18T10:46:53Z

☀️ Try build successful - checks-actions
Build commit: 393f6bcfbcbf8ba2089decd734dbf36b4081686c (393f6bcfbcbf8ba2089decd734dbf36b4081686c)

rust-timer · 2021-10-18T10:46:55Z

Queued 393f6bcfbcbf8ba2089decd734dbf36b4081686c with parent 5e02151, future comparison URL.

rust-timer · 2021-10-18T12:55:35Z

Finished benchmarking commit (393f6bcfbcbf8ba2089decd734dbf36b4081686c): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 5.3% on full builds of keccak)

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

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

estebank · 2021-10-18T16:51:50Z

@bors try @rust-timer queue

rust-timer · 2021-10-18T16:51:51Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-10-18T16:51:59Z

⌛ Trying commit 009cc3d96655794853c7f9fd34f7a3dd517a9e55 with merge 3125063ba74acf6044a7425b21bbfddead7736b5...

bors · 2021-10-18T18:45:21Z

☀️ Try build successful - checks-actions
Build commit: 6dbdb30db6f062a3181014360f5f679d7fb1132d (6dbdb30db6f062a3181014360f5f679d7fb1132d)

rust-timer · 2021-10-18T18:45:23Z

Queued 6dbdb30db6f062a3181014360f5f679d7fb1132d with parent 5dab47d, future comparison URL.

rust-timer · 2021-10-18T20:17:13Z

Finished benchmarking commit (6dbdb30db6f062a3181014360f5f679d7fb1132d): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Moderate improvement in instruction counts (up to -0.7% on incr-patched: b9b3e592dd cherry picked builds of style-servo)
Small regression in instruction counts (up to 0.5% on full builds of piston-image)

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

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

compiler/rustc_resolve/src/late/diagnostics.rs

compiler/rustc_typeck/src/check/op.rs

petrochenkov · 2021-10-19T14:20:36Z

compiler/rustc_typeck/src/check/op.rs

+                                        lhs_ty
+                                    )
+                                } else {
+                                    format!("can't compare `{}` with `{}`", lhs_ty, rhs_ty,)


The "can't compare" wording is used for all comparison operations, so it's not always clear what trait should be implemented exactly, especially if the comparison happens in a macro.
I think it would be more useful to extend the old warning instead:

"binary operation `{op}` cannot be applied to types `{ty1}` and `{ty2}`"

petrochenkov · 2021-10-19T14:23:06Z

src/test/ui/binop/issue-81293.stderr

+   |
+help: you can convert a `u16` to a `usize`
+   |
+LL |     a = c + (b * 5).into();


usize::from(b * 5) was a better suggestion, so if the suggestions are filtered, then from should be given a higher priority.

petrochenkov · 2021-10-19T14:23:39Z

src/test/ui/suggestions/missing-trait-bound-for-op.stderr

@@ -1,4 +1,4 @@
-error[E0369]: binary operation `==` cannot be applied to type `&[T]`
+error[E0369]: can't compare `&[T]` with `&[T]`


Why are these types considered different?

petrochenkov · 2021-10-19T15:18:59Z

compiler/rustc_typeck/src/check/op.rs

@@ -475,11 +553,54 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                    }
                }
                err.emit();
-                self.tcx.ty_error()
+                let ty = self.tcx.ty_error();


Suggested change

let ty = self.tcx.ty_error();

let err_ty = self.tcx.ty_error();

petrochenkov · 2021-10-19T15:20:07Z

compiler/rustc_typeck/src/check/op.rs

@@ -422,6 +484,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                                "*".to_string(),
                                rustc_errors::Applicability::MachineApplicable,
                            );
+                            suggested = true;
+                        } else if let IsAssign::No = is_assign {


Suggested change

} else if let IsAssign::No = is_assign {

} else if is_assign == IsAssign::No {

petrochenkov · 2021-10-19T15:27:18Z

compiler/rustc_typeck/src/check/op.rs

+            if let Ok(method) = result {
+                errors.retain(|e| match e.obligation.predicate.kind().skip_binder() {
+                    ty::PredicateKind::Trait(predicate) => {
+                        // Skip the obligations coming from the binop, we want to emit E0369 instead


"Skip the errors coming from obligations related to the binop, we want to emit ..., these errors must be emitted by the code below."

And the same wording change below.
And maybe factor out this common logic to a closure/function.

petrochenkov · 2021-10-19T15:28:14Z

compiler/rustc_typeck/src/check/op.rs

+                let trait_did = self.op_metadata(Op::Binary(op, is_assign), op.span).1.unwrap();
+                let (obligation, _) =
+                    self.obligation_for_method(op.span, trait_did, lhs_ty, Some(&[rhs_ty]));
+                let mut fulfill = <dyn TraitEngine<'_>>::new(self.tcx);


Why do you need to create a new inference context?

Using the current inference context incurs a huge cost on all binops. This change brought down the perf impact on keccak from ~5% to non-detectable on that crate, and the crates that are affected have under ~.5% impact.

petrochenkov · 2021-10-19T15:33:00Z

compiler/rustc_typeck/src/check/op.rs

+                                    let def_id = predicate.def_id();
+                                    // We also remove obligations for `PartialEq` because
+                                    // `PartialOrd` introduces them and this way we don't get
+                                    // duplicated output.


I don't think this is a good idea, too subtle and special-cased, and I don't see anything ensuring that PartialOrd is involved at all.

petrochenkov · 2021-10-19T15:35:34Z

I would also suggest splitting this into a few more focused PRs, the "running inference twice" part, the "consider &T in addition to T part", and maybe the "everything else" part (but with separate commits per independent change).

(The "running inference twice" part should also get reviewed by someone from traits/inference WG, I'm not sure that it doesn't change the language behavior.)

bors · 2021-10-20T01:47:18Z

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

JohnCSimon · 2022-01-30T04:36:04Z

Ping from triage:
@estebank can you please post the status of this PR?

JohnCSimon · 2022-04-11T01:34:33Z

Ping for triage:
@estebank FYI this PR has sat idle for months.

estebank · 2022-04-11T15:55:31Z

@JohnCSimon yes, I tried rebasing and cleaning it up 2 months ago, but the divergence has gotten big enough that I need to relearn how to accomplish the same effect I had in this PR. I'll try to get back to it in the short term.

Dylan-DPC · 2022-11-22T09:03:06Z

I am closing this as it is bitrotted.

rust-highfive assigned petrochenkov Oct 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 18, 2021

This was referenced Oct 18, 2021

Ergonomics: &String does not implement PartialEq<str> #44695

Closed

Better error messages for &T == T #40660

Closed

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 18, 2021

This comment has been minimized.

Sign in to view

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 18, 2021

This comment has been minimized.

Sign in to view

estebank added 6 commits October 18, 2021 17:06

Evaluate binops a second time with resolved rhs type for suggestions

9d8fb13

Add FIXME comment for follow up work

dc7785c

Suggest replacement of existing bound instead of further contraining

2f499dd

Fix output formatting for numeric cast

2e6cab8

More accurate tracking of cast suggestions

86aeb23

Remove useless comment

56b12d7

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 18, 2021

estebank removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 18, 2021

estebank added 3 commits October 19, 2021 10:18

Use span_suggestion_verbose for uncalled functions

e938489

Remove duplicated errors

777e477

Fix formatting of comment

4797ad9

petrochenkov reviewed Oct 19, 2021

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 14, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2022

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2022

Dylan-DPC closed this Nov 22, 2022

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More accurate error for binop errors after identifying RHS type #90006

More accurate error for binop errors after identifying RHS type #90006

estebank commented Oct 18, 2021 •

edited by petrochenkov

Loading

rust-highfive commented Oct 18, 2021

estebank commented Oct 18, 2021

rust-timer commented Oct 18, 2021

bors commented Oct 18, 2021

bors commented Oct 18, 2021

rust-timer commented Oct 18, 2021

rust-timer commented Oct 18, 2021

This comment has been minimized.

estebank commented Oct 18, 2021

rust-timer commented Oct 18, 2021

bors commented Oct 18, 2021

This comment has been minimized.

bors commented Oct 18, 2021

rust-timer commented Oct 18, 2021

rust-timer commented Oct 18, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

petrochenkov Oct 19, 2021

estebank Oct 20, 2021

petrochenkov Oct 19, 2021

petrochenkov commented Oct 19, 2021 •

edited

Loading

bors commented Oct 20, 2021

JohnCSimon commented Jan 30, 2022

JohnCSimon commented Apr 11, 2022

estebank commented Apr 11, 2022

Dylan-DPC commented Nov 22, 2022

		@@ -1,4 +1,4 @@
		error[E0369]: binary operation `==` cannot be applied to type `&[T]`
		error[E0369]: can't compare `&[T]` with `&[T]`

	let ty = self.tcx.ty_error();
	let err_ty = self.tcx.ty_error();

	} else if let IsAssign::No = is_assign {
	} else if is_assign == IsAssign::No {

More accurate error for binop errors after identifying RHS type #90006

More accurate error for binop errors after identifying RHS type #90006

Conversation

estebank commented Oct 18, 2021 • edited by petrochenkov Loading

rust-highfive commented Oct 18, 2021

estebank commented Oct 18, 2021

rust-timer commented Oct 18, 2021

bors commented Oct 18, 2021

bors commented Oct 18, 2021

rust-timer commented Oct 18, 2021

rust-timer commented Oct 18, 2021

This comment has been minimized.

estebank commented Oct 18, 2021

rust-timer commented Oct 18, 2021

bors commented Oct 18, 2021

This comment has been minimized.

bors commented Oct 18, 2021

rust-timer commented Oct 18, 2021

rust-timer commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 19, 2021 • edited Loading

bors commented Oct 20, 2021

JohnCSimon commented Jan 30, 2022

JohnCSimon commented Apr 11, 2022

estebank commented Apr 11, 2022

Dylan-DPC commented Nov 22, 2022

estebank commented Oct 18, 2021 •

edited by petrochenkov

Loading

petrochenkov commented Oct 19, 2021 •

edited

Loading