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

Trait bounds are not yet enforced in type definitions #21903

Open
Marwes opened this Issue Feb 3, 2015 · 24 comments

Comments

Projects
None yet
@Marwes
Copy link
Contributor

Marwes commented Feb 3, 2015

Opening an issue since I could not find any information on when/if this will be enforced. Would be really useful for some generalizing I am trying for parser-combinators which I can't do currently without complicating the API.

Specifically I'd like to use a trait bound in a type definition to access an associated type to avoid passing it seperately but due to this issue I simply get a compiler error.

Simplified code which exhibits the error:

trait Stream {
    type Item;
}

trait Parser {
    type Input: Stream;
}

fn test<T>(u: T) -> T
    where T: Parser {
    panic!()
}

type Res<I: Stream> = (I, <I as Stream>::Item);

impl <'a> Stream for &'a str { type Item = char; }
impl <I> Parser for fn (I) -> Res<I> where I: Stream { type Input = I; }

//Works
//fn f(_: &str) -> (&str, char) {

//Errors
fn f(_: &str) -> Res<&str> {
    panic!()
}

fn main() {
    let _ = test(f as fn (_) -> _);
}
@jroesch

This comment has been minimized.

Copy link
Member

jroesch commented Feb 3, 2015

At first glance this is probably related to the internal messy-ness of bounds vs. where-clauses. I'm not sure if the predicate constraints for type aliases are propagated correctly. You should be able to use a newtype as a work around:

struct Res<I: Stream>((I, <I as Stream>::Item))

Seems to work just fine on playground with the newtype: http://is.gd/krOTZv.

cc @nikomatsakis

@Marwes

This comment has been minimized.

Copy link
Contributor Author

Marwes commented Feb 3, 2015

I would use a newtype but since the type I want to define is a specialized Result type that isn't possible in my case. I didn't really mention that since it is tangential to the bug itself.

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Apr 22, 2015

Looks like warnings are now generated for this case, but not consistently[0].

Should this be a compile-time error instead?

[0] #20222 (comment)

@Marwes

This comment has been minimized.

Copy link
Contributor Author

Marwes commented Aug 9, 2015

What is the status on trait bounds in type definitions? I worked around it the best I could for combine but it won't be as clean as it could be until bounds are allowed.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Nov 26, 2015

Where bounds are allowed, they're not checked properly.

I'm using this one and it compiles from Rust 1.0 or later

pub type MapFn<I, B> where I: Iterator = iter::Map<I, fn(I::Item) -> B>;
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 15, 2015

Nominating for discussion. Seems like backcompat hazard we should consider trying to address with a feature gate.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Dec 15, 2015

Since the bounds provide writing types you couldn't do otherwise, they are very useful. A feature gate would be a breaking change without recourse(?).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 17, 2015

triage: P-medium

I think this might be not that hard to fix though!

@nixpulvis

This comment has been minimized.

Copy link

nixpulvis commented Feb 9, 2016

Unless I'm missing something, this isn't a huge problem because bad things will still fail to type check right? For example the following doesn't compile.

trait Foo {
    fn foo(&self) -> u32 {
        1
    }
}

type Bar<F: Foo> = F;

fn foo(f: Bar<&str>) -> Bar<&str> {
    f
}

fn main() {
    let bar: Bar<&str> = "";
    println!("{:?}", foo(bar.foo()));
}
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Feb 9, 2016

@nixpulvis I think the argument for checking type definitions is analogous to our philosophy about other polymorphic code: we don't do C++ style post template-instantiation type-checking; instead we check the polymorphic code at the definition site (even though both strategies are sound)

Or maybe you are just saying the backwards incompatibility hit won't be that bad? (To which I say: famous last words. .. ;)

@nixpulvis

This comment has been minimized.

Copy link

nixpulvis commented Feb 9, 2016

@pnkfelix I was really just posting to confirm that I wasn't missing something. The warning sounds a bit scarier than it is in reality. I think this is tangental to #30503, as I do feel having type aliases which aren't check could be useful.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 25, 2016

So this issue is a regular annoyance. It's worth trying to figure out just what we want to do, however!

The key problem is that type aliases are "invisible" to the type checker. You could almost just remove where-clauses from them altogether, but that we need them to resolve associated types in things like type Foo<I: Iterator> = I::Item. Interestingly, in cases like that, if in fact Foo<X> is applied to some type X where X: Iterator does not hold, the code should still fail to compile today, despite that warning that the trait bound is "unenforced". This is because the trait bound is implied by the type itself, which expands to <I as Iterator>::Item.

Enforcing all type bounds could be tricky, but may well be readily achievable. There is some backwards compatibility to worry about. I guess how we would implement it would be to extend the "item type" associated with a type item. This might be a small patch, actually.

Another option would be to detect if the bound is needed (i.e., consumed by an associated type), and warn only if that is not the case. That would just be accepting that we allow where-clauses here even when they are not needed and will not be unenforced. Doesn't feel very good though.

Seems like we might want to start by trying for the "better" fix, and then measuring the impact...?

ncalexan added a commit to ncalexan/combine that referenced this issue Feb 17, 2017

Add map functions for Error<> and Info<> ranges. (Marwes#86)
It's possible to `map_err_range` for `ParseResult<>` too, but it's
awkward because the output type needs to be a compatible `StreamOnce`.
As suggested in
Marwes#86 (comment),
it's probably best to either change the parse result type entirely, or
wait for rust-lang/rust#21903.

This at least helps consumers convert `ParseError<>` into something
that can implement `std::fmt::Display`.

ncalexan added a commit to ncalexan/combine that referenced this issue Feb 17, 2017

Add map functions for Error<> and Info<> ranges. (Marwes#86)
It's possible to `map_err_range` for `ParseResult<>` too, but it's
awkward because the output type needs to be a compatible `StreamOnce`.
As suggested in
Marwes#86 (comment),
it's probably best to either change the parse result type entirely, or
wait for rust-lang/rust#21903.

This at least helps consumers convert `ParseError<>` into something
that can implement `std::fmt::Display`.
@RReverser

This comment has been minimized.

Copy link
Contributor

RReverser commented Feb 17, 2017

Interestingly, in cases like that, if in fact Foo is applied to some type X where X: Iterator does not hold, the code should still fail to compile today, despite that warning that the trait bound is "unenforced". This is because the trait bound is implied by the type itself, which expands to ::Item.

@nikomatsakis Would it be possible to remove that warning for these cases then? It's quite confusing.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 21, 2017

@RReverser I agree. I just added a to do item to try and investigate disabling the warnings.

@Twey

This comment has been minimized.

Copy link

Twey commented Mar 15, 2017

Workaround: if you're using an associated type on the type parameter, you can replace type T<U: Bound> = V<U::Assoc> with type T<U> = V<<U as Bound>::Assoc>, which avoids this warning until there's a way to disable it.

(Interestingly, even #[allow(warnings)] fails to disable this warning.)

@RReverser

This comment has been minimized.

Copy link
Contributor

RReverser commented Mar 15, 2017

(Interestingly, even #[allow(warnings)] fails to disable this warning.)

Yeah, was exactly my problem. Thanks for the workaround, I'll give it a try!

@RReverser

This comment has been minimized.

Copy link
Contributor

RReverser commented Mar 20, 2017

@Twey thank you, that did it!

RReverser added a commit to RReverser/serde-xml-rs that referenced this issue Mar 20, 2017

Workaround alias generics warning
Trick taken from comments in rust-lang/rust#21903

@petrochenkov petrochenkov referenced this issue Oct 5, 2017

Merged

trait alias infrastructure #45047

7 of 10 tasks complete

bors added a commit that referenced this issue Dec 13, 2017

Auto merge of #45047 - durka:trait-alias, r=petrochenkov
trait alias infrastructure

This will be an implementation of trait aliases (RFC 1733, #41517).

Progress so far:

- [x] Feature gate
- [x] Add to parser
  - [x] `where` clauses
    - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment)
- [x] Add to AST and HIR
  - [x] make a separate PathSource for trait alias contexts #45047 (comment)
- [x] Stub out enough of typeck and resolve to just barely not ICE

Postponed:

- [ ] Actually implement the alias part
- [ ] #21903
- [ ] #24010

I need some pointers on where to start with that last one. The test currently does this:

```
error[E0283]: type annotations required: cannot resolve `_: CD`
  --> src/test/run-pass/trait-alias.rs:34:16
   |
34 |     let both = foo();
   |                ^^^
   |
   = note: required by `foo`
```

bors added a commit that referenced this issue Dec 13, 2017

Auto merge of #45047 - durka:trait-alias, r=petrochenkov
trait alias infrastructure

This will be an implementation of trait aliases (RFC 1733, #41517).

Progress so far:

- [x] Feature gate
- [x] Add to parser
  - [x] `where` clauses
    - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment)
- [x] Add to AST and HIR
  - [x] make a separate PathSource for trait alias contexts #45047 (comment)
- [x] Stub out enough of typeck and resolve to just barely not ICE

Postponed:

- [ ] Actually implement the alias part
- [ ] #21903
- [ ] #24010

I need some pointers on where to start with that last one. The test currently does this:

```
error[E0283]: type annotations required: cannot resolve `_: CD`
  --> src/test/run-pass/trait-alias.rs:34:16
   |
34 |     let both = foo();
   |                ^^^
   |
   = note: required by `foo`
```

bors added a commit that referenced this issue Dec 13, 2017

Auto merge of #45047 - durka:trait-alias, r=petrochenkov
trait alias infrastructure

This will be an implementation of trait aliases (RFC 1733, #41517).

Progress so far:

- [x] Feature gate
- [x] Add to parser
  - [x] `where` clauses
    - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment)
- [x] Add to AST and HIR
  - [x] make a separate PathSource for trait alias contexts #45047 (comment)
- [x] Stub out enough of typeck and resolve to just barely not ICE

Postponed:

- [ ] Actually implement the alias part
- [ ] #21903
- [ ] #24010

I need some pointers on where to start with that last one. The test currently does this:

```
error[E0283]: type annotations required: cannot resolve `_: CD`
  --> src/test/run-pass/trait-alias.rs:34:16
   |
34 |     let both = foo();
   |                ^^^
   |
   = note: required by `foo`
```

bors added a commit that referenced this issue Dec 14, 2017

Auto merge of #45047 - durka:trait-alias, r=petrochenkov
trait alias infrastructure

This will be an implementation of trait aliases (RFC 1733, #41517).

Progress so far:

- [x] Feature gate
- [x] Add to parser
  - [x] `where` clauses
    - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment)
- [x] Add to AST and HIR
  - [x] make a separate PathSource for trait alias contexts #45047 (comment)
- [x] Stub out enough of typeck and resolve to just barely not ICE

Postponed:

- [ ] Actually implement the alias part
- [ ] #21903
- [ ] #24010

I need some pointers on where to start with that last one. The test currently does this:

```
error[E0283]: type annotations required: cannot resolve `_: CD`
  --> src/test/run-pass/trait-alias.rs:34:16
   |
34 |     let both = foo();
   |                ^^^
   |
   = note: required by `foo`
```

bors added a commit that referenced this issue Dec 14, 2017

Auto merge of #45047 - durka:trait-alias, r=petrochenkov
trait alias infrastructure

This will be an implementation of trait aliases (RFC 1733, #41517).

Progress so far:

- [x] Feature gate
- [x] Add to parser
  - [x] `where` clauses
    - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment)
- [x] Add to AST and HIR
  - [x] make a separate PathSource for trait alias contexts #45047 (comment)
- [x] Stub out enough of typeck and resolve to just barely not ICE

Postponed:

- [ ] Actually implement the alias part
- [ ] #21903
- [ ] #24010

I need some pointers on where to start with that last one. The test currently does this:

```
error[E0283]: type annotations required: cannot resolve `_: CD`
  --> src/test/run-pass/trait-alias.rs:34:16
   |
34 |     let both = foo();
   |                ^^^
   |
   = note: required by `foo`
```
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 10, 2018

(Interestingly, even #[allow(warnings)] fails to disable this warning.)

With #48326 merged, this warning is now a normal lint and can be disabled with #[allow(warnings)].

However, what I did not realize is that the bounds are actually not entirely ignored -- type T<U: Bound> = U::Assoc compiles while type T<U> = U::Assoc does not. I'm currently preparing a PR to at least improve the wording (and sorry for the mess), but I am wondering what the best strategy is here.

Given that this bug is unfixed for three years, it seems overly optimistic to expect a proper implementation of enforcing these bounds any time soon. Also, I think the warning is crucial because currently, the compiler accepts code such as the following:

use std::cell::Cell;
type SVec<T: Send> = Vec<T>;
fn foo(_x: SVec<&Cell<i32>>) {}
pub fn bar() {
  foo(Vec::new());
}

This is clearly nonsensical -- a type declared with SVec<T: Send> should only be allowed to contain Send types. This is also a footgun, because a user could expect the compiler to actually enforce this bound! I think this code should be a hard error eventually (e.g. in the next epoch).

The best option I can think of currently is to make people write <T as Trait>::Assoc. That seems to be the best practice as of now. It is slightly surprising that you can use T as Trait without having T: Trait, but that is consistent with being able to write

struct Sendable<T: Send>(T);
type MySendable<T> = Sendable<T>; // no error here!

So, if type aliases are ever checked for well-formedness, that would require a transition period anyway.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 12, 2018

Given that this bug is unfixed for three years, it seems overly optimistic to expect a proper implementation of enforcing these bounds any time soon. Also, I think the warning is crucial because currently, the compiler accepts code such as the following:

My expectation here has been that we will do the following:

(1) Implement lazy normalization (underway)
(2) In Epoch 2018 (hopefully we get this done in time...), handle type aliases not as "eagerly normalizing" in the type-checker, but rather as "lazy normalizing", just like an associated type
(2a) A side-effect would be strict enforcement of bounds
(2b) But in Epoch 2015, we would just warn and treat as we do today

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 12, 2018

The best option I can think of currently is to make people write <T as Trait>::Assoc. That seems to be the best practice as of now. It is slightly surprising that you can use T as Trait without having T: Trait, but that is consistent with being able to write

Yes, that's probably the most forwards compatible thing you can do, and yes it's somewhat surprising (but consistent). I tend to make an alias in practice just so it is shorter (and to not get warnings):

type Assoc<T> = <T as Trait>::Assoc;
type SomethingElse<T> = Vec<Assoc<T>>;
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 12, 2018

(1) Implement lazy normalization (underway)
(2) In Epoch 2018 (hopefully we get this done in time...), handle type aliases not as "eagerly normalizing" in the type-checker, but rather as "lazy normalizing", just like an associated type

Yes, that's probably the most forwards compatible thing you can do, and yes it's somewhat surprising

Okay, so given that, do you think the approach I took in #48909 makes sense?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 13, 2018

@RalfJung are you asking if your suggestion to use <T as Trait>::Foo form makes sense? I think it does, I mean -- as you said -- we can't exactly break that usage, at least not without some kind of opt-in, for back-compat reasons. I can take a look at the PR in terms of giving an improved suggestion.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Mar 13, 2018

@RalfJung are you asking if your suggestion to use ::Foo form makes sense?

Yes. Originally I intended this to be a step towards turning the lint into a hard error eventually, but of course if this gets implemented properly that's even better. :) I can't imagine how to implement this in a nice backwards-compatible way, but well, you seem to have some idea :D

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018

Rollup merge of rust-lang#48909 - RalfJung:type_alias_bounds, r=petro…
…chenkov

Improve lint for type alias bounds

First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So:
```rust
type T1<U: Bound> = U::Assoc; // compiles
type T2<U> = U::Assoc; // fails
type T3<U> = <U as Bound>::Assoc; // "correct" way to write this, maybe?
```
I am sorry for creating this mess.

This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like `T1` above and show a helpful message to the user:
```
warning: bounds on generic parameters are not enforced in type aliases
  --> $DIR/type-alias-bounds.rs:57:12
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |            ^^^^^
   |
   = help: the bound will not be checked when the type alias is used, and should be removed
help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases
  --> $DIR/type-alias-bounds.rs:57:21
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |                     ^^^^^^^^
```
I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait `Assoc` belongs to above. The lint does not even run if that resolution fails, so I assume that information is available *somewhere*...

This is a follow-up for (parts of) rust-lang#48326. Also see rust-lang#21903.

This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018

Rollup merge of rust-lang#48909 - RalfJung:type_alias_bounds, r=petro…
…chenkov

Improve lint for type alias bounds

First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So:
```rust
type T1<U: Bound> = U::Assoc; // compiles
type T2<U> = U::Assoc; // fails
type T3<U> = <U as Bound>::Assoc; // "correct" way to write this, maybe?
```
I am sorry for creating this mess.

This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like `T1` above and show a helpful message to the user:
```
warning: bounds on generic parameters are not enforced in type aliases
  --> $DIR/type-alias-bounds.rs:57:12
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |            ^^^^^
   |
   = help: the bound will not be checked when the type alias is used, and should be removed
help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases
  --> $DIR/type-alias-bounds.rs:57:21
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |                     ^^^^^^^^
```
I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait `Assoc` belongs to above. The lint does not even run if that resolution fails, so I assume that information is available *somewhere*...

This is a follow-up for (parts of) rust-lang#48326. Also see rust-lang#21903.

This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018

Rollup merge of rust-lang#48909 - RalfJung:type_alias_bounds, r=petro…
…chenkov

Improve lint for type alias bounds

First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So:
```rust
type T1<U: Bound> = U::Assoc; // compiles
type T2<U> = U::Assoc; // fails
type T3<U> = <U as Bound>::Assoc; // "correct" way to write this, maybe?
```
I am sorry for creating this mess.

This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like `T1` above and show a helpful message to the user:
```
warning: bounds on generic parameters are not enforced in type aliases
  --> $DIR/type-alias-bounds.rs:57:12
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |            ^^^^^
   |
   = help: the bound will not be checked when the type alias is used, and should be removed
help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases
  --> $DIR/type-alias-bounds.rs:57:21
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |                     ^^^^^^^^
```
I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait `Assoc` belongs to above. The lint does not even run if that resolution fails, so I assume that information is available *somewhere*...

This is a follow-up for (parts of) rust-lang#48326. Also see rust-lang#21903.

This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.

bors added a commit that referenced this issue Sep 9, 2018

Auto merge of #54090 - eddyb:reverse-waffles-implicit-sized, r=<try>
[WIP]  rustc_typeck: ensure type alias bounds are implied by the type being well-formed.

**NOTE**: this is mainly for crater, we'll only emit hard errors on Rust 2018.
This is the second half to a plan outlined in comment on #49441 (see #54033 for the first half).

Fixes #21903 (by disallowing currently unenforceable bounds instead of enforcing them).

This PR includes two hacks: first to not check (usually implied) `T: Sized` bounds, as they're a common source of breakage and would probably be too annoying in practice, and a second to remove some overlap with #54033, so adding missing bounds is *not* required to test this PR.
A decision should be made about the former hack, while the latter is only a temporary convenience.

r? @nikomatsakis
@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Oct 10, 2018

Any progress here lately? (Trying to have a go at implementing trait aliases here.)

CC @nikomatsakis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.