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

Poor interaction between NLL-borrowck, async, and c_variadic's ... desugaring (VaListImpl<'_>) #125431

Open
nnethercote opened this issue May 23, 2024 · 6 comments
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. F-c_variadic `#![feature(c_variadic)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nnethercote
Copy link
Contributor

nnethercote commented May 23, 2024

#124918 made note_and_explain_region abort if ReVar occurred. Fuzzing found a test case that triggers the abort in #124973, having to do with the c_variadic feature:

async fn multiple_named_lifetimes(_: u8, ...) {}

ReVar was re-allowed in #125054.

@compiler-errors had the following analysis:

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... the diagnostic isn't useful at all.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 23, 2024
@nnethercote nnethercote added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 23, 2024
@compiler-errors compiler-errors added A-async-await Area: Async & Await requires-nightly This issue requires a nightly compiler in some way. F-c_variadic `#![feature(c_variadic)]` labels May 23, 2024
@compiler-errors compiler-errors changed the title ReVar shouldn't occur in note_and_explain_region Poor interaction between NLL-borrowck and c_variadic's ... desugaring (VaListImpl<'_>) May 23, 2024
@compiler-errors compiler-errors changed the title Poor interaction between NLL-borrowck and c_variadic's ... desugaring (VaListImpl<'_>) Poor interaction between NLL-borrowck, async, and c_variadic's ... desugaring (VaListImpl<'_>) May 23, 2024
@tmandry
Copy link
Member

tmandry commented Jun 6, 2024

I can't reproduce the abort. Playground

   Compiling playground v0.0.1 (/playground)
error: only foreign or `unsafe extern "C"` functions may be C-variadic
 --> src/lib.rs:3:42
  |
3 | async fn multiple_named_lifetimes(_: u8, ...) {}
  |                                          ^^^

error[E0700]: hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
 --> src/lib.rs:3:47
  |
3 | async fn multiple_named_lifetimes(_: u8, ...) {}
  | --------------------------------------------- ^^
  | |
  | opaque type defined here
  |
  = note: hidden type `{async fn body of multiple_named_lifetimes()}` captures lifetime `'_`

For more information about this error, try `rustc --explain E0700`.
error: could not compile `playground` (lib) due to 2 previous errors

The second error is a little weird, but maybe we can close this issue?

@traviscross
Copy link
Contributor

@rustbot labels +AsyncAwait-Triaged

As above, we discussed this in the async call. We were a bit unclear about whether this is still an issue or if something else was being reported here.

@rustbot rustbot added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Jun 6, 2024
@compiler-errors
Copy link
Member

The fact that the error is weird is the bug. Also, there's no clear justification why this code shouldn't compile.

@tmandry
Copy link
Member

tmandry commented Jun 6, 2024

Also, there's no clear justification why this code shouldn't compile.

As in we could have an extern "C" function defined in Rust which returns an opaque type? I.. guess that's possible, but it would be weird and not actually callable from C.

@compiler-errors
Copy link
Member

No, I mean the fact that there's a varargs argument is why this has a borrowck error. This leaks the implementation details of how we implement varargs.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 27, 2024

Also, there's no clear justification why this code shouldn't compile.

As in we could have an extern "C" function defined in Rust which returns an opaque type? I.. guess that's possible, but it would be weird and not actually callable from C.

I mean, we don't even lint on this program:

use std::ffi::{c_char, c_int};
extern "C" {
    fn printf(c: *const c_char, ...) -> c_int;
}

fn main() {
    let allocated_cstring = c", but it is still UB\n".to_owned();
    unsafe { printf(c"this kinda makes sense%s".as_ptr(), allocated_cstring) };
    
    let allocated_repr_rust = vec!["lol what"];
    unsafe { printf(c"but this doesn't: %s".as_ptr(), allocated_repr_rust) };
}

we don't seem to be too fussed about "makes sense" versus "technically possible" here. (though, this isn't a program that uses feature(c_variadic) to be clear, but to me it raises the question of "what is Rust's C variable argument support for?").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. F-c_variadic `#![feature(c_variadic)]` requires-nightly This issue requires a nightly compiler in some way. 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

6 participants