Skip to content
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

Permit unqualified references to associated types of the trait within the trait definition #18764

Closed
emberian opened this issue Nov 8, 2014 · 24 comments
Assignees
Labels
A-associated-items Area: Associated items such as associated types and consts.
Milestone

Comments

@emberian
Copy link
Member

emberian commented Nov 8, 2014

#![feature(associated_types)]
pub trait Transfer {
    type Result;
    fn transfer(&mut self) -> Result;
}

fn main(){}

Gives the error:

foo.rs:4:31: 4:37 error: wrong number of type arguments: expected 2, found 0
foo.rs:4     fn transfer(&mut self) -> Result;
                                       ^~~~~~

According to the RFC the Result ought to refer to the associated type.

@emberian
Copy link
Member Author

emberian commented Nov 8, 2014

Also:

#![feature(associated_types)]
pub trait Transfer<S> {
    type R;
    fn transfer(&mut self) -> R;
}

fn main(){}
foo.rs:4:31: 4:32 error: use of undeclared type name `R`
foo.rs:4     fn transfer(&mut self) -> R;
                                       ^
error: aborting due to previous error

@nikomatsakis nikomatsakis changed the title Associated type scoping is broken Permit unqualified references to associated types of the trait within the trait definition Nov 8, 2014
@nikomatsakis
Copy link
Contributor

Updated title for precision.

@Gankra
Copy link
Contributor

Gankra commented Jan 9, 2015

This makes refactoring code to use associated types a lot more painful, and adds another straw to the camel's back of associated types being less ergonomic than Foo<A, B, C, D> soup.

@aturon
Copy link
Member

aturon commented Jan 9, 2015

There's been some debate on whether this feature should go forward. The main objection, I believe, is that the RFC brings into scope all items of the trait itself, but not any super traits. This is partly due to hygiene concerns, and partly due to the fact that the notion of a "super trait" is somewhat blurred with generalized where clauses. It may be problematic to have to sometimes write Self::T and other times T.

Personally, I think the ergonomic and readability benefits are worth it even if in edge cases the Self:: is required. I would also like to be more certain that there are hygiene problems before we close the door to bringing in super trait items.

@aturon
Copy link
Member

aturon commented Jan 9, 2015

Nominating for beta milestone: we need to at least make a decision on this topic before then.

@ghost
Copy link

ghost commented Jan 9, 2015

This makes refactoring code to use associated types a lot more painful, and adds another straw to the camel's back of associated types being less ergonomic than Foo<A, B, C, D> soup.

Was that supposed to be one of the goals of moving to associated types? I have no idea but would have expected it was more about expressivity (with some cost in terms of ergonomics). In any case, I'm sort of on the fence about this one. I think I find having the Self::X makes it easier to read and understand the code at a glance, particularly when there are a lot of other parameters in the mix.

@Gankra
Copy link
Contributor

Gankra commented Jan 9, 2015

@darinmorrison it would be nice if the common case for generics didn't become harder, regardless of explicit goals. Especially with new Orphan/Impl checks forcing associated types.

@aturon
Copy link
Member

aturon commented Jan 9, 2015

Whatever decision we make here should apply to impls as well.

@brson brson added this to the 1.0 beta milestone Jan 15, 2015
@nrc nrc self-assigned this Feb 10, 2015
@nrc nrc added the A-associated-items Area: Associated items such as associated types and consts. label Feb 10, 2015
@nrc
Copy link
Member

nrc commented Feb 10, 2015

In the scope of

trait Foo {
    type Bar;
}

The programmer can't even write Foo::Bar, it requires <Self as Foo>::Bar of Self::Bar. It seems that is want to accept Bar we should accept Foo::Bar too (the compiler claims it is ambiguous, but I don't see why, perhaps with supertraits? But in this case there are none).

@nrc
Copy link
Member

nrc commented Feb 10, 2015

