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

Bounds in HRTB are ignored #42181

Closed
djzin opened this issue May 23, 2017 · 9 comments · Fixed by #48326
Closed

Bounds in HRTB are ignored #42181

djzin opened this issue May 23, 2017 · 9 comments · Fixed by #48326
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@djzin
Copy link
Contributor

djzin commented May 23, 2017

The syntax where for<'b: 'a> ... is permitted without warning, however the : 'a bound is not used to prove the identity; I would expect the following to compile:

trait Trait {}

impl Trait for &'static () {}

fn f<'a>(_: &'a ()) where for<'b: 'a> &'b () : Trait {}

fn main() {
    static X: &'static () = &();
    f::<'static>(X)
}

but it fails with

rustc 1.17.0 (56124baa9 2017-04-24)
error[E0277]: the trait bound `for<'b> &'b (): Trait` is not satisfied
 --> <anon>:9:5
  |
9 |     f::<'static>(X)
  |     ^^^^^^^^^^^^ the trait `for<'b> Trait` is not implemented for `&'b ()`
  |
  = help: the following implementations were found:
            <&'static () as Trait>
  = note: required by `f`

error: aborting due to previous error
@eddyb eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 7, 2017
@eddyb
Copy link
Member

eddyb commented Jun 7, 2017

This might be a dupe - either way, a potential backwards compat hazard. cc @nikomatsakis

@nikomatsakis
Copy link
Contributor

Yes, we should deal with this. Errors are best for now, though I hope to extend HRTB to support where-clauses.

@nikomatsakis
Copy link
Contributor

triage: P-medium

This is old. But let's fix it. Might be a good E-mentor candidate. I'll try to write-up some instructions.

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jun 8, 2017
@RalfJung
Copy link
Member

RalfJung commented Jul 12, 2017

Bounds in type aliases are similarily ignored:

type Test<'a, 'b: 'static> = &'a mut &'b mut i32;

fn main() {
    let x : Test = &mut &mut 0;
}

Is that a separate bug? This seems closer to whatever lead to E0122 being added, which however seems to have happened long ago.
(Funny enough, the compiler even tries to be helpful and tells me that I can just remove 'b and replace it by 'static -- which doesn't match how the Test type actually behaves.)

EDIT: Same for type Test<'a, 'b> where 'b: 'static = &'a mut &'b mut i32;.

@eddyb
Copy link
Member

eddyb commented Jul 12, 2017

(Funny enough, the compiler even tries to be helpful and tells me that I can just remove 'b and replace it by 'static -- which doesn't match how the Test type actually behaves.)

It only doesn't behave that way because the bounds are forgotten... which is a real shame.
IMO possibly the best path forward is to add more projections, including one for type aliases. That would allow recovering the bounds.

@RalfJung
Copy link
Member

IMO possibly the best path forward is to add more projections

What do you mean, add more projections?

@eddyb
Copy link
Member

eddyb commented Jul 12, 2017

@RalfJung Right now we only have <T as Trait>::Assoc projections. We could add a type alias projection, which would turn into the type being aliased at the first occasion, while producing the obligations from the bounds of the type alias.

@RalfJung
Copy link
Member

RalfJung commented Jul 12, 2017

Ah so you're not talking about a new syntax, just a different implementation strategy.

@eddyb
Copy link
Member

eddyb commented Jul 12, 2017

@RalfJung Right. The compiler has everything it needs, there just isn't a way to get the information to the uses. Inside functions it's not that hard, but outside there isn't really where to store it.

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 27, 2017
bors added a commit that referenced this issue Feb 22, 2018
Warn about ignored generic bounds in `for`

This adds a new lint to fix #42181. For consistency and to avoid code duplication, I also moved the existing "bounds in type aliases are ignored" here.

Questions to the reviewer:
* Is it okay to just remove a diagnostic error code like this? Should I instead keep the warning about type aliases where it is? The old code provided a detailed explanation of what's going on when asked, that information is now lost. On the other hand, `span_warn!` seems deprecated (after this patch, it has exactly one user left!).
* Did I miss any syntactic construct that can appear as `for` in the surface syntax? I covered function types (`for<'a> fn(...)`), generic traits (`for <'a> Fn(...)`, can appear both as bounds as as trait objects) and bounds (`for<'a> F: ...`).
* For the sake of backwards compatibility, this adds a warning, not an error. @nikomatsakis suggested an error in #42181 (comment), but I feel that can only happen in a new epoch -- right?

Cc @eddyb
bors added a commit that referenced this issue Mar 9, 2018
Warn about ignored generic bounds in `for`

This adds a new lint to fix #42181. For consistency and to avoid code duplication, I also moved the existing "bounds in type aliases are ignored" here.

Questions to the reviewer:
* Is it okay to just remove a diagnostic error code like this? Should I instead keep the warning about type aliases where it is? The old code provided a detailed explanation of what's going on when asked, that information is now lost. On the other hand, `span_warn!` seems deprecated (after this patch, it has exactly one user left!).
* Did I miss any syntactic construct that can appear as `for` in the surface syntax? I covered function types (`for<'a> fn(...)`), generic traits (`for <'a> Fn(...)`, can appear both as bounds as as trait objects) and bounds (`for<'a> F: ...`).
* For the sake of backwards compatibility, this adds a warning, not an error. @nikomatsakis suggested an error in #42181 (comment), but I feel that can only happen in a new epoch -- right?

Cc @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants