small fix to closure debuginfo #38483
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
ahicks92
changed the title
|
I just hit the wrong button and submitted this without a description, sorry. EDIT(@eddyb): moved the rest of this comment to the PR description. |
|
r? @eddyb seems ok to me, but I'm probably not best to review |
alexcrichton
added
beta-nominated
|
@bors r+ |
|
📌 Commit e1d8806 has been approved by |
|
@bors: p=1 fixes a regression, so I think we'll want to land this soon hopefully |
Rollup of 29 pull requests - Successful merges: #37761, #38006, #38131, #38150, #38158, #38171, #38208, #38215, #38236, #38245, #38289, #38302, #38315, #38346, #38388, #38395, #38398, #38418, #38432, #38451, #38463, #38468, #38470, #38471, #38472, #38478, #38486, #38493, #38498 - Failed merges: #38271, #38483
|
Marking as beta-accepted. Small (if...dense) patch, regression. cc @rust-lang/compiler |
nikomatsakis
added
the
beta-accepted
|
Definitely we want a test here... |
|
@nikomatsakis IIRC we had a test but it wasn't running on the bots, @alexcrichton knows more. |
|
Oh yes #37429 should have been prevented from landing in the first place due to it causing a regression to an existing test that this PR fixes. That test was erroneously not running and has since been fixed to run on all PRs. In that sense I believe we've already got tests checked in for this. |
brson
removed
the
beta-nominated
|
I'm not sure what dense means but suspect it is bad; if you're referring to the description and the commit it's because I don't fully understand what's going on. @eddyb gets credit for this fix, and they tried to explain but not with much luck, I'm afraid. I forget which test specifically checks this, but there are already tests examining closure-local variables. |
|
@camlorn I didn't mean dense in a bad way, just meant that there are few lines changed, but the correctness of the changes is non-obvious -- although re-reading they actually look pretty simple. |
This is fallout from #37429. The faking we do to make closures appear to have local variables was broken and made it through CI because the debuginfo tests got turned off.
This probably needs a dedicated test, but I wanted to get this in as quickly as possible because we probably need to backport this to the beta.
r? @alexcrichton