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

Fix SpanlessHash and SpanlessEq tables #2763

Merged
merged 3 commits into from
May 18, 2018

Conversation

phansch
Copy link
Member

@phansch phansch commented May 15, 2018

This makes sure the correct tables are used for constant lookup around SpanlessHash and SpanlessEq

Fixes #2767
Fixes #2594
Fixes #2499
Closes #1783
Fixes #1782

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2018

is in a different body table for some reason:

Oh, it's in a different body table because it's a constant ;). The expression variant (ExprRepeat) has a field of type BodyId. Try calling cx.tcx.body_tables(id) with said id to obtain the correct table

@phansch
Copy link
Member Author

phansch commented May 17, 2018

Thanks! The code slowly starts to make sense to me 👀

8fdf2a8#diff-facf8838686f92b7be75999cfbe7e987R493 fixes the SpanlessHash ICE. However it feels not correct to me, because it will set the table to the table of the constant for the following iterations, which could potentially cause other lookup issues.

And actually it does change the debug output for run-pass/ice-2499 that shows that now the table has been changed to the table that contains the constants and any non-constant item lookup fails :/

@@ -490,6 +490,7 @@ impl<'a, 'tcx: 'a> SpanlessHash<'a, 'tcx> {
let c: fn(_, _) -> _ = ExprRepeat;
c.hash(&mut self.s);
self.hash_expr(e);
self.tables = self.cx.tcx.body_tables(l_id);
self.hash_expr(&self.cx.tcx.hir.body(l_id).value);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can store the old tables in a temporary variable and restore them afterwards. This is essentially how rustc does it for LateContext. We should probably abstract that feature in some generic form so all the cases where we or rustc do it can be made correct in a way that can't be accidentally forgotten.

@phansch phansch force-pushed the tasty_ice_cream branch 2 times, most recently from 1d0105e to a9e09ba Compare May 17, 2018 18:22
@phansch phansch force-pushed the tasty_ice_cream branch 3 times, most recently from c473221 to cb2f507 Compare May 17, 2018 18:59
@phansch phansch changed the title [wip] Fix SpanlessHash and SpanlessEq tables Fix SpanlessHash and SpanlessEq tables May 17, 2018
@phansch phansch force-pushed the tasty_ice_cream branch from cb2f507 to 373033e Compare May 17, 2018 19:06
@phansch phansch force-pushed the tasty_ice_cream branch from 373033e to ed885dc Compare May 17, 2018 19:12
@phansch
Copy link
Member Author

phansch commented May 17, 2018

Ok, I think this is ready for review @oli-obk

@oli-obk oli-obk merged commit dc5a5a4 into rust-lang:master May 18, 2018
@Ameobea
Copy link

Ameobea commented May 18, 2018

Thanks very much for this!

@phansch phansch deleted the tasty_ice_cream branch May 18, 2018 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants