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 ItemKind::TraitAlias encoding #56541

Closed
wants to merge 1 commit into from

Conversation

dlrobertson
Copy link
Contributor

ItemKind::TraitAlias should be encoded just like a regular Trait.

Fixes: #56488

ItemKind::TraitAlias should be encoded just like a regular Trait.
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 5, 2018
@dlrobertson
Copy link
Contributor Author

r? @Centril

@dlrobertson
Copy link
Contributor Author

The test I wrote doesn't include all of the example in #56488 but it still causes nightly to panic.

@Centril
Copy link
Contributor

Centril commented Dec 5, 2018

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Centril Dec 5, 2018
@nikomatsakis
Copy link
Contributor

Hmm, this doesn't seem like the right fix. I think the bug is a touch deeper though (cc @alexreg). In particular, the is_trait_alias function seems to currently be thread-local:

/// Returns `true` if `def_id` is a trait alias.
pub fn is_trait_alias(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> bool {
    if let Some(node_id) = tcx.hir.as_local_node_id(def_id) {
        if let Node::Item(item) = tcx.hir.get(node_id) {
            if let hir::ItemKind::TraitAlias(..) = item.node {
                return true;
            }
        }
    }
    false
}

but I think that it will not work correctly when you try to actually use a trait alias across crates, as a result.

i.e., can we make a test like

// Crate A
trait Foo = Send + Sync;

// Crate b:
use crate_a::Foo;
fn use_b<T: Foo>() { }
fn main() {
    use_b::<u32>(); // Expect Ok
    use_b::<Rc<u32>>(); // Expect error
}

and see what behavior is?

@nikomatsakis
Copy link
Contributor

In any case, I feel like we can't just pretend a trait is a trait alias across crates.

@dlrobertson
Copy link
Contributor Author

Hmm, this doesn't seem like the right fix... In any case, I feel like we can't just pretend a trait is a trait alias across crates.

Ah, right... good point.

@alexreg
Copy link
Contributor

alexreg commented Dec 6, 2018

Thanks for having a go at this @dlrobertson!

I think @nikomatsakis is right; the is_trait_alias fn that I wrote is crate-local (I think Niko writing "thread-local" was a slip heh). I'm not sure on the best way to fix this right now, but let me give it some though.

@dlrobertson
Copy link
Contributor Author

👍

@dlrobertson dlrobertson closed this Dec 6, 2018
@dlrobertson dlrobertson deleted the fix_56488 branch December 6, 2018 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants