Fix incorrect suggestion for trait bounds involving binary operators #94034

willcrichton · 2022-02-15T22:52:47Z

This PR fixes #93927, #92347, #93744 by replacing the bespoke trait-suggestion logic in op.rs with a more common code path.

The downside is that this fix causes some suggestions to not include an Output= type, reducing their usefulness.

Note that this causes one case in the missing-bounds.rs test to fail rustfix. So I would need to move that code into a separate non-fix test if this PR is otherwise acceptable.

rust-highfive · 2022-02-15T22:52:50Z

r? @cjgillot

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

oli-obk · 2022-02-15T23:01:17Z

r? @oli-obk

cc @rust-lang/wg-diagnostics

oli-obk · 2022-02-15T23:08:50Z

compiler/rustc_typeck/src/check/op.rs

+                            .collect::<Vec<_>>();
+                        if !predicates.is_empty() {
+                            for pred in predicates {
+                                self.infcx.suggest_restricting_param_bound(


I think we should teach this function about associated type bounds. You could filter all projection predicates for one that is for the current trait and type and if there is such a projection predicate, extend the suggestion with the assoc bound

One issue I'm encountering. I don't think there's a sound way to determine what the appropriate Output type should be. The function check_overloaded_binop attempts to do this, but it can only compute return_ty if the initial lookup_op_method returns a MethodCallee, which it does not in the case being addressed.

Is there some possibility here that I'm missing?

I'm hoping that the correct associated type bound is in the errors list. Try dumping the entire list and checking if it contains PredicateKind::Projection that match the pred in the loop here.

Yes I've checked, and a Projection predicate does not already exist. This is because the relevant predicates are generated by the function obligation_for_method. That function would have to be extended to produce a Projection predicate, except it can only do so if it knows about the expected associated types of a method, which it cannot in this case.

I think I made some related changes in https://github.com/rust-lang/rust/pull/90006/files, which won't be landing until I manage to get rid of the perf regression.

oli-obk · 2022-02-15T23:10:22Z

src/test/ui/issues/issue-35668.stderr

@@ -6,10 +6,10 @@ LL |     a.iter().map(|a| a*a)
   |                      |
   |                      &T
   |
-help: consider restricting type parameter `T`
+help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement


Preexisting, but this message is too vague. We should probably check all occurrences in our ui tests and see if we can be more targetted (or at least change the wording)

estebank · 2022-02-16T18:47:34Z

The downside is that this fix causes some suggestions to not include an Output= type, reducing their usefulness.

Note that this causes one case in the missing-bounds.rs test to fail rustfix. So I would need to move that code into a separate non-fix test if this PR is otherwise acceptable.

Do we suggest the right Output in the new error? If we still lead the user in the right direction, only with an extra iteration, then that's fine.

estebank · 2022-02-16T18:49:19Z

src/test/ui/issues/issue-35668.stderr

+LL | fn func<'a, T>(a: &'a [T]) -> impl Iterator<Item=&'a T> where &T: Mul<&T> {
+   |                                                         +++++++++++++++++


Is this suggestion change correct?

Yes, albeit underconstrained on Output. It actually fixes the previously incorrect suggestion of T: std::ops::Mul<Output = f64>. In that case, Rhs = Self means only T * T is valid, whereas in this case you specifically need T * f64 here.

Edit: I got mixed up. resolution-in-overloaded-op.stderr is what I was referring to.

This suggestion is still correct, but underconstrained by not suggesting Output.

willcrichton · 2022-02-16T18:54:30Z

Do we suggest the right Output in the new error? If we still lead the user in the right direction, only with an extra iteration, then that's fine.

No, because I don't think there is a sound way to guess what Output should be as mentioned here: #94034 (comment)

bors · 2022-02-25T03:53:38Z

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

Dylan-DPC · 2022-04-25T18:51:11Z

@willcrichton
can you rebase, resolve the CI failure and provide an update on this pr? thanks

to fix incorrect suggestion for trait bounds involving binary operators. Fixes rust-lang#93927, rust-lang#92347, rust-lang#93744.

estebank · 2022-04-26T01:28:15Z

r=me after rebasing

willcrichton · 2022-04-26T02:15:58Z

Ok, I have rebased / fixed and also updated the tests such that everything now passes under this PR.

willcrichton · 2022-04-26T02:51:44Z

@rustbot ready

Dylan-DPC · 2022-04-26T02:59:12Z

@bors r=estebank

bors · 2022-04-26T02:59:13Z

📌 Commit dc41dba has been approved by estebank

bors · 2022-04-26T07:29:26Z

⌛ Testing commit dc41dba with merge d6a57d3...

bors · 2022-04-26T10:13:59Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing d6a57d3 to master...

rust-timer · 2022-04-26T11:29:35Z

Finished benchmarking commit (d6a57d3): comparison url.

Summary:

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	1	0	0	0	1
mean²	0.3%	N/A	N/A	N/A	0.3%
max	0.3%	N/A	N/A	N/A	0.3%

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

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

estebank · 2022-04-26T15:44:02Z

src/test/ui/generic-associated-types/missing-bounds.stderr

-LL | impl<B: std::ops::Add<Output = B>> Add for D<B> {
-   |       +++++++++++++++++++++++++++
+LL | impl<B: std::ops::Add> Add for D<B> {
+   |       +++++++++++++++


Note to self: check if we can get this back.

…-binops, r=estebank Add Output = expected type trait obligation for known binary operators This PR is a follow-on to rust-lang#94034 that addresses rust-lang#96442. That is, after replacing the trait-suggestion logic in `op.rs` with a more generic path that analyzes a general set of `Obligation`s, then we lost some specificity in the suggestions where the bounds on the associated type `Output=` would not get suggested. This PR fixes this issue by changing `FnCtxt::construct_obligation_for_trait` to include a new `ProjectionPredicate` obligation for binary operators that obliges that `Output` is the same as the expected type of the expression. Additionally, to get the expected type of the expression, this PR threads the `Expectation<'tcx>` structure throughout several functions. See src/test/ui/generic-associated-types/missing-bounds.stderr for an example of how this works. One side effect of this change is it causes type-check failures with binops to include additional information. Specifically, many now say ``` error: type mismatch resolving `<Lhs as TheBinop>::Output == ExpectedTy` ``` It's up for discussion whether this added context is worth it to the user. r? `@estebank`

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 15, 2022

rust-highfive assigned cjgillot Feb 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2022

rust-highfive assigned oli-obk and unassigned cjgillot Feb 15, 2022

oli-obk reviewed Feb 15, 2022

View reviewed changes

estebank reviewed Feb 16, 2022

View reviewed changes

compiler-errors mentioned this pull request Mar 19, 2022

Add known-bug for #95034 #95102

Merged

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 24, 2022

Add known-bug for rust-lang#94034

d5a32d8

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022

Dylan-DPC 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 Apr 25, 2022

Replace suggest_constraining_param with suggest_restricting_param_bound

4d0fe27

to fix incorrect suggestion for trait bounds involving binary operators. Fixes rust-lang#93927, rust-lang#92347, rust-lang#93744.

Update unop path, fix tests

dc41dba

willcrichton force-pushed the fix-trait-suggestion-for-binops branch from f79c5d3 to dc41dba Compare April 26, 2022 02:14

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

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 Apr 26, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 26, 2022

bors merged commit d6a57d3 into rust-lang:master Apr 26, 2022

rustbot added this to the 1.62.0 milestone Apr 26, 2022

estebank reviewed Apr 26, 2022

View reviewed changes

estebank mentioned this pull request Apr 26, 2022

More accurate suggestion for missing binop trait bounds #96442

Open

willcrichton mentioned this pull request Apr 27, 2022

Add Output = expected type trait obligation for known binary operators #96482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect suggestion for trait bounds involving binary operators #94034

Fix incorrect suggestion for trait bounds involving binary operators #94034

willcrichton commented Feb 15, 2022 •

edited

Loading

rust-highfive commented Feb 15, 2022

oli-obk commented Feb 15, 2022

oli-obk Feb 15, 2022

willcrichton Feb 16, 2022

oli-obk Feb 16, 2022

willcrichton Feb 16, 2022

estebank Feb 16, 2022

oli-obk Feb 15, 2022

estebank commented Feb 16, 2022

estebank Feb 16, 2022

willcrichton Feb 16, 2022 •

edited

Loading

willcrichton commented Feb 16, 2022

bors commented Feb 25, 2022

Dylan-DPC commented Apr 25, 2022

estebank commented Apr 26, 2022

willcrichton commented Apr 26, 2022

willcrichton commented Apr 26, 2022

Dylan-DPC commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

estebank Apr 26, 2022

		LL \| fn func<'a, T>(a: &'a [T]) -> impl Iterator<Item=&'a T> where &T: Mul<&T> {
		\| +++++++++++++++++

Fix incorrect suggestion for trait bounds involving binary operators #94034

Fix incorrect suggestion for trait bounds involving binary operators #94034

Conversation

willcrichton commented Feb 15, 2022 • edited Loading

rust-highfive commented Feb 15, 2022

oli-obk commented Feb 15, 2022

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

estebank commented Feb 16, 2022

Choose a reason for hiding this comment

willcrichton Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

willcrichton commented Feb 16, 2022

bors commented Feb 25, 2022

Dylan-DPC commented Apr 25, 2022

estebank commented Apr 26, 2022

willcrichton commented Apr 26, 2022

willcrichton commented Apr 26, 2022

Dylan-DPC commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

Footnotes

Choose a reason for hiding this comment

willcrichton commented Feb 15, 2022 •

edited

Loading

willcrichton Feb 16, 2022 •

edited

Loading