Fix indeterminism in ty::TraitObject representation. #36425

michaelwoerister · 2016-09-12T20:25:05Z

Make sure that projection bounds in ty::TraitObject are sorted in a way that is stable across compilation sessions and crate boundaries.

This PR

moves DefPathHashes up into librustc so it can be used there to create a stable sort key for DefIds,
changes PolyExistentialProjection::sort_key() to take advantage of the above,
and removes the unused PolyProjectionPredicate::sort_key() and ProjectionTy::sort_key() methods.

Fixes #36155

rust-highfive · 2016-09-12T20:25:17Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

arielb1 · 2016-09-12T20:36:50Z

src/librustc/ty/context.rs

@@ -1303,7 +1303,11 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
 }

 pub fn mk_trait(self, mut obj: TraitObject<'tcx>) -> Ty<'tcx> {
- obj.projection_bounds.sort_by(|a, b| a.sort_key().cmp(&b.sort_key()));
+


nit: extra whitespace

eddyb · 2016-09-12T20:46:04Z

@michaelwoerister I believe the extra logic in tyencode can be removed with this change.

alexcrichton · 2016-09-12T20:46:52Z

Thanks for the investigation and fix @michaelwoerister!

michaelwoerister · 2016-09-12T21:16:59Z

I believe the extra logic in tyencode can be removed with this change.

Yes.

michaelwoerister · 2016-09-12T22:32:42Z

I followed @arielb1's suggestion and made TraitDef cache the DefPath hash. No need to move DefPathHashes into librustc anymore. I also removed the sorting logic from tyencode as @eddyb suggested.

eddyb · 2016-09-12T23:06:57Z

@michaelwoerister It might be a good idea at this point to fix TypeId trait object handling and the general TypeId hashing of DefId if you have a working solution, otherwise they risk getting left behind and causing subtle bugs.

michaelwoerister · 2016-09-12T23:12:05Z

@eddyb Can't we just do exactly the same as we do when computing the symbol hash? That seems to work pretty well, so far. I'll look into it tomorrow.

eddyb · 2016-09-12T23:22:31Z

@michaelwoerister The symbol hash is a gross hack that dumps type metadata into strings and hashes that with sha2. The new TypeId hasher is much simpler and more thorough.
I've successfully replaced the symbol hash with a generic version of the TypeId hasher in a WIP branch with no trouble, but I used a stringified version of the DefPath instead of a proper hash.
That part is not very relevant to this PR though.

The immediate result of this PR is that the sorting of trait object projections is no longer necessary, and since I believe that all TyTrait with the same principal trait have the exact same projection bounds, that whole arm can be just:

            TyTrait(ref data) => {
                self.def_id(data.principal.def_id());
                self.hash(data.builtin_bounds);
            }

michaelwoerister · 2016-09-12T23:43:54Z

I don't think it's 'gross' :) It's rather DRY actually (at the expense
of some computational overhead).

Sha2 is not necessary, probably, but how is it more thorough?

On 12.09.2016 19:22, Eduard-Mihai Burtescu wrote:

@michaelwoerister https://github.com/michaelwoerister The symbol
hash is a gross hack that dumps type metadata into strings and hashes
that with sha2. The new |TypeId| hasher is much simpler and more thorough.
I've successfully replaced the symbol hash with a generic version of
the |TypeId| hasher in a WIP branch with no trouble, but I used a
stringified version of the |DefPath| instead of a proper hash.
That part is not very relevant to this PR though.

The immediate result of this PR is that the sorting of trait object
projections

rust/src/librustc/ty/util.rs

Lines 461 to 466 in f81f496

// Only projection bounds are left, sort and hash them.

let mut projection_bounds: Vec<_> = data.projection_bounds

.iter()

.map(|b| (b.item_name().as_str(), b))

.collect();

projection_bounds.sort_by_key(|&(ref name, _)| name.clone());

is no longer necessary, and since I believe that all |TyTrait| with
the same principal trait have the exact same projection bounds, that
whole arm can be just:
         TyTrait(ref  data)=>  {
             self.def_id(data.principal.def_id());
             self.hash(data.builtin_bounds);
         }
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#36425 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvcZg9Dy0bWjxLk1btyfyms9v81MW8lks5qpd7EgaJpZM4J6_gh.

eddyb · 2016-09-12T23:52:19Z

@michaelwoerister I only meant that the new TypeId hasher is more thorough than the old one, sorry.

michaelwoerister · 2016-09-13T00:55:32Z

@eddyb I'd be all for consolidating the various type-id/type-hash implementations we have in the compiler (and get rid of this shit for example :)

…ay that is stable across compilation sessions and crate boundaries.

eddyb · 2016-09-13T19:45:32Z

src/librustc/ty/util.rs

@@ -453,17 +453,6 @@ impl<'a, 'gcx, 'tcx> TypeVisitor<'tcx> for TypeIdHasher<'a, 'gcx, 'tcx> {
 // Hash region and builtin bounds.
 data.region_bound.visit_with(self);


