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

Rewrite TypeId computation to not miss anything and work cross-crate. #35267

Merged
merged 1 commit into from
Aug 6, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 3, 2016

Fixes #33703 and also some soundness holes in Any due to TypeId ignoring most lifetimes.

@rust-highfive
Copy link
Collaborator

r? @nrc

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

@nrc
Copy link
Member

nrc commented Aug 3, 2016

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nrc Aug 3, 2016
@@ -525,6 +393,142 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

struct TypeIdHasher<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
tcx: TyCtxt<'a, 'gcx, 'tcx>,
svh: &'a Svh,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should just be hashing the crate-name/crate-discriminant pair, rather than employing the SVH.

@nikomatsakis
Copy link
Contributor

@eddyb

That and not hashing the SVH would mean that TypeId is stable, which I guess is needed for incremental recompilation, but a bit scary.

Why is that scary? It seems... right to me. The TypeId is basically just the "name" of a type, hence it is a nominal thing, no? Otherwise you have to deal with a lot of complexity around recursive types and so forth that doesn't seem worthwhile.

Note that the TypeId is only stable for a particular version of the crate/compiler -- not stable for all time. (If I have two versions of a crate foo, then foo:Foo will have 2 hashes, depending on which origin it comes from.)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 5, 2016

📌 Commit 7bef809 has been approved by nikomatsakis

@eddyb
Copy link
Member Author

eddyb commented Aug 6, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 6, 2016

📌 Commit d42da7b has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 6, 2016

⌛ Testing commit d42da7b with merge ecdd51b...

bors added a commit that referenced this pull request Aug 6, 2016
Rewrite TypeId computation to not miss anything and work cross-crate.

Fixes #33703 and also some soundness holes in `Any` due to `TypeId` ignoring most lifetimes.
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.

5 participants