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

Error when using cross-crate static methods exported from a private module via `pub use` #4202

Closed
brendanzab opened this Issue Dec 16, 2012 · 10 comments

Comments

Projects
None yet
5 participants
@brendanzab
Member

brendanzab commented Dec 16, 2012

(commented-keywords below are copied over from original bug report.)

foo.rs

#[link(name = "foo",
       vers = "0.1")];
#[crate_type = "lib"];

pub use sub_foo::Foo;

mod sub_foo {
    pub trait Foo {
        /*static*/ /*pub*/ fn foo() -> Self;
    }

    /*pub*/ impl Foo for int {
        /*static*/ /*pub*/ fn foo() -> int { 42 }
    }
}

bar.rs

extern mod foo;
use foo::Foo;

fn main() {
    assert!(42 == Foo::foo());
}
$ rustc --out-dir . src/foo.rs     
warning: no debug symbols in executable (-arch x86_64)
$ rustc --out-dir . src/bar.rs -L .
src/bar.rs:5:17: 5:25 error: unresolved name
src/bar.rs:5     assert 42 == Foo::foo();
                              ^~~~~~~~
src/bar.rs:5:17: 5:25 error: use of undeclared module `Foo`
src/bar.rs:5     assert 42 == Foo::foo();
                              ^~~~~~~~
src/bar.rs:5:17: 5:25 error: unresolved name: Foo::foo
src/bar.rs:5     assert 42 == Foo::foo();
                              ^~~~~~~~
error: aborting due to 3 previous errors
@brendanzab

This comment has been minimized.

Show comment
Hide comment
@brendanzab

brendanzab Dec 16, 2012

Member

I just tested and it's also a problem for public modules as well. ie:

foo.rs

#[link(name = "foo",
       vers = "0.1")];
#[crate_type = "lib"];

pub use sub_foo::Foo;

pub mod sub_foo {    // <- added `pub` to this line and got the same error when compiling bar.rs
    pub trait Foo {
        static pub fn foo() -> self;
    }

    pub impl int: Foo {
        static pub fn foo() -> int { 42 }
    }
}
Member

brendanzab commented Dec 16, 2012

I just tested and it's also a problem for public modules as well. ie:

foo.rs

#[link(name = "foo",
       vers = "0.1")];
#[crate_type = "lib"];

pub use sub_foo::Foo;

pub mod sub_foo {    // <- added `pub` to this line and got the same error when compiling bar.rs
    pub trait Foo {
        static pub fn foo() -> self;
    }

    pub impl int: Foo {
        static pub fn foo() -> int { 42 }
    }
}

@ghost ghost assigned catamorphism Feb 21, 2013

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Mar 15, 2013

Contributor

I'm hitting this too trying to update Path to proper constructor conventions. It's reexported from the prelude and after adding the new method nobody can construct one.

Contributor

brson commented Mar 15, 2013

I'm hitting this too trying to update Path to proper constructor conventions. It's reexported from the prelude and after adding the new method nobody can construct one.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 25, 2013

Member

Not critical for 0.6; de-milestoning

Member

pnkfelix commented Mar 25, 2013

Not critical for 0.6; de-milestoning

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 5, 2013

Contributor

Updated reproduction for rust incoming as of May 5th 2013: https://gist.github.com/thomaslee/5522568

Going to see if I can fix this ...

Contributor

thomaslee commented May 5, 2013

Updated reproduction for rust incoming as of May 5th 2013: https://gist.github.com/thomaslee/5522568

Going to see if I can fix this ...

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 6, 2013

Contributor

Looks like rustc::middle::resolve::resolve_path tries to resolve Foo as a module because path has more than one element in its idents member (specifically, the path is Foo::foo & the path.idents.len() > 1 check succeeds).

I'll keep going down the rabbit hole tomorrow night -- I think the prime suspect atm is a parser bug.

Contributor

thomaslee commented May 6, 2013

Looks like rustc::middle::resolve::resolve_path tries to resolve Foo as a module because path has more than one element in its idents member (specifically, the path is Foo::foo & the path.idents.len() > 1 check succeeds).

I'll keep going down the rabbit hole tomorrow night -- I think the prime suspect atm is a parser bug.

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 8, 2013