This and data.principal.visit_with(self); above it are unnecessary because automatic recursion will hit them anyway.

michaelwoerister · 2016-09-13T20:30:49Z

r? @eddyb

eddyb · 2016-09-13T20:31:42Z

@bors r+

bors · 2016-09-13T20:31:43Z

📌 Commit 7ec9b81 has been approved by eddyb

bors · 2016-09-14T02:28:02Z

⌛ Testing commit 7ec9b81 with merge fdb4f63...

bors · 2016-09-14T02:33:56Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

alexcrichton · 2016-09-14T05:44:33Z

@bors: retry

On Tue, Sep 13, 2016 at 7:33 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/2489

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36425 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95JIcM3in_wsv7KS34pNWA7pzzicaks5qp10XgaJpZM4J6_gh
.

…bounds, r=eddyb Fix indeterminism in ty::TraitObject representation. Make sure that projection bounds in `ty::TraitObject` are sorted in a way that is stable across compilation sessions and crate boundaries. This PR + moves `DefPathHashes` up into `librustc` so it can be used there to create a stable sort key for `DefId`s, + changes `PolyExistentialProjection::sort_key()` to take advantage of the above, + and removes the unused `PolyProjectionPredicate::sort_key()` and `ProjectionTy::sort_key()` methods. Fixes rust-lang#36155

Rollup of 9 pull requests - Successful merges: #36384, #36405, #36425, #36429, #36438, #36454, #36459, #36461, #36463 - Failed merges: #36444

rust-highfive assigned nrc Sep 12, 2016

arielb1 reviewed Sep 12, 2016
View reviewed changes

michaelwoerister force-pushed the stable-projection-bounds branch from 03cee05 to 338943a Compare September 12, 2016 22:28

michaelwoerister added 5 commits September 13, 2016 15:22

Make sure that projection bounds in ty::TraitObject are sorted in a w…

5a881e9

…ay that is stable across compilation sessions and crate boundaries.

Remove redundant sorting of projection bounds in tyencode.

5c923f0

Remove redundant sorting of projections in TypeIdHasher.

94d7501

Make TypeIdHasher use DefPath::deterministic_hash() for stability.

75a0dd0

TypeIdHasher: Let projections be hashed implicitly by the visitor.

869d144

eddyb reviewed Sep 13, 2016
View reviewed changes

Fix rebasing fallout.

377c3e1

michaelwoerister force-pushed the stable-projection-bounds branch from 5009d71 to 377c3e1 Compare September 13, 2016 19:46

TypeIdHasher: Remove more redundant explicit visit calls.

7ec9b81

rust-highfive assigned eddyb and unassigned nrc Sep 13, 2016

Manishearth mentioned this pull request Sep 15, 2016

Rollup of 9 pull requests #36491

Merged

bors added a commit that referenced this pull request Sep 15, 2016

Auto merge of #36491 - Manishearth:rollup, r=Manishearth

dc75933

Rollup of 9 pull requests - Successful merges: #36384, #36405, #36425, #36429, #36438, #36454, #36459, #36461, #36463 - Failed merges: #36444

bors merged commit 7ec9b81 into rust-lang:master Sep 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix indeterminism in ty::TraitObject representation. #36425

Fix indeterminism in ty::TraitObject representation. #36425

michaelwoerister commented Sep 12, 2016

rust-highfive commented Sep 12, 2016

arielb1 Sep 12, 2016

eddyb commented Sep 12, 2016

alexcrichton commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016 •

edited

Loading

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016

michaelwoerister commented Sep 13, 2016

eddyb Sep 13, 2016

michaelwoerister commented Sep 13, 2016

eddyb commented Sep 13, 2016

bors commented Sep 13, 2016

bors commented Sep 14, 2016

bors commented Sep 14, 2016

alexcrichton commented Sep 14, 2016

		@@ -453,17 +453,6 @@ impl<'a, 'gcx, 'tcx> TypeVisitor<'tcx> for TypeIdHasher<'a, 'gcx, 'tcx> {
		// Hash region and builtin bounds.
		data.region_bound.visit_with(self);

Fix indeterminism in ty::TraitObject representation. #36425

Fix indeterminism in ty::TraitObject representation. #36425

Conversation

michaelwoerister commented Sep 12, 2016

rust-highfive commented Sep 12, 2016

arielb1 Sep 12, 2016

Choose a reason for hiding this comment

eddyb commented Sep 12, 2016

alexcrichton commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016 • edited Loading

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016

michaelwoerister commented Sep 12, 2016

eddyb commented Sep 12, 2016

michaelwoerister commented Sep 13, 2016

eddyb Sep 13, 2016

Choose a reason for hiding this comment

michaelwoerister commented Sep 13, 2016

eddyb commented Sep 13, 2016

bors commented Sep 13, 2016

bors commented Sep 14, 2016

bors commented Sep 14, 2016

alexcrichton commented Sep 14, 2016

eddyb commented Sep 12, 2016 •

edited

Loading