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.

Sign up for GitHub

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

Jump to bottom

Point at overlapping impls when type annotations are needed #89427

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Point at overlapping impls when type annotations are needed #89427

merged 2 commits into from
Oct 25, 2021
+661 −116

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Oct 1, 2021 •
edited
Loading

Address #89254.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2021
@estebank
Copy link
Contributor Author

estebank commented Oct 1, 2021

@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 Oct 1, 2021
@bors
Copy link
Contributor

bors commented Oct 1, 2021

⌛ Trying commit ce9b15e5fe151c352daef9a952be011112cfe881 with merge 96d6dfc04cc7ea021ad7b494bfbb0041be4e083c...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the collect-overlapping-impls branch from ce9b15e to a6b9719 Compare October 1, 2021 13:23
@estebank
Copy link
Contributor Author

estebank commented Oct 1, 2021

@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 Oct 1, 2021

⌛ Trying commit a6b97196c4c1ff8acf22d3b2f27848de9c325a4e with merge 19148e7dc43cf0314f81a60dd944631ca719942c...

@petrochenkov
Copy link
Contributor

r? @rust-lang/wg-traits

@petrochenkov
Copy link
Contributor

It didn't work, selecting a random active member.
r? @spastorino

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued 19148e7dc43cf0314f81a60dd944631ca719942c with parent 69eb996, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (19148e7dc43cf0314f81a60dd944631ca719942c): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.7% on full builds of diesel)

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.

@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 Oct 1, 2021
@jackh726
Copy link
Member

jackh726 commented Oct 1, 2021

It didn't work, selecting a random active member.

Yeah, should be "rust-lang/traits", but it might be prudent to make "wg-traits" an alias for that.

@estebank estebank changed the title [wip] Point at overlapping impls when type annotations are needed Point at overlapping impls when type annotations are needed Oct 4, 2021
@estebank estebank force-pushed the collect-overlapping-impls branch from 7c1f014 to 4a85552 Compare October 4, 2021 12:17
nikomatsakis
nikomatsakis reviewed Oct 4, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

this looks good to me! I didn't give it a detailed read, though, so maybe better for @spastorino or others to look a touch more closely at the code.

@bors
Copy link
Contributor

bors commented Oct 8, 2021

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

@estebank estebank force-pushed the collect-overlapping-impls branch from 4a85552 to cb807e7 Compare October 13, 2021 14:50
@estebank
Copy link
Contributor Author

@spastorino ping

@jackh726 jackh726 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2021
estebank
estebank commented Oct 24, 2021
View reviewed changes
src/test/ui/traits/multidispatch-convert-ambig-dest.stderr
Comment on lines +28 to +31
help: consider specifying the type arguments in the function call
|
LL | test::<T, U>(22, std::default::Default::default());
| ++++++++
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we should try and figure out that T is a type we can infer, while U is not, for a more targeted suggestion.

@estebank estebank force-pushed the collect-overlapping-impls branch from cb807e7 to 8c2bed8 Compare October 24, 2021 16:38
@estebank
Copy link
Contributor Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Oct 24, 2021

📌 Commit 8c2bed87beb9558feaa310a2dc8d93f67edf2705 has been approved by jackh726

@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 Oct 24, 2021
@bors
Copy link
Contributor

bors commented Oct 24, 2021

⌛ Testing commit 8c2bed87beb9558feaa310a2dc8d93f67edf2705 with merge 3296562b6104d82890a18c6ce7073df9032be3f9...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 24, 2021

💔 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 Oct 24, 2021
@estebank estebank force-pushed the collect-overlapping-impls branch from 8c2bed8 to 881a50c Compare October 24, 2021 20:28
@estebank
Copy link
Contributor Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Oct 24, 2021

📌 Commit 881a50c has been approved by jackh726

@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 Oct 24, 2021
@bors
Copy link
Contributor

bors commented Oct 24, 2021

⌛ Testing commit 881a50c with merge 41d8c94...

@bors
Copy link
Contributor

bors commented Oct 25, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 41d8c94 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2021
@bors bors merged commit 41d8c94 into rust-lang:master Oct 25, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 25, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (41d8c94): comparison url.

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

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

@rustbot label: -perf-regression

@spastorino spastorino mentioned this pull request Oct 25, 2021
Open
@JakobDegen JakobDegen mentioned this pull request Oct 27, 2021
Merged
@SNCPlay42 SNCPlay42 mentioned this pull request Jan 29, 2022
Closed
@estebank estebank deleted the collect-overlapping-impls branch November 9, 2023 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis left review comments

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

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
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.