Contributor

Alright, I haven't fully proven it yet, but it seems that after more investigation it looks like we may be skipping over some encoding work for the trait we're trying to expose via "pub use". Here's some debug output from building bar.rs as it relates to Bar (a known "good" trait):

rust: ~"(each_path) yielding reexported item: Bar"
rust: ~"(building reduced graph for external crate) found path entry: Bar (dl_def(def_trait({crate: 2, node: 6})))"
rust: ~"(building reduced graph for external crate) building type Bar"
rust: ~"(building reduced graph for external crate) ... adding trait method \'bar\'"
...
rust: ~"(each_path) yielding explicit item: Bar"
rust: ~"(building reduced graph for external crate) found path entry: Baz (dl_def(def_trait({crate: 2, node: 6})))"
rust: ~"(building reduced graph for external crate) building type Bar"
rust: ~"(building reduced graph for external crate) ... adding trait method \'bar\'"
uust: ~"(each_path) yielding explicit item: Bar::bar"
rust: ~"(building reduced graph for external crate) found path entry: Bar::bar (dl_def(def_static_method({crate: 2, node: 5}, Some({crate: 2, node: 6}), impure_fn)))"

And the output relating to Foo (known to be broken) for the same build:

rust: ~"(each_path) yielding reexported item: Foo"
rust: ~"(building reduced graph for external crate) found path entry: Foo (dl_def(def_trait({crate: 2, node: 23})))"
rust: ~"(building reduced graph for external crate) building type Foo"
rust: ~"(building reduced graph for external crate) ... adding trait method \'foo\'"
...

We never see the explicit items that we saw decoded for our "good" trait. I'm still figuring out the metadata encoding/decoding stuff, but I have a strong suspicion this is at least related to the original gremlin.

Contributor

thomaslee commented May 8, 2013

Alright, I haven't fully proven it yet, but it seems that after more investigation it looks like we may be skipping over some encoding work for the trait we're trying to expose via "pub use". Here's some debug output from building bar.rs as it relates to Bar (a known "good" trait):

rust: ~"(each_path) yielding reexported item: Bar"
rust: ~"(building reduced graph for external crate) found path entry: Bar (dl_def(def_trait({crate: 2, node: 6})))"
rust: ~"(building reduced graph for external crate) building type Bar"
rust: ~"(building reduced graph for external crate) ... adding trait method \'bar\'"
...
rust: ~"(each_path) yielding explicit item: Bar"
rust: ~"(building reduced graph for external crate) found path entry: Baz (dl_def(def_trait({crate: 2, node: 6})))"
rust: ~"(building reduced graph for external crate) building type Bar"
rust: ~"(building reduced graph for external crate) ... adding trait method \'bar\'"
uust: ~"(each_path) yielding explicit item: Bar::bar"
rust: ~"(building reduced graph for external crate) found path entry: Bar::bar (dl_def(def_static_method({crate: 2, node: 5}, Some({crate: 2, node: 6}), impure_fn)))"

And the output relating to Foo (known to be broken) for the same build:

rust: ~"(each_path) yielding reexported item: Foo"
rust: ~"(building reduced graph for external crate) found path entry: Foo (dl_def(def_trait({crate: 2, node: 23})))"
rust: ~"(building reduced graph for external crate) building type Foo"
rust: ~"(building reduced graph for external crate) ... adding trait method \'foo\'"
...

We never see the explicit items that we saw decoded for our "good" trait. I'm still figuring out the metadata encoding/decoding stuff, but I have a strong suspicion this is at least related to the original gremlin.

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 8, 2013

Contributor

Looking closer at the code, I suspect the "reexported item: Foo" we're seeing in the log output may relate to sub_foo rather than our "pub use".

Contributor

thomaslee commented May 8, 2013

Looking closer at the code, I suspect the "reexported item: Foo" we're seeing in the log output may relate to sub_foo rather than our "pub use".

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 8, 2013

Contributor

Okay, lots of time lost on various rabbit holes, but I think I understand what's happening here now:

Basically we compile foo.rs & see the following explicit symbols exposed by the foo crate (omitting irrelevant junk):

  • Foo (pub use sub_foo::Foo)
  • Bar
  • Bar::bar
  • sub_foo::Foo
  • sub_foo::Foo::foo

