Improve C-variadic error messages: part 2 by folkertdev · Pull Request #146342 · rust-lang/rust (original) (raw)

@folkertdev

tracking issue: #44930

a reimplementation of #143546 that builds on #146165.

This PR

C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird.

For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message.

Made to be reviewed commit-by-commit.

cc @workingjubilee
r? compiler

@folkertdev

@folkertdev

there is no reason this should not work, really, we're just cutting some scope for now

@folkertdev

@folkertdev

@folkertdev

@folkertdev

@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

Sep 8, 2025

workingjubilee

@workingjubilee

Thank you, aside from the nitpicking this is a pretty good PR.

@folkertdev

@folkertdev

workingjubilee
#[suggestion(
ast_passes_suggestion,
applicability = "maybe-incorrect",

Choose a reason for hiding this comment

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

This seems machine-applicable? It is a direct fixup. The only other working solution is to remove the varargs.

It may lead to the code not compiling because the callers aren't in unsafe {}, but that's fine. Intentional, even.

Unless the main justification is to prevent the risk of such calls silently appearing in an unsafe {}? But I'm not sure how much that is a risk, as the code already doesn't compile. I suppose after refactoring? Someone could be refactoring a fixed args function to variable arguments, and it's used in unsafe {} incidentally, then they get a fixup like this without looking... hmm, yeah, I guess that's a risk. I suppose I talked myself into justifying the current state!

Choose a reason for hiding this comment

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

Also, adding unsafe to a function declaration should likely be accompanied by the addition of a safety doc comment, and having to manually fix the error makes it more likely that you will do that too

Choose a reason for hiding this comment

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

That's the sort of linting that clippy is good at enforcing (and despite my occasional grumbling in clippy's direction, lints on unsafety are one of the clear winners for clippy usage, IMO), so I'm less concerned at that level.

@workingjubilee

Yeah, that works!

Thinking: We're not doing any manual manipulation of spans still aside from picking edges, so this shouldn't run afoul of the classic problems with spans, as far as I am aware. I'm expecting issues, if any, to appear at the integration edges with tooling that works on suggestions, and not in rustc. So this code is fine in rustc, I believe.

Thanks!

@bors r+ rollup

@bors

📌 Commit 9196844 has been approved by workingjubilee

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-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 9, 2025

workingjubilee

async unsafe extern "C" fn multiple_named_lifetimes<'a, 'b>(_: u8, ...) {}
//~^ ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
//~^ ERROR functions cannot be both `async` and C-variadic
//~| ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds

Choose a reason for hiding this comment

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

This is an interesting quirk: varargs appear to interact with lifetimes in such a way that, plus async, this creates a bunch of hidden captured lifetimes? I think this might depend on varargs and not require async but I think all the other ways to generate this interaction with hidden types are unstable anyways.

Choose a reason for hiding this comment

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

Interesting, I believe that is what is actually happening here too #132142, where we actually run into an ICE. Anyhow the fix there is to disallow function definitions with that ABI, which should fix it I think?

But it does look like some part of the type checker doesn't properly account for c-variadics. It also interacts specifically with the async machinery, because this desugared version does not emit errors.

unsafe extern "C" fn multiple_named_lifetimes2<'a, 'b>(_: u8, ...) -> impl std::future::Future<Output = ()> { async {} }

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Yeah the desugared version doesn't have the same lifetime-capturing rules, I believe. It seems to Need to be an opaque type.

Choose a reason for hiding this comment

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

"but Jubilee, the opaque type is right there, impl Something, you even tried to do the same with a different trait!"

...uhm, hmm.

Right, my brain started churning over "something with use<'???> maybe?" but then I couldn't find it and eventually concluded it was the compiler interacting with something we couldn't name as a programmer.

Choose a reason for hiding this comment

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

Something like that, because this also just works

unsafe extern "C" fn multiple_named_lifetimes2<'a, 'b>(_: u8, ...) -> impl std::future::Future<Output = ()> + 'a + 'b { async {} }

so I'm guessing the desugared version has the async {} itself capture the lifetime?

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

Sep 9, 2025

@GuillaumeGomez

…3, r=workingjubilee

Improve C-variadic error messages: part 2

tracking issue: rust-lang#44930

a reimplementation of rust-lang#143546 that builds on rust-lang#146165.

This PR

C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird.

For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message.

Made to be reviewed commit-by-commit.

cc @workingjubilee r? compiler

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

Sep 9, 2025

@GuillaumeGomez

…3, r=workingjubilee

Improve C-variadic error messages: part 2

tracking issue: rust-lang#44930

a reimplementation of rust-lang#143546 that builds on rust-lang#146165.

This PR

C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird.

For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message.

Made to be reviewed commit-by-commit.

cc @workingjubilee r? compiler

bors added a commit that referenced this pull request

Sep 10, 2025

@bors

Rollup of 6 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit that referenced this pull request

Sep 10, 2025

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Sep 11, 2025

@rust-timer

Rollup merge of #146342 - folkertdev:c-variadic-errors-take-3, r=workingjubilee

Improve C-variadic error messages: part 2

tracking issue: #44930

a reimplementation of #143546 that builds on #146165.

This PR

C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird.

For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message.

Made to be reviewed commit-by-commit.

cc @workingjubilee r? compiler

fmease added a commit to fmease/rust that referenced this pull request

Sep 18, 2025

@fmease

…hods, r=workingjubilee

c-variadic: allow c-variadic inherent and trait methods

tracking issue: rust-lang#44930

Continuing the work of rust-lang#146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible.

There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a dyn object are cast to a function pointer, a ReifyShim is created. If that shim is c-variadic, it would need to forward the C variable argument list.

That does appear to work, because the va_list is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation.

Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with dyn objects at all? We can revisit this limitation if a need arises.

r? @workingjubilee

bors added a commit that referenced this pull request

Sep 18, 2025

@bors

…kingjubilee

c-variadic: allow c-variadic inherent and trait methods

tracking issue: #44930

Continuing the work of #146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible.

There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a dyn object are cast to a function pointer, a ReifyShim is created. If that shim is c-variadic, it would need to forward the C variable argument list.

That does appear to work, because the va_list is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation.

Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with dyn objects at all? We can revisit this limitation if a need arises.

r? @workingjubilee

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 18, 2025

@matthiaskrgr

…hods, r=workingjubilee

c-variadic: allow c-variadic inherent and trait methods

tracking issue: rust-lang#44930

Continuing the work of rust-lang#146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible.

There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a dyn object are cast to a function pointer, a ReifyShim is created. If that shim is c-variadic, it would need to forward the C variable argument list.

That does appear to work, because the va_list is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation.

Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with dyn objects at all? We can revisit this limitation if a need arises.

r? @workingjubilee

rust-timer added a commit that referenced this pull request

Sep 18, 2025

@rust-timer

Rollup merge of #146434 - folkertdev:c-variadic-inherent-methods, r=workingjubilee

c-variadic: allow c-variadic inherent and trait methods

tracking issue: #44930

Continuing the work of #146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible.

There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a dyn object are cast to a function pointer, a ReifyShim is created. If that shim is c-variadic, it would need to forward the C variable argument list.

That does appear to work, because the va_list is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation.

Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with dyn objects at all? We can revisit this limitation if a need arises.

r? @workingjubilee