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

#[test] visibility changes can produce conflicts and type errors with glob use. #52557

Closed
djrenren opened this Issue Jul 20, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@djrenren
Contributor

djrenren commented Jul 20, 2018

Because #[test] marks tests as public so that they can be reexported (avoiding E0364), they can cause namespace pollutions that only occur in test builds.

Minimal repro:

mod A {
    #[test]
    fn foo() {}
}

mod B {
    pub fn foo() -> bool {
        true
    }
}

use B::foo;
use A::*;

fn conflict() {
    let x: bool = foo();
}

In a normal build, the only foo in scope is B::foo, but in a test build A::foo will shadow it. And produce the following error:

error[E0308]: mismatched types
  --> ./test.rs:20:19
   |
20 |     let x: bool = foo();
   |                   ^^^^^ expected bool, found ()
   |
   = note: expected type `bool`
              found type `()`
@djrenren

This comment has been minimized.

Contributor

djrenren commented Jul 20, 2018

I propose that instead of expanding:

#[test]
fn foo() {}

to:

pub fn foo(){}
pub mod __test_reexports {
   pub use super::foo;
}

we expand to:

pub mod __test_reexports {
    use super::*;
    pub fn foo() {...}
}

This is a breaking change but only for the semi-pathological case where you have test functions referencing each other. It also brings test into line with standard builds in that #[test] are not referenceable. (in debug/release due to removal, and in test due to a gensymed module).

@nrc

This comment has been minimized.

Member

nrc commented Jul 20, 2018

the semi-pathological case where you have test functions referencing each other

I bet that's not uncommon enough to make a breaking change. However, I would expect test functions that reference each other to do so via relative paths, and this would only be breaking if absolute paths were used (iiuc). In any case we should probably just fix this and do a Crater run.

@nrc

This comment has been minimized.

Member

nrc commented Jul 20, 2018

cc @petrochenkov and @rust-lang/compiler

We might be able to hack something in name resolution? Perhaps glob imports don't treat test functions as public? Or we give them some weird hygiene marker or something?

@djrenren

This comment has been minimized.

Contributor

djrenren commented Jul 20, 2018

Actually if we did:

use __test_reexports::*;

pub mod __test_reexports {
    use super::*;
    pub fn foo() {...}
}

I think we'd be compatible and safe

@djrenren

This comment has been minimized.

Contributor

djrenren commented Jul 20, 2018

Well, technically we should use or pub use each test in accordance with its visibility

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 1, 2018

I much better prefer rust-lang/rfcs#2471 (comment) as a solution to all our testing woes. I'll maybe try to prototype it soon.

bors added a commit that referenced this issue Aug 2, 2018

Auto merge of #52890 - djrenren:test-visibility, r=petrochenkov
Reexport tests without polluting namespaces

This should fix issue #52557.

Basically now we gensym a new name for the test function and reexport that.
That way the test function's reexport name can't conflict because it was impossible for the test author to write it down.
We then use a `use` statement to expose the original name using the original visibility.
@djrenren

This comment has been minimized.

Contributor

djrenren commented Aug 2, 2018

Fixed by #52890

@djrenren djrenren closed this Aug 2, 2018

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