improve c-variadic errors by folkertdev · Pull Request #143546 · rust-lang/rust (original) (raw)

tracking issue: #44930

Make some errors more specific, and clean up the logic

joshtriplett is not on the review rotation at the moment.
They may take a while to respond.

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

Jul 6, 2025

Comment on lines 30 to 32

unsafe extern "C" fn trait_method(&self, mut ap: ...) -> i32 {
unsafe { ap.arg() }
}

Choose a reason for hiding this comment

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

This raises questions about how we should handle the codegen for its self parameter, dyn compatibility, and so on. I would prefer we not get into those for now and continue to reject this case, please.

Choose a reason for hiding this comment

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

So reject in traits if there is a self parameter of some kind? reject in traits in general?

Previously also just normal associated methods/functions (like in impl S { }) were rejected, that is unproblematic right?

Choose a reason for hiding this comment

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

I would prefer to continue to reject in all of the associated function cases for consistency and visit relaxation of this constraint in a deliberate fashion.

There have already been cases where people who work primarily with more frontend-oriented aspects of the compiler have misunderstood C's varargs as purely some syntactic quirk, and thus e.g. easily moved from function to function, instead of the nightmare it really is.

Choose a reason for hiding this comment

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

fair enough, I'll just add a custom error message then.

Choose a reason for hiding this comment

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

This now generates. At least we have tests now, none of this was exercised apparently.

error: associated functions cannot have a C-variadic argument
  --> $DIR/no-associated-function.rs:9:46
   |
LL |     unsafe extern "C" fn associated_function(mut ap: ...) -> i32 {
   |

Comment on lines 656 to 658

// Closures cannot be c-variadic (they can't even be `extern "C"`).
self.dcx().emit_err(errors::CVariadicClosure { span: variadic_param.span });
return;

Choose a reason for hiding this comment

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

This is currently unreachable in practice, the parser already rejects ... in closures. So, this could be a span_bug instead?

Choose a reason for hiding this comment

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

huh, span_bug is only defined in rustc_middle, so we can't use that here. panic! or unreachable! also don't occur in this crate.

Choose a reason for hiding this comment

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

The AST is weird on this point, yes.

I think ideally we'd want to reject this in the parser itself (the same way we reject #[cfg(false)] fn f(u32) {}). However, it seems all the syntax gating for c_variadic has been done post-expansion meaning that this has compiled on stable since Rust 1.35.0 (although it seems unlikely anyone is relying on it):

#[cfg(any())] // Equivalent to the more recent #[cfg(false)] unsafe extern "C" fn bar(_: u32, ...) {}

Given that this is essentially a stability hole, I think it would probably be ok to break it, but it would need a crater run and a lang FCP.

Sigh. I hate post-expansion syntax gating.

This comment has been minimized.

folkertdev changed the title~~improve c-variadic errors and reject plain ...~~ improve c-variadic errors

Jul 7, 2025

Choose a reason for hiding this comment

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

allright, let's handle _: ... separately then.

r? @workingjubilee now that this is no longer making any changes that are relevant for T-lang

This comment has been minimized.

Comment on lines 659 to 698

if let Some(coroutine_kind) = sig.header.coroutine_kind {
self.dcx().emit_err(errors::CoroutineAndCVariadic {
span: coroutine_kind.span().to(variadic_param.span),
coroutine_kind: coroutine_kind.as_str(),
coroutine_span: coroutine_kind.span(),
variadic_span: variadic_param.span,
});
}

Choose a reason for hiding this comment

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

I think this fixes #125431 by just making it illegal, which is neat.

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

bors added a commit that referenced this pull request

Sep 8, 2025

improve c-variadic error reporting

tracking issue: #44930

The parts of #143546 that don't require any particular knowledge about c-variadic functions.

This prepares the way for rejecting c-variadic functions that are also coroutines, safe functions, or associated functions.

re-implemented more cleanly in #146342

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

Sep 9, 2025

…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

disallows coroutines (e.g. async fn) from having a ... argument
disallows associated functions (both in traits and standard impl blocks) from having a ... argument
splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword

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

…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

disallows coroutines (e.g. async fn) from having a ... argument
disallows associated functions (both in traits and standard impl blocks) from having a ... argument
splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword

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

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

Sep 10, 2025

…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

disallows coroutines (e.g. async fn) from having a ... argument
disallows associated functions (both in traits and standard impl blocks) from having a ... argument
splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword

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

rust-timer added a commit that referenced this pull request

Sep 11, 2025

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

disallows coroutines (e.g. async fn) from having a ... argument
disallows associated functions (both in traits and standard impl blocks) from having a ... argument
splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword

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

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Sep 11, 2025

improve c-variadic error reporting

tracking issue: rust-lang/rust#44930

The parts of rust-lang/rust#143546 that don't require any particular knowledge about c-variadic functions.

This prepares the way for rejecting c-variadic functions that are also coroutines, safe functions, or associated functions.

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

Sep 22, 2025

improve c-variadic error reporting

tracking issue: rust-lang/rust#44930

The parts of rust-lang/rust#143546 that don't require any particular knowledge about c-variadic functions.

This prepares the way for rejecting c-variadic functions that are also coroutines, safe functions, or associated functions.

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

Nov 11, 2025

…mann

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

This PR makes ... arguments without a pattern in non-foreign functions (such as the argument in unsafe extern "C" fn f(...) {}) a future-compatibility warning; making this error would be consistent with how unsafe extern "C" fn f(u32) {} is handled. Allowing ... arguments without a pattern in non-foreign functions is a source of confusion for programmers coming from C, where the ... parameter is never named and instead calling va_start is required; disallowing ... arguments without a pattern also improves the overall consistency of the Rust language by matching the treatment of other arguments without patterns. ... arguments without a pattern in extern blocks (such as unsafe extern "C" { fn f(...); }) continue to compile without warnings after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here rust-lang#143546 (comment).

Tracking issue: rust-lang#44930 cc @folkertdev @workingjubilee r? @joshtriplett

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

Nov 11, 2025

…mann

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here rust-lang#143546 (comment).

Tracking issue: rust-lang#44930 cc @folkertdev @workingjubilee r? @joshtriplett

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

Nov 11, 2025

…mann

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here rust-lang#143546 (comment).

Tracking issue: rust-lang#44930 cc @folkertdev @workingjubilee r? @joshtriplett

bors added a commit that referenced this pull request

Nov 11, 2025

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here #143546 (comment).

Tracking issue: #44930 cc @folkertdev @workingjubilee r? @joshtriplett

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

Nov 11, 2025

…mann

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here rust-lang#143546 (comment).

Tracking issue: rust-lang#44930 cc @folkertdev @workingjubilee r? @joshtriplett

rust-timer added a commit that referenced this pull request

Nov 11, 2025

Rollup merge of #143619 - beetrees:varargs-named, r=jdonszelmann

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here #143546 (comment).

Tracking issue: #44930 cc @folkertdev @workingjubilee r? @joshtriplett

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Nov 12, 2025

c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks

As all the syntax gating for c_variadic has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:

#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}

Since this is more or less a stability hole and a Crater run shows only the binrw crate is using this, I think it would be ok to break this. This will require a lang FCP.

The idea of rejecting ... pre-expansion was first raised here rust-lang/rust#143546 (comment).

Tracking issue: rust-lang/rust#44930 cc @folkertdev @workingjubilee r? @joshtriplett