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

rewrite error handling for unresolved inference vars #89862

Merged
merged 7 commits into from
Jun 3, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Oct 13, 2021

Pretty much completely rewrites fn emit_inference_failure_err.

This new setup should hopefully be easier to extend and is already a lot better when looking for generic arguments.
Because this is a rewrite there are still some parts which are lacking, these are tracked in #94483 and will be fixed in later PRs.

r? @estebank @petrochenkov

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

This comment has been minimized.

Comment on lines 1 to 10
error[E0282]: type annotations needed
--> $DIR/fn-needing-specified-return-type-param.rs:3:13
|
LL | let _ = f;
| - ^ cannot infer type for type parameter `A` declared on the function `f`
| |
| consider giving this pattern the explicit type `fn() -> A`, where the type parameter `A` is specified
| ^ cannot infer type of the type parameter `A` declared on the function `f`
|
help: consider specifying the generic argument
|
LL | let _ = f::<A>;
| +++++
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with most of the results, but we do lose the special casing of assignments, where the stylistic solution is to give the binding a type, instead of leveraging the turbofish.

That shouldn't be a big problem, and we could remove a bunch of suggestion code by only relying on the new logic, but idealy it'd be nice if we could keep that.

I'll take a closer look at the logic later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, prioritizing assignments seems like the right choice, I am a bit torn however, because for
let x = returns_complex_type() i would prefer to suggest let x = returns_complex_type::<T>() instead of let x: SomeComplex<Type<T>, T>> = returns_complex_type(). Maybe adding weights to each option, and prioritizing locals and closure parameters over generic arguments which are prioritized over closure return types or sth 🤔

Comment on lines 7 to 10
help: consider specifying the generic arguments
|
LL | let x = |a: (), b: ()| -> Result<(), _> {
| ++++++++++++++++
LL | Ok::<T, E>(b)
| ++++++++
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 another case of what I was mentioning: The current suggestion is closer to what I'd want to recommend users do, and T is already figured out (thanks to b), the E is the one that needs clarification.

Copy link
Contributor Author

@lcnr lcnr Oct 14, 2021

Choose a reason for hiding this comment

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

I quite dislike explicitly mentioning closure return types, so I would use Ok::<(); u32> here.

I considered (at least partially) using the inferred generic args for the suggestion, but i don't think we always want to do this because sometimes the inferred types can be quite large. There's definitely quite a few possible improvements here however ^^ took me a while to understand how this code works and think that there are some pretty big wins here.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the changes I'm making in #89427, we might be able to be explicit about the potentially usable types because we'll know all the impls that apply. And we already know that E is the problematic one, so we can suggest Ok::<_, SomeE>. Having said that, that doesn't need to be addressed yet, I think.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the path-generics-diagnostics branch 4 times, most recently from 78d5028 to d42c7ab Compare October 16, 2021 09:27
@JohnCSimon 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 Oct 31, 2021
@JohnCSimon 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 Dec 5, 2021
@pnkfelix pnkfelix 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 Jan 6, 2022
@JohnCSimon 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
Copy link
Member

ping from triage:
@lcnr what is the status of this PR?

@lcnr
Copy link
Contributor Author

lcnr commented Jan 31, 2022

it's blocked on me taking a full day to work on this, should happen at some point during february 😆

@lcnr lcnr mentioned this pull request Feb 14, 2022
@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Testing commit b343a46 with merge c5fda9cb3b4a0952fd3db63a3552dde0a1cba9b8...

@bors
Copy link
Contributor

bors commented Jun 3, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-nopt failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@lcnr
Copy link
Contributor Author

lcnr commented Jun 3, 2022

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2022
@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Testing commit b343a46 with merge e40d5e8...

@bors
Copy link
Contributor

bors commented Jun 3, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2022
@bors bors merged commit e40d5e8 into rust-lang:master Jun 3, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e40d5e8): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
0.2% 0.3% 7
Regressions 😿
(secondary)
0.4% 1.0% 23
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 0.2% 0.3% 7

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
3.6% 4.4% 2
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 3.6% 4.4% 2

Cycles

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

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@rylev
Copy link
Member

rylev commented Jun 7, 2022

@lcnr @estebank looks like this caused a perf regression. It's minor enough in primary benchmarks, but still worth looking into.

I ran cache grind diff and saw that ObligationForest::process_obligations is getting called a lot more. I'm very unfamiliar with trait resolution, so I'm unsure if this is a red herring or not. In any case, here is the full diff.

@lcnr lcnr deleted the path-generics-diagnostics branch June 7, 2022 14:57
Comment on lines +986 to +990
Node::PathSegment(seg) => {
let ident_span = seg.ident.span;
ident_span
.with_hi(seg.args.map_or_else(|| ident_span.hi(), |args| args.span_ext.hi()))
}
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 would guess that the only thing which may impact the amount of process_obligations would be this.

We now use a different span from seg.ident which may mean that we don't dedup obligations as readily as they now have different spans? Can try to take a deeper look at this in the near future

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. perf-regression Performance regression. 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.