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

regression: X does not live long enough #100544

Closed
Mark-Simulacrum opened this issue Aug 14, 2022 · 8 comments
Closed

regression: X does not live long enough #100544

Mark-Simulacrum opened this issue Aug 14, 2022 · 8 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Aug 14, 2022
@Mark-Simulacrum Mark-Simulacrum added this to the 1.64.0 milestone Aug 14, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 14, 2022
@ehuss
Copy link
Contributor

ehuss commented Aug 14, 2022

I think these are two separate (unrelated) issues.

#98835 had a crater run where these regressions were identified: #98835 (comment)

#99413 had a crater run where there were quite a lot of regressions, but nobody has reviewed it: #99413 (comment)

There are quite a few regressions in #99413, but I suspect that will need to be accepted since it is a soundness fix.

@estebank
Copy link
Contributor

From #98835 (comment):

The migration is trivial in most cases: removing lifetime annotations is sufficient, however innernet requires replacing Self constructor with the struct name.

I wish this had been flagged as a potential diagnostic issue back when this was identified. Filed #100572.

@jackh726
Copy link
Member

In theory, we could back out of #98835, maybe with a future compat warning.

@steffahn
Copy link
Member

steffahn commented Aug 16, 2022

I bisected charcoal and dkr, both pointing to caee496 which does contain #98835.
And I bisected operator, pointing to af7ab34 which contains #99413.

@steffahn
Copy link
Member

steffahn commented Aug 17, 2022

json2rss is also #99413
(by the way, I have opened PRs to multiple affected crates, including json2rss and operator, those two are already merged ^^)

FGS regresses at the same nightly as #98835, as does wonnx

ft-cli is most definitely #99413 (the handlebars method “render” was involved in a lot of other #99413-regressions, too). I didn’t bisect or send a patch, since the repo is archived and has lots of dependencies.

@rustbot label -E-needs-bisection

@aliemjay provided a minimal example of the #98835-regression in their PRs:

async fn test<'a>() { // `'a`, being a lifetime parameter, must be longer than the entire function body
    let f = |_: &'a str| {}; // the argument is required to outlive `'a`
    f(&String::new()); // an argument of shorter lifetime is passed!
}

For the #99413-regressions, we don’t need a mcve, since breakage was expected from the soundness fix.

@rustbot label -E-needs-mcve

@rustbot rustbot removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Aug 17, 2022
@seanpianka
Copy link

seanpianka commented Oct 3, 2022

Is this actually a regression? I saw it mentioned that this is a correctness fixed introduced as a part of #98835.

I think this issue is probably safe to close.

@apiraino
Copy link
Contributor

apiraino commented Oct 5, 2022

@seanpianka I think at this point I agree with your opinion (also by looking at this comment).

@jackh726 do you agree this can be closed without further action? is it a case of intended behaviour but perhaps poorly communicated to users? other interesting points to note here?

thanks!

@apiraino
Copy link
Contributor

apiraino commented Oct 26, 2022

I'll tentatively close this issue as it this seems now it's an accepted breakage that will need crates to update their code (example, the above mentioned #102937)

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants