Uh oh!
There was an error while loading. Please reload this page.
[WIP] rustc: avoid checking Trait's predicates for WF(<T as Trait>::X). - #66020
Closed
eddyb wants to merge 1 commit into
Closed
[WIP] rustc: avoid checking Trait's predicates for WF(<T as Trait>::X).#66020eddyb wants to merge 1 commit into
eddyb wants to merge 1 commit into
Conversation
rust-highfive
commented
Nov 1, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
JohnCSimon
commented
Nov 9, 2019
Ping from triage: cc: @nikomatsakis Thanks! |
eddyb
commented
Nov 9, 2019
ContributorAuthor
Sorry, this is waiting for a review, I should've mentioned I didn't fully address the test failures because it's not clear what the correct outcome is. |
JohnCSimon
commented
Nov 16, 2019
Ping from triage, it's been a week since this was last touched. Thanks! |
nikomatsakis
commented
Nov 22, 2019
Contributor
My take is this: This may well be sound but it's not obviously so. I would prefer to look at steps to avoid merging the where clauses from associated types into the trait listing -- I believe that would have a similar effect to this PR and just be generally less work overall (and a closer match to what chalk does). However, such a change might have some subtle effects on very strange code. Notably, I imagine that if a user wrote this: traitFoowhereSelf::Bar:Display{typeBar;}that is today probably equivalent to traitFoo{typeBar:Display;}but under the most straightforward version of the code I am imagining, it would no longer be (although, now that I think about it, I think that implied bounds in chalk might ultimately judge them to be equivalent regardless). |
eddybforce-pushed
the
wf-projection-speedup
branch
from
November 26, 2019 20:09
8807a70 to
7a82867Compareeddyb
commented
Nov 26, 2019
ContributorAuthor
While I don't expect this to land, I am curious how perf would measure the impact. @bors try |
bors
commented
Nov 26, 2019
Collaborator
⌛ Trying commit 7a82867 with merge fad7de5f0bbacf986f3cf2baa8006b977b6e2107... |
eddyb
commented
Nov 26, 2019
ContributorAuthor
@rust-timer queue |
rust-timer
commented
Nov 26, 2019
Collaborator
Awaiting bors try build completion |
rust-highfive
commented
Nov 26, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
commented
Nov 26, 2019
Collaborator
☀️ Try build successful - checks-azure |
rust-timer
commented
Nov 26, 2019
Collaborator
Queued fad7de5f0bbacf986f3cf2baa8006b977b6e2107 with parent 797fd92, future comparison URL. |
rust-timer
commented
Nov 27, 2019
Collaborator
Finished benchmarking try commit fad7de5f0bbacf986f3cf2baa8006b977b6e2107, comparison URL. |
ContributorAuthor
At a first glance, that might be so, but consider that now every place, which used One of those places is the very one modified by this PR! So moving associated type bounds off of the trait |
This was referenced Nov 27, 2019
bors added a commit
that referenced
this pull request
Nov 28, 2019
[WIP] [DO NOT MERGE] combine #66020 and #66821. That is, the two fixes for #65510, and only for perf testing purposes. The fact that they both work to a comparable extent, while touching different parts of the trait system, made me curious if there would be any gains from having both. r? @nikomatsakis
nikomatsakis
commented
Dec 2, 2019
Contributor
Yes, I agree that this would be a larger change, though it wouldn't concern me for soundness per se. I might be concerned about bits of code that used to compile no longer doing so. |
hdhoang
commented
Dec 12, 2019
Contributor
ping from triage, @eddyb any update after niko's comment? Should we try a crater run or so? |
eddyb
commented
Dec 12, 2019
ContributorAuthor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#65510 - specifically, this is the WF disabling patch mentioned in #65510 (comment).
This is marked WIP to avoid merging it without serious scrutiny, since there's a chance this is unsound.
In case this doesn't work out, we can probably try caching proving WF obligations (perhaps when no type/const inference variables are involved? in general we could try to use queries more).
r? @nikomatsakis