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

Handle ReVar in note_and_explain_region #125054

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

nnethercote
Copy link
Contributor

PR #124918 made this path abort. The added test, from fuzzing, identified that it is reachable.

r? @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2024
@nnethercote
Copy link
Contributor Author

Prior to #124918 there was this comment:

        // We shouldn't really be having unification failures with ReVar
        // and ReBound though.
        //
        // FIXME(@lcnr): Figure out whether this is reachable and if so, why.
        ty::ReVar(_) | ty::ReBound(..) | ty::ReErased => (format!("lifetime `{region}`"), alt_span),

We now know how ReVar can occur here. I don't know if this is considered reasonable or not.

I also am not sure if I chose a good spot for the new test.

@bors rollup

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. This seems a bit sus to me tbh

I don't know if this is considered reasonable or not.

I would like to understand more clearly why we're encountering ReVar in the signature of the function. It's not clearly correct to me, and it's possibly better fixed by modifying some other code.

Please also move "fix #124973" out of the title and into the body of the PR so it actually cross-links and closes the issue correctly, and give the PR a better title.

@lcnr lcnr assigned compiler-errors and unassigned lcnr May 13, 2024
@nnethercote
Copy link
Contributor Author

I would like to understand more clearly why we're encountering ReVar in the signature of the function. It's not clearly correct to me, and it's possibly better fixed by modifying some other code.

I am unlikely to be the person to make changes elsewhere :) Maybe @lcnr has an opinion on this?

@compiler-errors
Copy link
Member

Well, If you're too busy to look into what the root cause of this ICE is, I'd prefer if we close this PR and leave the underlying issue open instead of just changing it back to silently accept.

I don't think that it's urgent enough that we should land this without investigation, especially since this is of the Error -> ICE category and doesn't affect code that I expect "regular" usages of rustc to hit often.

@compiler-errors
Copy link
Member

Actually in the process of writing that comment out I think I figured out what the issue is. Give me a few mins; I'll put something up to fix this with a better diagnostic.

@nnethercote
Copy link
Contributor Author

I'm not too busy, I just don't know much about region analysis :)

@compiler-errors
Copy link
Member

compiler-errors commented May 14, 2024

So the tl;dr is:

let reg_vid = self
.infcx
.next_nll_region_var(FR, || RegionCtxt::Free(Symbol::intern("c-variadic")))
.as_var();

We create an anonymous free region here for the VaListImpl<'_> that c-variadic is implicitly lowered to.

Async functions capture that var arg list (not exactly certain how they end up doing that, though), and so the lifetime ends up being captured by the async fn's future. That ends up erroring because the future ends up capturing a lifetime that it didn't expect, which should probably not happen, but regardless is a problem because we have no way of providing a useful error message even if we don't expect it to work.

Probably won't be able to put up a PR to fix this, at least not tonight since it's 23:30, and am becoming more sympathetic to land this PR with a FIXME, a better PR title, and a follow-up issue filed about how the diagnostic isn't useful at all, so we can link it w/ the c_variadic tracking issue at least. Do you want to do that?

@compiler-errors
Copy link
Member

@rustbot author

see comment above for leaving a fixme and linking w/ a FIXME issue for bad diagnostics to the tracking issue

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024
@nnethercote nnethercote changed the title Fix #124973. Handle ReVar in note_and_explain_region May 23, 2024
PR rust-lang#124918 made this path abort. The added test, from fuzzing,
identified that it is reachable.
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 23, 2024

📌 Commit 5f4424b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2024
@nnethercote
Copy link
Contributor Author

I have updated the description, filed #125431 as a follow-up, and added a FIXME comment. Can you add the relevant labels/markings to the follow-up issue for the c_variadic tracking?

bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#122665 (Add some tests for public-private dependencies.)
 - rust-lang#123623 (Fix OutsideLoop's error suggestion: adding label `'block` for `if` block.)
 - rust-lang#125054 (Handle `ReVar` in `note_and_explain_region`)
 - rust-lang#125156 (Expand `for_loops_over_fallibles` lint to lint on fallibles behind references.)
 - rust-lang#125222 (Migrate `run-make/issue-46239` to `rmake`)
 - rust-lang#125316 (Tweak `Spacing` use)
 - rust-lang#125392 (Wrap Context.ext in AssertUnwindSafe)
 - rust-lang#125417 (self-contained linker: retry linking without `-fuse-ld=lld` on CCs that don't support it)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 72fd85c into rust-lang:master May 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup merge of rust-lang#125054 - nnethercote:fix-124973, r=compiler-errors

Handle `ReVar` in `note_and_explain_region`

PR rust-lang#124918 made this path abort. The added test, from fuzzing, identified that it is reachable.

r? `@lcnr`
@nnethercote nnethercote deleted the fix-124973 branch May 23, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants