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

Handle impl Trait where Trait has an assoc type with missing bounds #69707

Merged
merged 9 commits into from
Apr 12, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Mar 4, 2020

When encountering a type parameter that needs more bounds the trivial case is T where T: Bound, but it can also be an impl Trait param that needs to be decomposed to a type param for cleaner code. For example, given

fn foo(constraints: impl Iterator) {
    for constraint in constraints {
        println!("{:?}", constraint);
    }
}

the previous output was

error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
1 | fn foo(constraints: impl Iterator) {
  |                                    - help: consider further restricting the associated type: `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug`
2 |     for constraint in constraints {
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`

which is incorrect as where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug is not valid syntax nor would it restrict the positional impl Iterator parameter if it were.

The output being introduced is

error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This suggestion is correct and lead the user in the right direction: because you have an associated type restriction you can no longer use impl Trait, the only reasonable alternative is to introduce a named type parameter, bound by Trait and with a where binding on the associated type for the new type parameter as Trait for the missing bound.

Ideally, we would want to suggest something like the following, but that is not valid syntax today

error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo(constraints: impl Iterator<Item: std::fmt::Debug>) {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Fix #69638.

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2020
@estebank estebank added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 6, 2020
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@estebank
Copy link
Contributor Author

r? @Centril this is a bit of a hack, I'd love it if you could find ways to clean up this.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments -- I can't say I followed everything on a first pass, but addressing these comments might help me.

let pred = trait_ref.without_const().to_predicate().to_string();
let pred = pred.replace(&impl_name, "T");
let mut sugg = vec![
match generics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I'd appreciate sprinkling more comments on the new code as its not clear what picture the parts are painting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation for what's going on here are in the match arms, namely figuring out whether there are type params already to add to them or suggest introducing type params. I think we might already have code for this elsewhere though.

@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2020
@Centril

This comment has been minimized.

@estebank estebank force-pushed the impl-trait-missing-bounds branch 2 times, most recently from 4862417 to 2f9aedd Compare April 5, 2020 00:58
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2020
@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2020
Comment on lines 179 to 187
ty::Param(param) => {
// `fn foo(t: impl Trait)`
// ^^^^^ get this string
param
.name
.as_str()
.strip_prefix("impl")
.map(|s| (s.trim_start().to_string(), sig))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looked at the whole diff, you have the hir::Generics, and you could either use that (tricky) or you could pass in the predicates_of for the item. Which, I think if you instantiate_identity, you get a flat list of all of the predicates in scope, and they should still have Spans.

You can presumably search for trait_ref in the list of predicate and get the corresponding Span.
Which should be exactly the Foo in impl Foo + Bar, that corresponds to the trait that's missing an associated type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonus: you can probably use the Spans in predicates_of for other suggestions elsewhere - their initial purpose was automatically applicable suggestions for type aliases not being WF, back in late 2018, before that stuff fell through the f loor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I added span labels for each TraitPredicate, and this is what I got:

error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
  --> src/test/ui/suggestions/impl-trait-with-missing-bounds.rs:6:13
   |
4  | fn foo(constraints: impl  Iterator) {
   |                     --------------
   |                     |     |
   |                     |     Binder(TraitPredicate(<impl Iterator as std::iter::Iterator>)) NotConst
   |                     Binder(TraitPredicate(<impl Iterator as std::marker::Sized>)) NotConst
5  |     for constraint in constraints {
6  |         qux(constraint);
   |             ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
...
27 | fn qux(_: impl std::fmt::Debug) {}
   |    ---         --------------- required by this bound in `qux`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
4  | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I can see how it could be useful for some things, but the problem I see is that because this is an assoc type bound instead of setting it a type we cannot use the sytnax impl Iterator<Item = Something>. These spans would be great for that, but in this case we need to sadly rely only on extending the where clause to <X as Iterator>::Item: Trait.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two options: switching to impl Iterator<Item = T> where T is a new type parameter and adding a T: Trait bound, or impl Iterator<Item: Trait>, although I'm not sure the latter works yet or when it will work and it's probably still unstable anyway.

Anyway I was commenting only on the approach of getting the trait bound from the ty::Param "name": my point is that you can get its definition and work with that, which lets you get the real snippet where impl Foo+Bar was written (and each Foo and Bar in that if you look in the predicates_of).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impl Iterator<Item: Trait>

Yeah, that's not valid today.

Anyway I was commenting only on the approach

I understand.

which lets you get the real snippet where impl Foo+Bar was written

The current output for a impl Foo + Bar case is the following:

error[E0277]: `<impl Iterator + std::fmt::Debug as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
  --> src/test/ui/suggestions/impl-trait-with-missing-bounds.rs:37:13
   |
37 |         qux(constraint);
   |             ^^^^^^^^^^ `<impl Iterator + std::fmt::Debug as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
...
42 | fn qux(_: impl std::fmt::Debug) {}
   |    ---         --------------- required by this bound in `qux`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator + std::fmt::Debug as std::iter::Iterator>::Item`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
35 | fn bak<T: Iterator + std::fmt::Debug>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We need the whole Foo + Bar for the type param T: Foo + Bar. The only place where having the span to Foo for its snippet could be useful as things stand now is for the suggestion message where we now use <impl Foo + Bar as Foo>::AssocType where we could say <_ as Foo>::AssocType instead, but I feel that leaving it as it is is reasonable.

I think suggesting fn foo<T: Debug>(constraints: impl Iterator<Item = T>) {} is reasonable, but at that point the only thing left is the tension between ease of implementation of the suggestion and stylistic preference. The later seems to be more stylistically preferable, but don't know what the code to suggest it will look like yet.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 10, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 10, 2020
… r=Centril

Handle `impl Trait` where `Trait` has an assoc type with missing bounds

When encountering a type parameter that needs more bounds the trivial case is `T` `where T: Bound`, but it can also be an `impl Trait` param that needs to be decomposed to a type param for cleaner code. For example, given

```rust
fn foo(constraints: impl Iterator) {
    for constraint in constraints {
        println!("{:?}", constraint);
    }
}
```

the previous output was

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
1 | fn foo(constraints: impl Iterator) {
  |                                    - help: consider further restricting the associated type: `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug`
2 |     for constraint in constraints {
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
```

which is incorrect as `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug` is not valid syntax nor would it restrict the positional `impl Iterator` parameter if it were.

The output being introduced is

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

This suggestion is correct and lead the user in the right direction: because you have an associated type restriction you can no longer use `impl Trait`, the only reasonable alternative is to introduce a named type parameter, bound by `Trait` and with a `where` binding on the associated type for the new type parameter `as Trait` for the missing bound.

*Ideally*, we would want to suggest something like the following, but that is not valid syntax today

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo(constraints: impl Iterator<Item: std::fmt::Debug>) {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Fix rust-lang#69638.
@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

Seems to have failed tests in #71012, @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 10, 2020
@estebank
Copy link
Contributor Author

@bors r=Centril had to rebase and re-bless because of small change to error output in master

@bors
Copy link
Contributor

bors commented Apr 11, 2020

📌 Commit 984aac6 has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 12, 2020
… r=Centril

Handle `impl Trait` where `Trait` has an assoc type with missing bounds

When encountering a type parameter that needs more bounds the trivial case is `T` `where T: Bound`, but it can also be an `impl Trait` param that needs to be decomposed to a type param for cleaner code. For example, given

```rust
fn foo(constraints: impl Iterator) {
    for constraint in constraints {
        println!("{:?}", constraint);
    }
}
```

the previous output was

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
1 | fn foo(constraints: impl Iterator) {
  |                                    - help: consider further restricting the associated type: `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug`
2 |     for constraint in constraints {
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
```

which is incorrect as `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug` is not valid syntax nor would it restrict the positional `impl Iterator` parameter if it were.

The output being introduced is

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

This suggestion is correct and lead the user in the right direction: because you have an associated type restriction you can no longer use `impl Trait`, the only reasonable alternative is to introduce a named type parameter, bound by `Trait` and with a `where` binding on the associated type for the new type parameter `as Trait` for the missing bound.

*Ideally*, we would want to suggest something like the following, but that is not valid syntax today

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo(constraints: impl Iterator<Item: std::fmt::Debug>) {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Fix rust-lang#69638.
@bors
Copy link
Contributor

bors commented Apr 12, 2020

⌛ Testing commit 984aac6 with merge 32fb4dc...

@bors
Copy link
Contributor

bors commented Apr 12, 2020

☀️ Test successful - checks-azure
Approved by: Centril
Pushing 32fb4dc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 12, 2020
@bors bors merged commit 32fb4dc into rust-lang:master Apr 12, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #69707!

Tested on commit 32fb4dc.
Direct link to PR: #69707

🎉 rls on linux: test-fail → test-pass (cc @Xanewok).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 12, 2020
Tested on commit rust-lang/rust@32fb4dc.
Direct link to PR: <rust-lang/rust#69707>

🎉 rls on linux: test-fail → test-pass (cc @Xanewok).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestions for refining impl Trait argument are invalid Rust code
6 participants