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

Rejecting higher-kinded lifetime bounds is a breaking change #50555

Closed
comex opened this issue May 9, 2018 · 6 comments
Closed

Rejecting higher-kinded lifetime bounds is a breaking change #50555

comex opened this issue May 9, 2018 · 6 comments

Comments

@comex
Copy link
Contributor

comex commented May 9, 2018

fn foo<'s, F>(s: &'s i32, f: F) where for<'a: 's> F: FnOnce(&'a i32) {
    f(s)
}

fn main() {
    foo(&42, |x| println!("{}", x));
}

Playground Link

This compiles on stable; on beta and above, it produces:

error: lifetime bounds cannot be used in this context

Apparently this is on purpose:

commit 49abd8748357012e5db10bf11077384f727e2177
Author: Ralf Jung <post@ralfj.de>
Date:   Tue Mar 6 11:22:24 2018 +0100

    make bounds on higher-kinded lifetimes a hard error in ast_validation
    
    Also move the check for not having type parameters into ast_validation.
    
    I was not sure what to do with compile-fail/issue-23046.rs: The issue looks like
    maybe the bounds actually played a role in triggering the ICE, but that seems
    unlikely given that the compiler seems to entirely ignore them.  However, I
    couldn't find a testcase without the bounds, so I figured the best I could do is
    to just remove the bounds and make sure at least that keeps working.

But it's still a gratuitous breaking change, which seems unfortunate… even if the lifetimes weren't being handled correctly before.

cc @RalfJung

@ExpHP
Copy link
Contributor

ExpHP commented May 9, 2018

This is in the 1.26 that's about to ship, right? Maybe it should be mentioned in the release notes?

I don't think it's too far of a stretch to say that there is live code out there with a for<'a: 'b> somewhere in it. Barely weeks ago, I myself was playing around with a for<'a, 'b: 'a> fn on the playground, wondering if I'd ever need to impl a trait for one.

@comex
Copy link
Contributor Author

comex commented May 9, 2018

For the record, I reported this after running into it with real code I was developing on stable. I needed to debug a macro, so I switched to nightly in order to use --pretty=expanded; then my code stopped compiling.

@RalfJung
Copy link
Member

RalfJung commented May 9, 2018

Yes it is a breaking change indeed, introduced in #48326. That's why we did a crater run, which came back with a single failure.

However, @comex your code is equivalent to

fn foo<'s, F>(s: &'s i32, f: F) where for<'a> F: FnOnce(&'a i32) {
...
}

That is, the bound you wrote is entirely ignored. So, this only breaks code that already did not do what the author thought it would. Given that so far, not many people seem to have made this mistake (writing lifetime bounds that are ignored), it seemed worth the breaking change to make sure no more people will do so in the future.

@comex
Copy link
Contributor Author

comex commented May 10, 2018

Welp, I didn't realize we were so close to a stable release containing the change in question. I guess this is now moot. But I wish the change had gone through a warning cycle.

@comex comex closed this as completed May 10, 2018
RSSchermer added a commit to RSSchermer/viemo that referenced this issue Apr 6, 2022
U cannot get this to work. It's close but I think it requires lifetime bounds on the HRTB lifetimes e.g. `F: for<'a, 'store: 'a> ...`, which currently isn't possible (seems to have been possible at one point? rust-lang/rust#50555 Could also be just that the syntax didn't result in an error, but had no semantic meaning attached to it.)
RSSchermer added a commit to RSSchermer/viemo that referenced this issue Apr 6, 2022
U cannot get this to work. It's close but I think it requires lifetime bounds on the HRTB lifetimes e.g. `F: for<'a, 'store: 'a> ...`, which currently isn't possible (seems to have been possible at one point? rust-lang/rust#50555 Could also be just that the syntax didn't result in an error, but had no semantic meaning attached to it.)
@RSSchermer
Copy link

Apologies for resurrecting an old issue; I find myself in need of for<'a, 'b: 'a> ... and my google-fu turned up this thread, which makes it seem like it might have been possible at some point.

Is this a false impression, was it just that the syntax didn't trigger an error, but no semantics were attached to it? Or was this indeed a working feature that was removed (presumably for soundness reasons)?

@RalfJung
Copy link
Member

RalfJung commented Apr 6, 2022

was it just that the syntax didn't trigger an error, but no semantics were attached to it?

That was indeed the case. The bound got parsed, and then ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants