trait permitting extra members

[nikomatsakis]

I've been tinkering locally with a crate called salsa. Along the way, I noticed that my impl permits me to add extra members. e.g., I have this trait:

pub trait QueryContext: QueryContextStorageTypes {
    fn salsa_runtime(&self) -> &runtime::Runtime<Self>;
}

But in my cargo example I can use this impl:

impl salsa::QueryContext for QueryContextImpl {
    fn extra(&self) -> u32 {
        0
    }

    fn salsa_runtime(&self) -> &salsa::runtime::Runtime<QueryContextImpl> {
        &self.runtime
    }
}

What the heck is going on here? I couldn't easily reproduce this in a smaller example, so I saved the commit. It did reproduce cleanly after rm -rf target and cargo clean, so I guess it's (probably?) not an incremental bug.

To reproduce:

• Clone the salsa repo at https://github.com/nikomatsakis/salsa and go to the commit 36e2b9cdcc01228560f9fb7ffdfadf466d21e60d.
• Run cargo build --example storage_varieties
• Observe that build does not fail

Marking as T-compiler and P-high. P-high because this could be a very dangerous bug in terms of backwards compat risk! But I'm hoping it's something more bizarre (e.g., not rebuilding when I think it is). I did validate though that adding other weird stuff (e.g., invalid tokens) causes compilation to fail. Assigning to myself to investigate on Monday.

[GabrielMajeri]

@nikomatsakis Hi, while playing around with that example I found something interesting:

If I add an

extern crate salsa;

at the top it does error correctly:

error[E0407]: method `extra` is not a member of trait `salsa::QueryContext`                                                                              
  --> examples/storage_varieties/implementation.rs:30:5                                                                                                  
   |                                                                                                                                                     
30 | /     fn extra(&self) -> u32 {                                                                                                                      
31 | |         0                                                                                                                                         
32 | |     }                                                                                                                                             
   | |_____^ not a member of trait `salsa::QueryContext`                                                                                                 
                                                     

So at least this seems to be an issue with the new Rust 2018 module system?

[GabrielMajeri]

I think I've got a minimal repro (repro.tar.gz). Considering a Rust 2018 crate with a library and a binary:

lib.rs:

pub trait Foo {
    fn foo();
}

main.rs:

// Uncomment to make error work:
//extern crate repro;

pub struct Bar;

impl repro::Foo for Bar {
    fn foo() {}

    // This compiles!
    fn extra() {}
}

fn main() {}

I can also work around the bug by adding a use repro::Foo;

[nikomatsakis]

@GabrielMajeri awesome, thanks!

[nikomatsakis]

cc @petrochenkov — any ideas? I'm looking into the code a bit

[nikomatsakis]

The immediate cause of the problem is that current_trait_ref is None here:

rust/src/librustc_resolve/lib.rs

Line 2612 in 5472b07

if let Some((module, _)) = self.current_trait_ref {

[nikomatsakis]

We seem to enter this block of code with a successful resolution in def:

rust/src/librustc_resolve/lib.rs

Lines 2472 to 2485 in 5472b07

	new_id = Some(def.def_id());
	let span = trait_ref.path.span;
	if let PathResult::Module(ModuleOrUniformRoot::Module(module)) =
	self.resolve_path(
	None,
	&path,
	None,
	false,
	span,
	CrateLint::SimplePath(trait_ref.ref_id),
	)
	{
	new_val = Some((module, trait_ref.clone()));
	}

But something goes wrong after that.

[nikomatsakis]

It seems like the problem is that resolve_path returns "Indeterminate"

[nikomatsakis]

Here is the output from some logs I added, showing the value of def and also the result of the call to resolve_path:

DEBUG 2018-10-04T16:36:34Z: rustc_resolve: with_optional_trait_ref: def=Trait(DefId(9/0:3))
DEBUG 2018-10-04T16:36:34Z: rustc_resolve: resolve_path(path=[repro#0, Foo#0], opt_ns=None, record_used=false, path_span=Span { lo: BytePos(42), hi: BytePos(52), ctxt: #0 }, crate_lint=SimplePath(NodeId(9)))
DEBUG 2018-10-04T16:36:34Z: rustc_resolve: resolve_path ident 0 repro#0
DEBUG 2018-10-04T16:36:34Z: rustc_resolve: with_optional_trait_ref: path_resolution=Indeterminate

[nikomatsakis]

cc @eddyb also — maybe you or @petrochenkov has some clue what's happening here based on the above output.

[petrochenkov]

I'll look at this once I'm back from vacation next week (unless it's resolved before that).

[pnkfelix]

visited for triage. Leaving on RC2 milestone, and thus, leaving at P-high.

[petrochenkov]

Fixed in #55102