diff --git a/active/0000-anonymous-impl-only-in-same-module.md b/active/0000-anonymous-impl-only-in-same-module.md new file mode 100644 index 00000000000..88c695a3f81 --- /dev/null +++ b/active/0000-anonymous-impl-only-in-same-module.md @@ -0,0 +1,136 @@ +- Start Date: 2014-07-04 +- RFC PR #: (leave this empty) +- Rust Issue #: (leave this empty) + +# Summary + +Require "anonymous traits", i.e. `impl MyStruct` to occur only in the same module that `MyStruct` is defined. + +# Motivation + +Before I can explain the motivation for this, I should provide some background +as to how anonymous traits are implemented, and the sorts of bugs we see with +the current behaviour. The conclusion will be that we effectively already only +support `impl MyStruct` in the same module that `MyStruct` is defined, and +making this a rule will simply give cleaner error messages. + +- The compiler first sees `impl MyStruct` during the resolve phase, specifically + in `Resolver::build_reduced_graph()`, called by `Resolver::resolve()` in + `src/librustc/middle/resolve.rs`. This is before any type checking (or type + resolution, for that matter) is done, so the compiler trusts for now that + `MyStruct` is a valid type. +- If `MyStruct` is a path with more than one segment, such as `mymod::MyStruct`, + it is silently ignored (how was this not flagged when the code was written??), + which effectively causes static methods in such `impl`s to be dropped on the + floor. A silver lining here is that nothing is added to the current module + namespace, so the shadowing bugs demonstrated in the next bullet point do not + apply here. (To locate this bug in the code, find the `match` immediately following + the `FIXME (#3785)` comment in `resolve.rs`.) This leads to the following +```` +mod break1 { + pub struct MyGuy; + + impl MyGuy { + pub fn do1() { println!("do 1"); } + } +} + +impl break1::MyGuy { + fn do2() { println!("do 2"); } +} + +fn main() { + break1::MyGuy::do1(); + break1::MyGuy::do2(); +} +```` +```` +:15:5: 15:23 error: unresolved name `break1::MyGuy::do2`. +:15 break1::MyGuy::do2(); +```` + as noticed by @huonw in https://github.com/rust-lang/rust/issues/15060 . +- If one does not exist, the compiler creates a submodule `MyStruct` of the + current module, with `kind` `ImplModuleKind`. Static methods are placed into + this module. If such a module already exists, the methods are appended to it, + to support multiple `impl MyStruct` blocks within the same module. If a module + exists that is not `ImplModuleKind`, the compiler signals a duplicate module + definition error. +- Notice at this point that if there is a `use MyStruct`, the compiler will act + as though it is unaware of this. This is because imports are not resolved yet + (they are in `Resolver::resolve_imports()` called immediately after + `Resolver::build_reduced_graph()` is called). In the final resolution step, + `MyStruct` will be searched in the namespace of the current module, checking + imports only as a fallback (and only in some contexts), so the `use MyStruct` is + effectively shadowed. If there is an `impl MyStruct` in the file being imported + from, the user expects that the new `impl MyStruct` will append to that one, + same as if they are in the original file. This leads to the original bug report + https://github.com/rust-lang/rust/issues/15060 . +- In fact, even if no methods from the import are used, the name `MyStruct` will + not be associated to a type, so that +```` +trait T {} +impl Vec { + fn from_slice<'a>(x: &'a [uint]) -> Vec { + fail!() + } +} +fn main() { let r = Vec::from_slice(&[1u]); } +```` +```` +error: found module name used as a type: impl Vec::Vec (id=5) +impl Vec +```` + which @Ryman noticed in https://github.com/rust-lang/rust/issues/15060 . The + reason for this is that in `Resolver::resolve_crate()`, the final step of + `Resolver::resolve()`, the type of an anonymous `impl` is determined by + `NameBindings::def_for_namespace(TypeNS)`. This function searches the namespace + `TypeNS` (which is not affected by imports) for a type; failing that it + tries for a module; failing that it returns `None`. The result is that when + typeck runs, it sees `impl [module name]` instead of `impl [type name]`. + + +The main motivation of this RFC is to clear out these bugs, which do not make +sense to a user of the language (and had me confused for quite a while). + +A secondary motivation is to enforce consistency in code layout; anonymous traits +are used the way that class methods are used in other languages, and the data +and methods of a struct should be defined nearby. + +# Detailed design + +I propose three changes to the language: + +- `impl` on multiple-ident paths such as `impl mymod::MyStruct` is disallowed. + Since this currently suprises the user by having absolutely no effect for + static methods, support for this is already broken. +- `impl MyStruct` must occur in the same module that `MyStruct` is defined. + This is to prevent the above problems with `impl`-across-modules. + Migration path is for users to just move code between source files. + +# Drawbacks + +Static methods on `impl`s-away-from-definition never worked, while non-static +methods can be implemented using non-anonymous traits. So there is no loss in +expressivity. However, using a trait where before there was none may be clumsy, +since it might not have a sensible name, and it must be explicitly imported by +all users of the trait methods. + +For example, in the stdlib `src/libstd/io/fs.rs` we see the code `impl path::Path` +to attach (non-static) filesystem-related methods to the `Path` type. This would +have to be done via a `FsPath` trait which is implemented on `Path` and exported +alongside `Path` in the prelude. + +It is worth noting that this is the only instance of this RFC conflicting with +current usage in the stdlib or compiler. + +# Alternatives + +- Leaving this alone and fixing the bugs directly. This is really hard. To do it + properly, we would need to seriously refactor resolve. + +# Unresolved questions + +None. + + +