RFC to require `impl MyStruct` to be nearby the definition of `MyStruct`

[apoelstra]

This fixes #15060 and #3785.

[adrientetar]

cc rust-lang/rust#15060 and rust-lang/rust#3785

[SiegeLord]

I have found this feature useful in my library to encompass the state of a foreign library and do dependency injection in a clean way. Here's a snippet of how its used:

mod a
{
    pub struct Bitmap
    {
        priv_field: () // note this field
    }

    impl super::Core
    {
        pub fn create_bitmap(&self) -> Bitmap
        {
            Bitmap{ priv_field: () }
        }
    }
}

pub struct Core;

impl Core
{
    pub fn init_library() -> Core
    {
        Core
    }
}

fn main()
{
    let core = Core::init_library();
    let bmp = core.create_bitmap();
}

I suppose I could work around this by using a macro that looks like this:

mod a
{
    pub struct Bitmap
    {
        priv_field: ()
    }

    pub mod export
    {
        cross_impl!
        {
            impl Core
            {
                pub fn create_bitmap(&self) -> Bitmap
                {
                    Bitmap{ priv_field: () }
                }
            }
        }

        // Expands to:

        /*
        pub trait _AnonTrait_XXXXXX
        {
            fn create_bitmap(&self) -> Bitmap;
        }

        impl _AnonTrait_XXXXXX for Core
        {
            fn create_bitmap(&self) -> Bitmap
            {
                Bitmap{ priv_field: () }
            }
        }
        */
    }
}

pub use a::export::*;
pub struct Core;

But if I can do this via a macro, what prevents the compiler from doing it?

[apoelstra]

@SiegeLord if you add any static methods to your impl super::Core they will not be visible anywhere and lead to confusing error messages. (This is what I was referring to when I said "only lucky users are doing this".)

If you use non-anonymous traits as described, you cannot have static methods that are accessible as Core::init_library(). You would need to do something like let core: Core = Trait::init_library(), so the trait name needs to be user-visible, defeating the hiding provided by the macro.

Edit: I think it doable to simply prevent static methods in impl long::path::to::Struct and impl StructThatIsntSeenYet while leaving non-static methods be. resolve doesn't care about non-static methods except to provide pretty error messages to users who call them out of context.

Edit2: If we were to take this approach, the pretty error messages would be missing in the currently-broken cases, which would still be a bug, though a minor one.

[apoelstra]

I realized this has nothing to do with typeck, that's just where "module instead of type" is noticed. The reason that Vec in impl<U: T> Vec<U> resolves to a module here rather than a type is in Resolver::def_for_namespace(), which is eventually called from Resolver::resolve_crate(), the final function call of Resolver::resolve(). (The purpose of resolve_crate() is to go through once every struct and impl has been seen to resolve things completely.)

The way that def_for_namespace() works is to try and return a DefType first, which it will only find if the type Vec had been defined somewhere. Failing that, try and return a DefMod, which will be defined if an impl Vec or struct Vec had occured somewhere. Failing that, return None.

Notice that at no point are imports used when resolving Vec. This could happen when resolving Vec::from_slice() later on, in Resolver::resolve_module_path(), which first tries to find Vec in the current module, then failing that, resolves imports. (In fact this would not happen, since Vec would be found in the current module, as a module rather than type, thanks to the impl Vec and the anonymous trait module it creates.) But this doesn't even have a chance to happen, since, resolve_module_path() is never called when resolving impl Vec; impl blocks and functions become different beasts during the initial build_reduced_graph() run, so different code paths are taken in resolve_crate(). (And this makes sense, since when resolving impl Vec in resolve_crate(), we know that Vec will is in the module because we added it in build_reduced_graph() as an anonymous trait module. So there is no point in resolving imports.)

[apoelstra]

There is no need for this restriction. The appropriate place to do the check is in Resolver::resolve_implementation(), which is called from Resolver::resolve_crate(), the final step of resolve. At this point, struct MyStruct will have been seen no matter where in the source file it occurs.

[apoelstra]

@SiegeLord I notice that in src/libstd/io/fs.rs there is impl path::Path which does exactly what you describe. It really is cleaner this way. Dang. So I will rethink my rules and try to come up with a consistent RFC which is (a) consistent, (b) lets you do this, and (c) doesn't expose the limitations of resolve through bizarre bugs.

Edit: In fact, I don't think there's a way (short of seriously refactoring resolve) to handle static methods for anonymous traits on elsewhere-defined structs. And I don't want to say "only non-static methods on impls, unless the struct is defined in the same file" because that's an extremely specific rule to memorize and clearly originates in compiler limitations. Let's not be like certain other languages which have a bajillion such rules :)

My feeling for simplicity's sake is to go with the original RFC, sans the "impl must occur after struct" restriction, and say that anonymous impls have to occur only in the defining module.

[sfackler]

👍 Even if resolve handled it well, I think it'd be best to restrict anonymous impls to the defining module.

[dobkeratops]

I'd be interested in a move in the opposite direction .. fewer restrictions on what can be impl'd where, and the option of duck type interfaces to cut down on the noise when dealing with single function traits

[apoelstra]

Pushed changes which fix my own line comments (remove requirement to have impl after struct and minor improvements to description of resolve process).

[apoelstra]

cc rust-lang/rust#12729 rust-lang/rust#12728 rust-lang/rust#8767 rust-lang/rust#7607 rust-lang/rust#9584

[pcwalton]

+1, language simplification.

[apoelstra]

I'm happy to implement this if it gains support. There are two new error messages to be added:

• impl aa::bb::cc is not allowed since paths of length > 1 are illegal.
• impl YetUnseen is not allowed if YetUnseen is not defined as a type in the current module.

We can detect if YetUnseen is simply undefined (though perhaps it appears in a use statement, which we won't see) or defined as something else altogether. We might want to give different error messages in each case.

I'm no good at bikeshedding, so can I get some suggestions for these?

[nikomatsakis]

I am of mixed minds.

On the one hand, this is the conservative choice, works around the various resolve bugs, and can easily be extended later. It also seems to satisfy a sort of encapsulation concern, in that it kind of makes sense for "inherent methods" to be defined close to the type in question.

On the other hand, I think it is frequently useful to be able to split apart the methods on a type. @SiegeLord brings up an interesting usage I hadn't considered. I frequently find I want to define a core type and then define methods related to (e.g.) type checking in the type checker and other things somewhere else. On the other other hand, both of these use cases can readily be accommodated with traits.

One specific concern I have is that violates the rule of thumb that if an action is legal in module M, it is legal in submodules of M. I think this is a good rule, since it ensures that you can always split up a module M into multiple submodules when it gets too complicated. (Still, as before, you can work-around it with traits.)

(I am also a bit disappointed in the idea of choosing our RFCs based on resolve bugs, rather than resolving on what we think the rules should be and filing bugs where we deviate.)

[pcwalton]

@nikomatsakis To me it's just a question of resourcing and priorities. I agree that what you describe is the best behavior, I just want to make sure we have something shippable for 1.0.

[apoelstra]

An example of the modularity that @nikomatsakis says is useful appears in @jeremyletang's rgtk. Combining all the impl cairo::Contexts into a single module is not hard to do, but changing filesystem layout for rustc's sake is not really desirable. It also forces you to either expose some private interfaces or combine functionally-distinct modules into one, just like in @SiegeLord's example.

[huonw]

Another (relatively minor) issue: rust-lang/rust#16398

[rainbow-alex]

I use impl super::MyStruct and would much rather see it fixed (so I can use MyStruct; impl MyStruct) than removed.