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

Add disambiugator to ExpnData #79811

Merged
merged 1 commit into from Jan 24, 2021
Merged

Add disambiugator to ExpnData #79811

merged 1 commit into from Jan 24, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Dec 7, 2020 •
edited

I still need to write a bunch of comments. Opening to see how bad the perf impact is.

cc #79560

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(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 Dec 7, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Trying commit 8dc97629d05ea91a12caaff6a561ef62a66e9a22 with merge bd66e0e098765f4953426b7f85e587966af9f286...

@bors
Copy link
Contributor

bors commented Dec 7, 2020

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

@rust-timer
Copy link
Collaborator

Queued bd66e0e098765f4953426b7f85e587966af9f286 with parent afa995b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (bd66e0e098765f4953426b7f85e587966af9f286): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@Aaron1011
Copy link
Member Author

I think this should actually use HashStable to determine when we need a disambiguation - theoretically, we could end up with two ExpnDatas with distinct parent fields, but which end up getting the same Fingerprint when we recursively hash them. However, we don't have a HashStableContext available when we create an ExpnId, so this could be tricky.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Dec 21, 2020

⌛ Trying commit 1e89e47e75aac5e03dc71de91adda259a0140abf with merge 97601cb25e1809878653c4c47e44e4edd6e49dc0...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 21, 2020

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

@rust-timer
Copy link
Collaborator

Queued 97601cb25e1809878653c4c47e44e4edd6e49dc0 with parent c813545, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 21, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (97601cb25e1809878653c4c47e44e4edd6e49dc0): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 21, 2020
@Aaron1011
Copy link
Member Author

This has a tricky interaction with incremental compilation, and will require some additional thought.

@Aaron1011 Aaron1011 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 Dec 23, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 7, 2021

⌛ Trying commit b5f187c55ffad1a368b84d5da5f6be82f05a8f4a with merge d8272d71cc39db9bc64feeb3ae02f0c2ea7bd572...

@bors
Copy link
Contributor

bors commented Jan 7, 2021

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

@rust-timer
Copy link
Collaborator

Queued d8272d71cc39db9bc64feeb3ae02f0c2ea7bd572 with parent 8f0b945, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 7, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d8272d71cc39db9bc64feeb3ae02f0c2ea7bd572): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 7, 2021
@petrochenkov
Copy link
Contributor

(I probably won't get to this until the next weekend.)

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
@m-ou-se
Rollup merge of rust-lang#80732 - spastorino:trait-inheritance-self2,…
d2009f0 
… r=nikomatsakis

Allow Trait inheritance with cycles on associated types take 2

This reverts the revert of rust-lang#79209 and fixes the ICEs that's occasioned by that PR exposing some problems that are addressed in rust-lang#80648 and rust-lang#79811.
For easier review I'd say, check only the last commit, the first one is just a revert of the revert of rust-lang#79209 which was already approved.

This also could be considered part or the actual fix of rust-lang#79560 but I guess for that to be closed and fixed completely we would need to land rust-lang#80648 and rust-lang#79811 too.

r? `@nikomatsakis`
cc `@Aaron1011`
petrochenkov
petrochenkov reviewed Jan 23, 2021
View reviewed changes
compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/hygiene.rs Show resolved Hide resolved
compiler/rustc_span/src/hygiene.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/hygiene.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/hygiene.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/hygiene.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov 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 23, 2021
@Aaron1011
Copy link
Member Author

@petrochenkov I've addressed your comments

@petrochenkov
Copy link
Contributor

r=me with commits squashed.

@Aaron1011
Add disambiugator to ExpnData
3540f93 
Due to macro expansion, its possible to end up with two distinct
`ExpnId`s that have the same `ExpnData` contents. This violates the
contract of `HashStable`, since two unequal `ExpnId`s will end up with
equal `Fingerprint`s.

This commit adds a `disambiguator` field to `ExpnData`, which is used to
force two otherwise-equivalent `ExpnData`s to be distinct.
@Aaron1011
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 23, 2021

📌 Commit 3540f93 has been approved by petrochenkov

@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 Jan 23, 2021
@bors
Copy link
Contributor

bors commented Jan 24, 2021

⌛ Testing commit 3540f93 with merge 26c2d1f...

@bors
Copy link
Contributor

bors commented Jan 24, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 26c2d1f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2021
@bors bors merged commit 26c2d1f into rust-lang:master Jan 24, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 24, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 9, 2021
@Dylan-DPC
Rollup merge of rust-lang#80732 - spastorino:trait-inheritance-self2,…
44e526b 
… r=nikomatsakis

Allow Trait inheritance with cycles on associated types take 2

This reverts the revert of rust-lang#79209 and fixes the ICEs that's occasioned by that PR exposing some problems that are addressed in rust-lang#80648 and rust-lang#79811.
For easier review I'd say, check only the last commit, the first one is just a revert of the revert of rust-lang#79209 which was already approved.

This also could be considered part or the actual fix of rust-lang#79560 but I guess for that to be closed and fixed completely we would need to land rust-lang#80648 and rust-lang#79811 too.

r? `@nikomatsakis`
cc `@Aaron1011`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Aaron1011 @rust-highfive @rust-timer @bors @petrochenkov @rust-log-analyzer @ecstatic-morse @rustbot