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

for<'a> &'a T: 'a and closures regressed #98437

Open
nikomatsakis opened this issue Jun 23, 2022 · 12 comments
Open

for<'a> &'a T: 'a and closures regressed #98437

nikomatsakis opened this issue Jun 23, 2022 · 12 comments
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 23, 2022

In #98109, we fixed some bugs relating to NLL and for but apparently regressed a few examples. There are two tests in the test suite:

@aliemjay
Copy link
Member

aliemjay commented Jun 24, 2022

#96899 is the root cause of these regressions:

Note that both regressed crates are on nightly, but the repro can use stable for both.

@rustbot label regression-from-stable-to-nightly C-bug T-compiler

@rustbot rustbot added C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 24, 2022
@jackh726
Copy link
Member

jackh726 commented Jun 24, 2022

Okay, so timeline is:
1.61 -> pass
#96899 -> fail
1.62 cutoff
#95565 -> pass
#98109 -> fail

If we revert #96899, do they get fixed with remaining changes still in place?

edit: The problem is #96899 landed in beta and these pass there?

@compiler-errors
Copy link
Member

compiler-errors commented Jun 24, 2022

@jack726 we reverted #96899 on beta (#97642), so the earliest #96899 is landing is 1.63

@aliemjay
Copy link
Member

If we revert #96899, do they get fixed with remaining changes still in place?

I think yes. I'll check when I have time.

@jackh726
Copy link
Member

@jack726 we reverted #96899 on beta (#97642), so the earliest #96899 is landing is 1.63

Ah right. Well that explains things. Sounds like we need to think about this again (if another beta revert, or full revert, is warranted)

@apiraino
Copy link
Contributor

apiraino commented Jun 30, 2022

If it's ok, I'm going to nominate this issue for the T-compiler meeting for a discussion about reverting multiple PRs related to NLL, seems there are many bits involved.

@rustbot label +I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jun 30, 2022
@apiraino
Copy link
Contributor

apiraino commented Jul 4, 2022

Issue discussed by T-compiler on Zulip, adding their notes, opinion was that there's no need revert.

(Also, the Types team had a discussion about this issue)

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jul 4, 2022
@apiraino
Copy link
Contributor

apiraino commented Jul 7, 2022

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 7, 2022
@apiraino
Copy link
Contributor

Revisited during T-compiler meeting (Zulip discussion), downgrading priority since it has been decided this is an acceptable breakage.

@rustbot label +P-high -P-critical

@rustbot rustbot added P-high High priority and removed P-critical Critical priority labels Jul 28, 2022
@jackh726
Copy link
Member

We discussed this a bit in the t-types meeting today.

We basically concluded (as has already really been said a few times in a couple places) that #96899 was a soundness fix and not worth reverting for this regression. Also, the regression is small, weird enough, and has an easy fix; so, it's likely not worth investing time trying to dig into a fix. Especially given this will be "fixed" at some point in the future anyways by a better HRTB implied bounds story.

@pnkfelix
Copy link
Member

pnkfelix commented Aug 4, 2022

reassigning to T-types on basis that the eventual plan for fixing this is tied to a better HRTB implied bounds story.

@rustbot label: -T-compiler +T-types

@rustbot rustbot added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 4, 2022
@pnkfelix pnkfelix added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-types Relevant to the types team, which will review and decide on the PR/issue. labels Aug 4, 2022
@pnkfelix
Copy link
Member

If I understand correctly, this was first going to affect 1.62, but then we reverted PR #97642 solely for the 1.62.* release, and thus it affected 1.63.0 (which just was released today).

Thus, this has not been a stable-to-nightly regression for several weeks, it has been a stable-to-beta regression, and as of today it is a stable-to-stable regression.

@rustbot label: -regression-from-stable-to-nightly +regression-from-stable-to-stable

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 11, 2022
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-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants