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

GATs: repeating not-locally-required bounds on impls? #91348

Closed
dhardy opened this issue Nov 29, 2021 · 4 comments · Fixed by #91849
Closed

GATs: repeating not-locally-required bounds on impls? #91348

dhardy opened this issue Nov 29, 2021 · 4 comments · Fixed by #91849
Labels
A-GATs Area: Generic associated types (GATs) F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs

Comments

@dhardy
Copy link
Contributor

dhardy commented Nov 29, 2021

See: #87479

So, I believe #89970 broke my code kas-text (nightly rustc + --feature gat).

Let's focus on the iter function. The first thing to realize is that we know that Self: 'a because of &'a self

This isn't true in general, though it may be in specific instances. My code (reduced) exemplifies this perfectly:

pub trait FormattableText: std::fmt::Debug {
    type FontTokenIter<'a>: Iterator<Item = FontToken>;

    fn font_tokens<'a>(&'a self, dpp: f32, pt_size: f32) -> Self::FontTokenIter<'a>;
}

impl<'t> FormattableText for &'t str {
    type FontTokenIter<'a> = std::iter::Empty<FontToken>;

    fn font_tokens<'a>(&'a self, _: f32, _: f32) -> Self::FontTokenIter<'a> {
        std::iter::empty()
    }
}

Here, font_tokens may return an iterator over an instance of Self... but it also may not (&str does not have any formatting). A possible workaround in the impl is to write a custom iterator with a reference to the &str anyway, but this seems unnecessary.

@dhardy
Copy link
Contributor Author

dhardy commented Nov 29, 2021

Okay, I'm wrong that a workaround is needed: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=9c22eb9f05bc249d062e794a2f9b49ec

That said, I'm still not convinced the bound should be necessary.

@dhardy
Copy link
Contributor Author

dhardy commented Nov 29, 2021

Reading through the issue again, I would like to say a couple of things.

First, the error message for missing bound in the trait is clear about what to do (though it would be helpful if it also mentioned why / referenced extra doc), however the same is not true for implementations: here it's less clear what bound is required (type FontTokenIter<'a> where Self: 'a = std::iter::Empty<FontToken>; is not entirely obvious syntax; I know this is #90076 but the only relevant point here is telling the user what to use in the error message).

Further, putting a where Self: 'a bound on the impl works, but where 't: 'a does not even though apparently this says the same thing.

Finally, the point of #87479 IIUC is that <&'t str as FormattableText>::FontTokenIter<'static> would imply 't: 'static. Given a where Self: 'a bound this is true, but I still don't understand why that bound is required (though it is implicit in the usage fn font_tokens<'a>(&'a self, ..) -> Self::FontTokenIter<'a>)... aha, is it to make trait implementations where Self: 'a is required legal (since such an impl cannot have extra bounds over the trait)?

In that case, the chosen solution does not appear "rustic". I would expect, then, that (1) the bound is not required in the trait, but a lint warns that it is likely wanted (allowing opt-out) and (2) that writing an impl without this bound fails. (This may be over-complicated, but at least better educates the programmer about what is going on here.)

@jackh726 jackh726 added the F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs label Nov 29, 2021
@jackh726
Copy link
Member

jackh726 commented Nov 29, 2021

Okay, I'll try to respond to points individually, since there a lot here.

This isn't true in general, though it may be in specific instances. My code (reduced) exemplifies this perfectly:

While it's not true to say that every type for a GAT will need the Self: 'a bound to be well-formed, the idea behind the lint is that the required bounds are those that we know based on how it's returned from the methods in the trait. Specifically, the idea is that with the required bounds, any type assigned to the GAT can be returned from the functions that return it in their signature. Put another way, for type FontTokenIter<'a> = std::iter::Empty<FontToken> there is no 'a in the type itself, but there could be, and the required bounds on the trait permit that.

the error message for missing bound in the trait is clear about what to do ... however the same is not true for implementations

You're completely right here; the error messages are this aren't great (because it's hard). But that should get better in time.

Further, putting a where Self: 'a bound on the impl works, but where 't: 'a does not even though apparently this says the same thing.

This works for me.

Finally, the point of #87479 IIUC is that <&'t str as FormattableText>::FontTokenIter<'static> would imply 't: 'static. Given a where Self: 'a bound this is true, but I still don't understand why that bound is required (though it is implicit in the usage fn font_tokens<'a>(&'a self, ..) -> Self::FontTokenIter<'a>)... aha, is it to make trait implementations where Self: 'a is required legal (since such an impl cannot have extra bounds over the trait)?

Right, the required bounds basically boil down to the fact that impls can't have extra bounds, so we have to do our best to infer how a GAT will be used to allow the most flexibility in impls.

In that case, the chosen solution does not appear "rustic". I would expect, then, that (1) the bound is not required in the trait, but a lint warns that it is likely wanted (allowing opt-out) and (2) that writing an impl without this bound fails. (This may be over-complicated, but at least better educates the programmer about what is going on here.)

This was discussed quite a bit on/around #87479. The idea is to eventually make this "implied" and not make the user write these bounds at all. But in order for this to not be a breaking change, we have to require them now. If we've done the lint correctly and have the correct assumptions, then there shouldn't be any code that won't work with the required bounds but would otherwise (and vice versa) - but we can't necessarily guarantee that, so we can only be potentially more restrictive than for now.

Another alternative is to just do nothing - or "only" warn. This isn't ideal because errors around these errors can be difficult to diagnose and fix. And, moreso, if a library publishes a trait without these bounds, downstream crates can't do anything about it and it can severely limit impls.


Please let me know if I've missed anything here. Ultimately, there are tradeoffs here and I want to hear about how the outlives lint impacts different use cases.

@dhardy
Copy link
Contributor Author

dhardy commented Nov 30, 2021

No, that's a great response, thanks.

So somehow I had trouble putting the bounds on the impl. Given better error messages this wouldn't have been an issue.

While it's not true to say that every type for a GAT will need the Self: 'a bound to be well-formed

The point is simply that it was not initially clear the bounds on the trait are required to cover all possible implementations:

  • In the case of (struct) type parameters, impls may have more bounds than the type (and the impl bounds cannot be assumed from the struct bounds since that would make relaxing bounds on the struct fragile, and apparently we like explicitness)
  • In the case of GAT parameters, impls must have at least as many bounds as the trait. Is it ever valid to have more bounds than on the trait? No, thus the arguments for why the bounds must also appear on the impl is reduced to only explicitness.
  • In the case of trait method parameters, impls are permitted to have fewer bounds than the trait, though those bounds are still enforced when calling the methods.

Are the situations for GATs and trait method parameters not analogous? Why then must unused bounds be specified on the impls of GATs but not trait method parameters?

@dhardy dhardy changed the title GATs: where Self: 'a bound breaks usage with possible-iterators GATs: repeating not-locally-required bounds on impls? Nov 30, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 13, 2021
…r=nikomatsakis

GATs outlives lint: Try to prove bounds

Fixes rust-lang#91036
Fixes rust-lang#90888
Fixes rust-lang#91348 (better error + documentation to be added to linked issue)

Instead of checking for bounds directly, try to prove them in the associated type environment.

Also, add a bit of extra information to the error, including a link to the relevant discussion issue (rust-lang#87479). That should be edited to include a brief summary of the current state of the outlives lint, including a brief background. It also might or might not be worth it to bump this to a full error code at some point.

r? `@nikomatsakis`
@bors bors closed this as completed in 8487833 Dec 13, 2021
@fmease fmease added the A-GATs Area: Generic associated types (GATs) label Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-GATs Area: Generic associated types (GATs) F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants