Some assorted region hashing fixes.

[michaelwoerister]

This PR contains three changes.

1. It changes what we implement HashStable for. Previously, the trait was implemented for things in the local TyCtxt. That was OK, since we only invoked hashing with a TyCtxt<'_, 'tcx, 'tcx> where there is no difference. With query result hashing this becomes a problem though. So we now implement HashStable for things in 'gcx.
2. The PR makes the regular HashStable implementation not anonymize late-bound regions anymore. It's a waste of computing resources and it's not clear that it would always be correct to do so.
3. The PR adds an option for stable hashing to treat all regions as erased and uses this new option when computing the TypeId. This should help with Tracking issue for non_static_type_id #41875.

I did not add a test case for (3) since that's not possible yet. But it looks like @zackmdavis has something in the pipeline there :).

r? @eddyb

[eddyb]

not anonymize late-bound regions anymore. It's a waste of computing resources and it's not clear that it would always be correct to do so.

This will break for<'a> fn(&'a T) -> &'a U sharing a TypeId with for<'b> fn(&'b T) -> &'b Uwhich is a feature. But it's only a TypeId feature, I'm worried about conflating it with HashStable.

[michaelwoerister]

This will break for<'a> fn(&'a T) -> &'a U sharing a TypeId with for<'b> fn(&'b T) -> &'b U

No it shouldn't because all regions are erased when computing the TypeId.

[michaelwoerister]

But maybe I read the corresponding RFC wrong and TypeId should act as if everything was anonymized instead of erased?

[eddyb]

@michaelwoerister Are you saying for<'a> fn(&'a T) -> &'a U and fn(&'static T) -> &'static U have the same TypeId? That means they can be transmuted by Any...

[michaelwoerister]

I see that that's a problem -- yet the assert_eq!(TypeId::of::<ContainsRef<'a>>(), TypeId::of::<ContainsRef<'static>>()) from the RFC thread suggests that they should be treated the same. I kind of don't get why we want to allow non-static lifetimes in type_id() anyway.

[eddyb]

@michaelwoerister That's completely different from for<'a> lifetimes (which shouldn't get erased).

[michaelwoerister]

That's completely different from for<'a> lifetimes

But if we did not have the 'static bound on Any, it could lead to the same kind of problems, right? That is, put in a ContainsRef<'a> and take out a ContainsRef<'static>. Or am I missing something here?

[eddyb]

@michaelwoerister Correct. for<'a> fn(&'a T) -> &'a U is 'static and rightly so.

[michaelwoerister]

I changed the last commit to just call tcx.erase_regions() before computing the type-id hash. That should have the desired effects: erase free regions, anonymize bound regions.

[eddyb]

@michaelwoerister I still see a removed anonymize_late_bound_regions call.

[michaelwoerister]

@eddyb Yes, the one in impls_ty.rs is removed intentionally. In the general case it might not be valid to anonymize. Once we start caching these things, I rather want these hashes to be accurate and there's no real need to optimize for a case where someone changes region names.

[eddyb]

@michaelwoerister Okay can you make sure we still produce the same TypeId (and add a test if one is missing) for fn(for<'a> fn(&'a T) -> &'a U) and fn(for<'b> fn(&'b T) -> &'b U)?
IIRC anonymize_late_bound_regions doesn't handle nesting.

[michaelwoerister]

Done!

[eddyb]

r=me I guess (if the test passes)

[michaelwoerister]

@bors r=eddyb

Thanks, @eddyb!

[bors]

📌 Commit 6dbd846 has been approved by eddyb

[bors]

⌛ Testing commit 6dbd846 with merge 73c3f55...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 73c3f55 to master...