Note that the "outer" module has no reference to the static method Foo::foo. When we say "use sub_foo::Foo" at the top level, the compiler goes to no additional effort to try & figure out if maybe we're trying to use a trait (and thus somehow need to expose the trait's associated static methods in addition to the trait itself).

Bar on the other hand is declared at the top level: the metadata decoder pulls it in so the resolver finds Bar::bar in the right place. Note that e.g. use foo::sub_foo::Foo works too for this reason: the metadata for foo::sub_foo::Foo::foo was generated when foo was compiled.

Contributor

thomaslee commented May 8, 2013

Okay, lots of time lost on various rabbit holes, but I think I understand what's happening here now:

Basically we compile foo.rs & see the following explicit symbols exposed by the foo crate (omitting irrelevant junk):

  • Foo (pub use sub_foo::Foo)
  • Bar
  • Bar::bar
  • sub_foo::Foo
  • sub_foo::Foo::foo

Note that the "outer" module has no reference to the static method Foo::foo. When we say "use sub_foo::Foo" at the top level, the compiler goes to no additional effort to try & figure out if maybe we're trying to use a trait (and thus somehow need to expose the trait's associated static methods in addition to the trait itself).

Bar on the other hand is declared at the top level: the metadata decoder pulls it in so the resolver finds Bar::bar in the right place. Note that e.g. use foo::sub_foo::Foo works too for this reason: the metadata for foo::sub_foo::Foo::foo was generated when foo was compiled.

@thomaslee

This comment has been minimized.

Show comment
Hide comment
@thomaslee

thomaslee May 10, 2013

Contributor

Have what I believe is a working fix for this that exposes static trait methods by including them in the reexport table iff the reexported trait has a path that differs from the path of the module in which is being reexported from. Will put together a test or two & submit a pull request.

Contributor

thomaslee commented May 10, 2013

Have what I believe is a working fix for this that exposes static trait methods by including them in the reexport table iff the reexported trait has a path that differs from the path of the module in which is being reexported from. Will put together a test or two & submit a pull request.

thomaslee added a commit to thomaslee/rust that referenced this issue May 11, 2013

bors added a commit that referenced this issue May 11, 2013

auto merge of #6384 : thomaslee/rust/issue-4202, r=catamorphism
This fixes the issue described in #4202.

From what I understood of the code, when we reexport a trait in a submodule using e.g. "pub use foo::SomeTrait", we were not previously making an effort to reexport the static methods on that trait.

I'm new to the Rust code base (and the Rust language itself) so my approach may not be kosher, but this patch works by changing the encoder to include the static methods associated with traits.

I couldn't see any tests for this area of the code, so I didn't really have any examples to go by. If tests are needed, I'm happy to work through that if I can get some assistance to do so.

bors added a commit that referenced this issue May 20, 2013

auto merge of #6432 : thomaslee/rust/issue-4202-02, r=catamorphism
My earlier fix for #4202 would not work correctly if the trait being exported was a top-level item relative to the module from which it was being exported. An example that would not work correctly with the original patch:

    // foo.rs

    pub use Foo = self::Bar;

    pub trait Bar {
      pub fn bar() -> Self;
    }

    impl Bar for int {
      pub fn bar() -> int { 42 }
    }

    // bar.rs

    fn main() {
      foo::Foo::bar()
    }

This is now supported.

I believe this change will allow the GenericPath trait to be exported from core::path as Path in such a manner, which should allow #5389 to move forward.
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 23, 2013

Member

confirmed fixed after updating original test case.

Member

pnkfelix commented May 23, 2013

confirmed fixed after updating original test case.

@pnkfelix pnkfelix closed this May 23, 2013

bors added a commit that referenced this issue Jun 1, 2013

auto merge of #6880 : thomaslee/rust/issue-6745, r=catamorphism
This fixes #6745, which itself relates to #4202. Slightly ham-fisted -- feel particularly funny about using the typeck phase to gather the base -> impl mapping, and the separate code paths for traits vs. "real" bases feels like it could be avoided -- but it seems to work.

As always, open to suggestions if there's a better way to accomplish what I'm trying to do.

@catamorphism r?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment