Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 16, 2022

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Apr 27, 2022

This PR is a follow-on to #94034 that addresses #96442. That is, after replacing the trait-suggestion logic in op.rs with a more generic path that analyzes a general set of Obligations, 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 rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 27, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2022
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Trying commit 6fe3b1debd3b44413c657a0b1dba37d553d7a526 with merge 2142253261d42f925cbaf34fe261e47feea3b67b...

@willcrichton
Copy link
Contributor Author

(ah I missed some tests, will fix those later today)

@compiler-errors
Copy link
Member

retrying perf run, pushes will cause it to cancel

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Trying commit e9ecae53d06562c178dfb3a81d298f5c167639b9 with merge f07f0f43ceab58a4ed0051bac441d2d07d6571e2...

obligations.push(traits::Obligation::new(
cause.clone(),
self.param_env,
ty::Binder::dummy(ty::PredicateKind::Projection(ProjectionPredicate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only worry is that registering this projection obligation will cause inference to break in certain places, since it asserts type equality as opposed to coercibility.

@@ -511,6 +516,31 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};
obligations.extend(traits::predicates_for_generics(cause.clone(), self.param_env, bounds));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible to stash the type from the expectation in the BinOp cause-code then edit the code that suggests "add std::ops::Mul" to add the associated type there, in case inference is broken by the current change (or if the perf hit is too massive). I'll try to think about if either is a really valid worry, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case inference is broken by the current change

I guess not necessarily, since we're only applying this when we have Expectation::ExpectHasType, not the other ones which are there to guide coercion... (I think)

@@ -511,6 +516,31 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};
obligations.extend(traits::predicates_for_generics(cause.clone(), self.param_env, bounds));

// If we're producing obligations for an operator trait, and it has an `Output`
// associated type, then we add an `Output=expected` obligation.
if let (true, Expectation::ExpectHasType(output_ty)) = (is_op, expected) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: let chains can make this more readable, and Expectation::only_has_type might be useful instead of matching on the variant itself.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 27, 2022

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

@rust-timer
Copy link
Collaborator

Queued f07f0f43ceab58a4ed0051bac441d2d07d6571e2 with parent ff18038, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f07f0f43ceab58a4ed0051bac441d2d07d6571e2): comparison url.

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

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

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@estebank
Copy link
Contributor

I'm not too happy with the increase in verbosity, but the perf impact isn't massive and we might be able to work things out so that we don't regress as much in the output.

error[E0271]: type mismatch resolving `<! as std::ops::Not>::Output == bool`
    |
    |
239 |     if !return () {}
    |        ^^^^^^^^^^ expected `!`, found `bool`
    = note: expected type `!`
               found type `bool`

This is a problem, though :(

I think this suggestion might be a more straightforward approach to get the same effect we're looking for.

@willcrichton
Copy link
Contributor Author

willcrichton commented Apr 28, 2022

Yeah I think my original strategy won't work if it's changing type-checking results. I tried what @compiler-errors suggested of carrying additional data in the obligation cause, and passing that through to the suggestion code. Take a look and let me know if the implementation seems reasonable. It fixes the specific issue in missing-bounds.rs while not breaking anything else.

The hackiest part is the code that generates and appends the string <Type=Expected> to the trait constraint. I'm not sure if there's a better way to do that.

@bors
Copy link
Contributor

bors commented May 18, 2022

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

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after rebase (ping me to approve)

Comment on lines +482 to +516
if !errors.is_empty() {
for error in errors {
if let Some(trait_pred) =
error.obligation.predicate.to_opt_poly_trait_pred()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid the Vec allocation, right? Good idea.

@@ -504,6 +525,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
rhs_span: opt_input_expr.map(|expr| expr.span),
is_lit: opt_input_expr
.map_or(false, |expr| matches!(expr.kind, hir::ExprKind::Lit(_))),
output_pred: None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary for this PR: I wonder if we can provide the predicate somehow here 🤔

@estebank
Copy link
Contributor

Apologies for the delay in reviewing. If you can squash the commits, that would also be useful.

@estebank estebank 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 Jun 2, 2022
@estebank
Copy link
Contributor

estebank commented Jun 2, 2022

Ping :)

@willcrichton
Copy link
Contributor Author

Ah apologies for the delay in the response to the review! Too busy graduating. I will get to this by the weekend.

@estebank
Copy link
Contributor

estebank commented Jun 2, 2022

@willcrichton no hurries! Congratulations!

@JohnCSimon
Copy link
Member

ping from triage:
@willcrichton -

estebank commented on May 18
Apologies for the delay in reviewing. If you can squash the commits, that would also be useful.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@willcrichton willcrichton force-pushed the fix-trait-suggestion-for-binops branch from c40c425 to b61f8ba Compare July 6, 2022 20:29
@willcrichton
Copy link
Contributor Author

Sorry again for the delays. I just rebased, and found a new regression. Whatever code is producing the suggestion is creating an incorrect syntax combining the associated type bound with an existing trait parameter. See: https://github.com/rust-lang/rust/pull/96482/files#diff-b4e22a434e69c155966cdab3f3b6ae44922478447624ba038055ca32f1b3a50f

I will try to debug this soon.

@bors
Copy link
Contributor

bors commented Jul 12, 2022

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

Comment on lines 11 to 12
LL | fn add_ten<N: std::ops::Add<i32><Output=N>>(n: N) -> N {
| ++++++++++++++++++++++++++++++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can't figure this one out, let's clean up for merge and open a follow up ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured out the issue. I added a fix, although it duplicates some code as you can see in the commit and the FIXME. If you want me to figure out a way to factor out the duplicated code I can do that, otherwise the PR is ready I think.

@willcrichton willcrichton force-pushed the fix-trait-suggestion-for-binops branch from ba5f969 to 2f15dfa Compare July 16, 2022 01:06
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit 2f15dfa has been approved by estebank

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2022
@bors
Copy link
Contributor

bors commented Jul 16, 2022

⌛ Testing commit 2f15dfa with merge d695a49...

@bors
Copy link
Contributor

bors commented Jul 16, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 16, 2022
@bors bors merged commit d695a49 into rust-lang:master Jul 16, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 16, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d695a49): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
2.9% 2.9% 1
Regressions 😿
(secondary)
2.9% 4.3% 3
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.9% 2.9% 1

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
5.8% 5.8% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

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

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants