Regression in type parameter defaults in types and impls

[bluss]

This regression is only visible in cross-crate code; NOTE it's very likely it's due to an actual rustc bug fix.

Here is some playpen code for illustration anyway.

Given a struct with default parameters and two constructor functions:

pub struct Graph<N, E, Ty = Directed, Ix: IndexType = DefIndex> {
    nodes: Vec<Node<N, Ix>>,
    edges: Vec<Edge<E, Ix>>,
    ty: PhantomData<Ty>,
}


impl<N, E> Graph<N, E, Directed> {
    pub fn new() -> Self {
        Graph{nodes: Vec::new(), edges: Vec::new(), ty: PhantomData}
    }
}

impl<N, E> Graph<N, E, Undirected> {
    pub fn new_undirected() -> Self {
        Graph{nodes: Vec::new(), edges: Vec::new(), ty: PhantomData}
    }
}

In Rust 1.4 or later (current stable and current nightly), both must be called like this to compile:

    let g = Graph::<i32, i32>::new();
    let ug = Graph::<i32, i32, _>::new_undirected();

In current stable Rust 1.4, the following compiles if you import Graph across crates!

let ug = Graph::<i32, i32>::new_undirected();

This is invalid in current nightly Rust ~1.6, see travis build example; the testcases are in their own crates.

tests/ograph.rs:1018:18: 1018:48 error: no associated item named `new_undirected` found for type `petgraph::graph::Graph<_, ()>` in the current scope
tests/ograph.rs:1018     let mut gr = Graph::<_, ()>::new_undirected();
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[nikomatsakis]

This does seem more like a bug fix, but I'm not sure what would have triggered it.

[nikomatsakis]

triage: P-high

It's important to decide if this is now doing the correct thing.

[bluss]

I'm sure it's an acceptable fix, but I think we want issues for all regressions, no matter why they happen.

I was mostly amused that there was no way to write a type hinted call of Graph::new_undirected() that would simultaneously compile with Rust 1.4 (requires 2 type params) and Rust 1.6 (requires three); you have to hint the types through the returned value instead.

[arielb1]

The 2-type-param version was working only by a bug - we have no plans of working around that.

[nikomatsakis]

@arielb1 do you think you know what fixed it? maybe something in metadata got corrected?

[brson]

I'm trying to bisect to find out what changed, then write a test case.

[bluss]

See the issue link ^ I'm not crazy, the issue has now appeared somewhere else too 😄

[brson]

Bisecting is proceeding slowly...

[brson]

My bisection attempt failed.

[nikomatsakis]

@brson :( if you like, I can try tracing through the relevant code paths with git blame to try and find likely candidates

[bluss]

I looked and my guess landed on a metadata change that @arielb1 did, would explain the cross crate only behavior.

[nikomatsakis]

@bluss seems likely, I agree.

On Thu, Jan 14, 2016 at 4:38 PM, bluss notifications@github.com wrote:

I looked and my guess landed on a metadata change that @arielb1
https://github.com/arielb1 did, would explain the cross crate only
behavior.

—
Reply to this email directly or view it on GitHub
#30123 (comment).

[bluss]

The commit was db817ce

[arielb1]

This commit should not have changed anything, but I imagine there is a bug here.

[brson]

@nikomatsakis nah that's ok. It's not a huge deal. I did start another bisect though. I'm having a hard time summarizing this change for the release notes. There are a lot of technical pieces here. Can somebody try?

[bluss]

@arielb1 My guess could have landed totally wrong. Don't know why I tried. You all know this better than me. For example, as the ImageBuffer example shows, type parameter defaults don't have to be involved, which has me a bit confounded.

[brson]

I am still trying to bisect.

[brson]

f5fbefa caused it.

[brson]

cc @arielb1

[arielb1]

I think the culprit is that I added the

child_name_bindings.define_type(def, DUMMY_SP, modifiers);

[arielb1]

That indeed seems to be the change (commenting it out makes things compile again).

Could someone who understands resolve tell me how the change could cause that kind of effects?

[nikomatsakis]

I'm going to close this issue then as "not a bug", since we know that commit caused it.

[eddyb]

@arielb1 Coming back to this, I remember that you removed some metadata code for saving inherent methods: wouldn't they be picked up by resolve, instead of letting typeck do UFCS resolution?

[hatahet]

@brson:

My bisection attempt failed.

What caused it to fail the first time?