Similarly in impls, if we have impl Foo for Baz, if we are to allow Bar it seems we should allow Baz::Bar too (and let the compiler assume that Baz is shorthand for <Baz as T> inside an impl T....

@nikomatsakis
Copy link
Contributor

One thing is that it would be inconsistent with referencing non-type members:

trait Foo {
    type Bar;
    fn m() -> Self { .. }

    fn n() {
        Foo::Bar // OK
        Foo::m(); // error, need to infer self-type
    }
}

@nrc
Copy link
Member

nrc commented Feb 10, 2015

After some discussion, the plan is (which we believe conforms to the RFC): Within a trait Foo or impl of trait Foo, α and Foo::α are both shorthands for <Self as Foo>::α where α is the name of an associated type, method, or lifetime (this last not actually implemented yet). Bar::α is a shorthand for <Self as Bar>::α where Bar is a supertrait of Foo.

In the future, we expect that α may also be a shorthand for <Self as Bar>::α (assuming we can resolve implementation issues and potential issues with macro hygiene). Since this would not be a backwards compatible change we might look at making code where this shorthand would introduce an ambiguity an error. The error case is where the programmer refers to α in a trait or impl, that is valid today (i.e., is in scope for some reason other than being in the supertrait), and α is also an associated type or method of a supertrait, but not overridden in the subtrait.

Note that within where clauses on a trait, the shorthands mentioned above do not apply, only within the body of the trait or impl. (We could revisit this in the future in a backwards compatible way if it is an issue).

@eddyb
Copy link
Member

eddyb commented Feb 13, 2015

cc me

@nrc
Copy link
Member

nrc commented Feb 13, 2015

After discussing with @eddyb on irc, it sounds like skipping the Trait::α sugar is a good idea for now. Reason is it makes possibly global paths context sensitive (in their exact semantics, if not in the meaning of the actual name). While it's not clear if that is a bad thing, it seems possible that it is and thus we shouldn't do it without more discussion/motivation. He also brought up the case of methods in inherent impls which would be inconsistent with the scheme in the above comment.

So, a revised spec: in any trait or impl, where α is the name of an associated type or method, α in type or expression position is shorthand for Self::α. That in turn is a shorthand for: <Self as Foo>::α in trait Foo { ... }; <Bar as Foo>::α in impl Foo for Bar { ... }; and Bar::α in impl Bar { ... } (note than in the inherent impl case, α may only be a method name, since their are no associated types, for now).

This has the downside (as previously discussed in person) that referring to an associated type in a super trait requires the Self:: prefix, but referring to one in the current trait does not. Therefore, it might be better to try an implement the shorthand for super traits too. It's probably worth thinking about if that is going to influence the implementation strategy.

(I actually have this (as previously spec'ed) working for associated types, but was having some difficulty with methods. It sounds like I'd be better off rebasing or starting again over @eddyb's UFCS branch. Using his partial resolution abstraction for these things sounds like a nice way forward).

@pnkfelix
Copy link
Member

leaving on 1.0 beta list, P-backcompat-lang. (i.e. we want the new behavior outlined in the bug description.)

@nrc
Copy link
Member

nrc commented Mar 29, 2015

A key question here is what to do where an associated item could shadow an outer item, e.g.,

type Foo = ...
trait Bar {
    type Foo;
    fn foo() -> Foo;   // <- which Foo does this refer to?
}

There are two options: we prefer the non-shorthand type of the module-level Foo or we imagine that the associated type Foo shadows the outer Foo. The latter seems the most expected behaviour, however, it is far more complex to implement. (There are a couple of other strategies too - we could consider this ambiguous and error out, but that has the same drawbacks, implementation-wise as preferring Self::Foo. We could prefer Self::Foo if the associated type is declared in the current trait, but not a super-trait (by using a more naive implementation strategy), but that is kind of awful). We also need to think about the expected behaviour for associated (static) methods.

The general implementation strategy here is that when resolving a path, we take note of whether Self is in scope (i.e., we're in a trait or impl) and if so we record two (well, n, really) possible resolutions - a partial resolution which assumes the Self:: shorthand and a regular resolution (in the example these are the two Foos). The former resolution is fully resolved during type checking, we then decided which resolution to use (this is something of a simplification, we arrange things in resolve such that if the partial resolution fully resolves, then it must be the correct resolution, otherwise we use the 'local' resolution). Note that the 'local' resolution may be no resolution at all, i.e., there is a resolve error if there is no Self:: resolution.

If this all worked, then the shadowing behaviour would fall out nicely. However, there are situations where even testing for Self:: shorthands causes an error, in particular, because of cycle detection (other cases can just defer the errors and fallback to the 'local' name resolution). For example, num::Int has a super-trait with a type parameter which has a bound of num::Int, and this is Ok because we don't need to fully resolve all the bounds to this depth in type checking as it currently stands. However, trying to resolve Self:: causes us to do so and detect this cycle. The only way I see to solve this is to defer this error too - so if we try to resolve the Self::Foo shorthand and hit an error, then we fallback to 'local' resolution. If that fails, then hopefully we can give the cycle error rather than just a bland name resolution error.

However, this seems kind of bad - by changing a parameter on a bound of super-trait (or something equally remote) we can cause name resolution errors in fairly unconnected places. Furthermore, whether you can use a Self:: shorthand or not in a given trait becomes essentially unpredictable (you need knowledge of all parameters and bounds on all super-traits, etc.). It also seems really unsatisfactory some how.

The alternative is a slightly weird scoping rule that non-associative items in an outer scope shadow unqualified, associated items from an inner scope. This breaks local reasoning somewhat, i.e., you need to know the entire context of an item to know whether you can use the shorthand or not. And adding an item in an arbitrary place can break the use of a shorthand (imagine a glob import in an outer module). OTOH, at least you don't need to know about bounds and super-traits.

The implementation is also much simpler - you only need to worry about the Self:: shorthand if name resolution fails otherwise. Furthermore, when finalising name resolution in type checking, you never need to fallback to another possible resolution, if finalisation fails, then you have an actual error (well, there might be more than one Self in scope, in which case you have to try multiple resolutions still, but we could simplify further and only take into account the innermost Self, which seems sensible since this is a shorthand and this is a kind of partial solution in any case because of the shadowing issue).

Note that relative to current Rust, the first solution is backwards incompatible and the second is backwards compatible. If we adopt the second solution, changing to the first is a backwards incompatible change.

@nrc
Copy link
Member

nrc commented Mar 29, 2015

So, questions, given the above, do we think it is worth the effort to adopt 'proper' shadowing of associated types, or is the 'associated types have lowest precedence' model sufficient? Does everyone still think having some shorthand is the way forward?

@nrc nrc removed the I-wrong label Mar 29, 2015
@aturon
Copy link
Member

aturon commented Mar 29, 2015

@nrc

However, this seems kind of bad - by changing a parameter on a bound of super-trait (or something equally remote) we can cause name resolution errors in fairly unconnected places. Furthermore, whether you can use a Self:: shorthand or not in a given trait becomes essentially unpredictable (you need knowledge of all parameters and bounds on all super-traits, etc.). It also seems really unsatisfactory some how.

Can you provide a minimal example of this problem? I'm having a hard time quite following the interactions here, or finding the relevant bit of Int. In particular, I was under the impression that explicit super traits were required to be acyclic:

trait Foo: Bar {}
trait Bar: Foo {}

fails to compile for this reason. So it seems that it should be possible to walk up the explicit trait hierarchy and discover all of the relevant items to bring into scope, and AFAIK this shouldn't involve bounds, but maybe I'm missing something?

So, questions, given the above, do we think it is worth the effort to adopt 'proper' shadowing of associated types, or is the 'associated types have lowest precedence' model sufficient? Does everyone still think having some shorthand is the way forward?

I think the "associated types have lowest precedence" option is a non-starter, personally: it's inconsistent with our general shadowing rules, and as you say loses the local reasoning about binding. I would rather forgo bringing the items into scope at all. (However, I still hope we can make the original semantics work out.)

@nrc
Copy link
Member

nrc commented Mar 29, 2015

@aturon, the relevant part of Int is uint and its bound (though I suspect that might be just the first one compilation hits, since a bunch of these are generated by macros). The minimal parts are something like:

trait Int: Shl<uint> + ... {}
impl Int for uint {}

Note that the cycle is not a simple one, but is via a type parameter. We don't need to resolve 'deeply' for normal type collection, so this is Ok. However, if we try to resolve Self::foo somewhere (I'm not exactly sure where the foo which we are shorthand checking comes from in the code, nor if it matters) we need to resolve a bit deeper, which runs in to the cycle which would otherwise be legal. It doesn't seem to me that this kind of cycle ought to be a problem, necessarily, but I bet there is a nasty edge case where it is. Assuming the existing cycle checking code is correct and not being overly conservative here, we have these weird Schroedinger's cat cycles, which are Ok until you look at Self::, making something visible or not based on that criteria (i.e., treat a cycle error as a name resolution error) makes me very uneasy.

By 'original semantics' do you mean the rules by which the scope of an associated type follows from Self? Or do you mean the syntactic approach discussed further up in this issue? That has issues, in particular with methods (you could only use the shorthand for local functions, not default methods from an impl or super-trait methods from a trait).

@aturon
Copy link
Member

aturon commented Mar 29, 2015

@nrc

Thanks for the clarification!

Note that the cycle is not a simple one, but is via a type parameter. We don't need to resolve 'deeply' for normal type collection, so this is Ok. However, if we try to resolve Self::foo somewhere (I'm not exactly sure where the foo which we are shorthand checking comes from in the code, nor if it matters) we need to resolve a bit deeper, which runs in to the cycle which would otherwise be legal. It doesn't seem to me that this kind of cycle ought to be a problem, necessarily, but I bet there is a nasty edge case where it is. Assuming the existing cycle checking code is correct and not being overly conservative here, we have these weird Schroedinger's cat cycles, which are Ok until you look at Self::, making something visible or not based on that criteria (i.e., treat a cycle error as a name resolution error) makes me very uneasy.

So, I fear I'm still missing something here. My expectation would be that the names brought into scope due to Shl<uint> are precisely the items provided by that trait and its supertraits, regardless of any bounds or impls. That is, I don't see why (conceptually) the impl of Int for uint and the uint parameter to Shl have any relevance to the scoping question at hand. But I suspect I'm being naive :-)

By 'original semantics' do you mean the rules by which the scope of an associated type follows from Self? Or do you mean the syntactic approach discussed further up in this issue? That has issues, in particular with methods (you could only use the shorthand for local functions, not default methods from an impl or super-trait methods from a trait).

Oh, sorry, I meant the semantics in #18764 (comment), which I believe is what you, @nikomatsakis and I discussed a while back. My understanding was that for beta we would focus on the items defined within the trait itself, and try to include supertrait methods as quickly as possible after that. I don't think I'm aware of the default method issues you're alluding to here, was that a recent discovery?

It sounds as if you've already abandoned the semantics we had discussed? If so, it seems I'm rather out of the loop :)

@nrc
Copy link
Member

nrc commented Mar 29, 2015

I think in an ideal world that is true (about the names brought into scope), but I think we need to know the type parameters in order to properly know those names. This might just be for the impl case - that is, inside an impl, Self::Assoc actually resolves to a concrete type which may depend on a type parameter (e.g., type Assoc = X::Foo where X is a type param). There might be cases in the trait case too, I'm not sure. In any case the code treats both cases the same in this respect, @nikomatsakis would have to let us know how necessary that is.

The idea that we would just look at associated items in the current trait is what I call the syntactic approach. It comes down to only looking at the associated items which are lexically declared in the current impl or trait. I think it is still feasible, but I was persuaded that it was even less optimal that we thought and that the semantic approach (with either kind of scoping) was easier than I originally thought (ha!).

Methods are an issue because expectations are a bit different, I think, it seems even more weird that whether you can write foo() or Self::foo() depends on whether foo is literally in lexical scope. Also because moving from the syntactic to semantic models is also backwards incompatible. I seem to remember I had other problems while implementing, but it was long enough ago that I can't remember them. Maybe I should continue along that track and see what happens...

@nikomatsakis
Copy link
Contributor

I don't think I understand the cycle that @nrc is describing yet. Perhaps it's too early and I've not had enough coffee. However, the reason that we sometimes do want to know more type parameters is because we compile an assoc type reference like Foo or even Self::Foo to <Self as TraitRef<T0>>::Foo. If this comes from a supertype, we may need to do some substitutions and so forth to find out the trait-reference. However, it's...probably possible to defer this precise expansion to typecheck, but it's certainly more complicated, particularly with the current setup. (@nrc and I were saying that how we would prefer to implement this is to just have resolve build up the necessary tables and have the actual lookup take place during type-checking.)

At some point @nrc and I discussed the idea of doing something that is ... mostly right now and then either reporting errors or having a known bug for the corner cases, to be resolved as we rework resolve. Not sure how viable that is right now.

@nikomatsakis
Copy link
Contributor

For the record I feel...fairly uncomfortable with a scoping rule that puts assoc type resolution as a fallback. It only seems like something we would consider because of the way that the impl works, I feel like it's not something we'd devise from first principles.

@nrc
Copy link
Member

nrc commented Mar 31, 2015

We decided at the weekly meeting today that we would stick with requiring Self:: rather than implementing this shorthand. If we decided we need this feature in the future, then we can add some kind of use statement to let us maintain backwards compatibility.

@nrc nrc closed this as completed Mar 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts.
Projects
None yet
Development

No branches or pull requests

9 participants