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`

Author: matthiaskrgr

compiler/rustc_ast_passes/messages.ftl

@@ -64,8 +64,6 @@ ast_passes_body_in_extern = incorrect `{$kind}` inside `extern` block
 
 ast_passes_bound_in_context = bounds on `type`s in {$ctx} have no effect
 
-ast_passes_c_variadic_associated_function = associated functions cannot have a C variable argument list
-
 ast_passes_c_variadic_bad_extern = `...` is not supported for `extern "{$abi}"` functions
     .label = `extern "{$abi}"` because of this
     .help = only `extern "C"` and `extern "C-unwind"` functions may have a C variable argument list

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -696,7 +696,7 @@ impl<'a> AstValidator<'a> {
 
         match fn_ctxt {
             FnCtxt::Foreign => return,
-            FnCtxt::Free => match sig.header.ext {
+            FnCtxt::Free | FnCtxt::Assoc(_) => match sig.header.ext {
                 Extern::Implicit(_) => {
                     if !matches!(sig.header.safety, Safety::Unsafe(_)) {
                         self.dcx().emit_err(errors::CVariadicMustBeUnsafe {
@@ -726,11 +726,6 @@ impl<'a> AstValidator<'a> {
                     self.dcx().emit_err(err);
                 }
             },
-            FnCtxt::Assoc(_) => {
-                // For now, C variable argument lists are unsupported in associated functions.
-                let err = errors::CVariadicAssociatedFunction { span: variadic_param.span };
-                self.dcx().emit_err(err);
-            }
         }
     }
 

compiler/rustc_ast_passes/src/errors.rs

@@ -318,13 +318,6 @@ pub(crate) struct ExternItemAscii {
     pub block: Span,
 }
 
-#[derive(Diagnostic)]
-#[diag(ast_passes_c_variadic_associated_function)]
-pub(crate) struct CVariadicAssociatedFunction {
-    #[primary_span]
-    pub span: Span,
-}
-
 #[derive(Diagnostic)]
 #[diag(ast_passes_c_variadic_no_extern)]
 #[help]

compiler/rustc_middle/src/traits/mod.rs

@@ -823,6 +823,9 @@ impl DynCompatibilityViolation {
             DynCompatibilityViolation::Method(name, MethodViolationCode::AsyncFn, _) => {
                 format!("method `{name}` is `async`").into()
             }
+            DynCompatibilityViolation::Method(name, MethodViolationCode::CVariadic, _) => {
+                format!("method `{name}` is C-variadic").into()
+            }
             DynCompatibilityViolation::Method(
                 name,
                 MethodViolationCode::WhereClauseReferencesSelf,
@@ -977,6 +980,9 @@ pub enum MethodViolationCode {
     /// e.g., `fn foo<A>()`
     Generic,
 
+    /// e.g., `fn (mut ap: ...)`
+    CVariadic,
+
     /// the method's receiver (`self` argument) can't be dispatched on
     UndispatchableReceiver(Option<Span>),
 }

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

@@ -426,6 +426,9 @@ fn virtual_call_violations_for_method<'tcx>(
     if let Some(code) = contains_illegal_impl_trait_in_trait(tcx, method.def_id, sig.output()) {
         errors.push(code);
     }
+    if sig.skip_binder().c_variadic {
+        errors.push(MethodViolationCode::CVariadic);
+    }
 
     // We can't monomorphize things like `fn foo<A>(...)`.
     let own_counts = tcx.generics_of(method.def_id).own_counts();

tests/ui/c-variadic/inherent-method.rs

@@ -0,0 +1,45 @@
+//@ run-pass
+#![feature(c_variadic)]
+
+#[repr(transparent)]
+struct S(i32);
+
+impl S {
+    unsafe extern "C" fn associated_function(mut ap: ...) -> i32 {
+        unsafe { ap.arg() }
+    }
+
+    unsafe extern "C" fn method_owned(self, mut ap: ...) -> i32 {
+        self.0 + unsafe { ap.arg::<i32>() }
+    }
+
+    unsafe extern "C" fn method_ref(&self, mut ap: ...) -> i32 {
+        self.0 + unsafe { ap.arg::<i32>() }
+    }
+
+    unsafe extern "C" fn method_mut(&mut self, mut ap: ...) -> i32 {
+        self.0 + unsafe { ap.arg::<i32>() }
+    }
+
+    unsafe extern "C" fn fat_pointer(self: Box<Self>, mut ap: ...) -> i32 {
+        self.0 + unsafe { ap.arg::<i32>() }
+    }
+}
+
+fn main() {
+    unsafe {
+        assert_eq!(S::associated_function(32), 32);
+        assert_eq!(S(100).method_owned(32), 132);
+        assert_eq!(S(100).method_ref(32), 132);
+        assert_eq!(S(100).method_mut(32), 132);
+        assert_eq!(S::fat_pointer(Box::new(S(100)), 32), 132);
+
+        type Method<T> = unsafe extern "C" fn(T, ...) -> i32;
+
+        assert_eq!((S::associated_function as unsafe extern "C" fn(...) -> i32)(32), 32);
+        assert_eq!((S::method_owned as Method<_>)(S(100), 32), 132);
+        assert_eq!((S::method_ref as Method<_>)(&S(100), 32), 132);
+        assert_eq!((S::method_mut as Method<_>)(&mut S(100), 32), 132);
+        assert_eq!((S::fat_pointer as Method<_>)(Box::new(S(100)), 32), 132);
+    }
+}

tests/ui/c-variadic/not-async.rs

@@ -2,6 +2,14 @@
 #![feature(c_variadic)]
 #![crate_type = "lib"]
 
-async unsafe extern "C" fn cannot_be_async(x: isize, ...) {}
+async unsafe extern "C" fn fn_cannot_be_async(x: isize, ...) {}
 //~^ ERROR functions cannot be both `async` and C-variadic
 //~| ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
+
+struct S;
+
+impl S {
+    async unsafe extern "C" fn method_cannot_be_async(x: isize, ...) {}
+    //~^ ERROR functions cannot be both `async` and C-variadic
+    //~| ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
+}

tests/ui/c-variadic/not-async.stderr

@@ -1,19 +1,35 @@
 error: functions cannot be both `async` and C-variadic
   --> $DIR/not-async.rs:5:1
    |
-LL | async unsafe extern "C" fn cannot_be_async(x: isize, ...) {}
-   | ^^^^^ `async` because of this                        ^^^ C-variadic because of this
+LL | async unsafe extern "C" fn fn_cannot_be_async(x: isize, ...) {}
+   | ^^^^^ `async` because of this                           ^^^ C-variadic because of this
+
+error: functions cannot be both `async` and C-variadic
+  --> $DIR/not-async.rs:12:5
+   |
+LL |     async unsafe extern "C" fn method_cannot_be_async(x: isize, ...) {}
+   |     ^^^^^ `async` because of this                               ^^^ C-variadic because of this
 
 error[E0700]: hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
-  --> $DIR/not-async.rs:5:59
+  --> $DIR/not-async.rs:5:62
    |
-LL | async unsafe extern "C" fn cannot_be_async(x: isize, ...) {}
-   | --------------------------------------------------------- ^^
+LL | async unsafe extern "C" fn fn_cannot_be_async(x: isize, ...) {}
+   | ------------------------------------------------------------ ^^
    | |
    | opaque type defined here
    |
-   = note: hidden type `{async fn body of cannot_be_async()}` captures lifetime `'_`
+   = note: hidden type `{async fn body of fn_cannot_be_async()}` captures lifetime `'_`
+
+error[E0700]: hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
+  --> $DIR/not-async.rs:12:70
+   |
+LL |     async unsafe extern "C" fn method_cannot_be_async(x: isize, ...) {}
+   |     ---------------------------------------------------------------- ^^
+   |     |
+   |     opaque type defined here
+   |
+   = note: hidden type `{async fn body of S::method_cannot_be_async()}` captures lifetime `'_`
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
 
 For more information about this error, try `rustc --explain E0700`.

tests/ui/c-variadic/not-dyn-compatible.rs

@@ -0,0 +1,35 @@
+// Traits where a method is c-variadic are not dyn compatible.
+//
+// Creating a function pointer from a method on an `&dyn T` value creates a ReifyShim.
+// This shim cannot reliably forward C-variadic arguments. Thus the trait as a whole
+// is dyn-incompatible to prevent invalid shims from being created.
+#![feature(c_variadic)]
+
+#[repr(transparent)]
+struct Struct(u64);
+
+trait Trait {
+    fn get(&self) -> u64;
+
+    unsafe extern "C" fn dyn_method_ref(&self, mut ap: ...) -> u64 {
+        self.get() + unsafe { ap.arg::<u64>() }
+    }
+}
+
+impl Trait for Struct {
+    fn get(&self) -> u64 {
+        self.0
+    }
+}
+
+fn main() {
+    unsafe {
+        let dyn_object: &dyn Trait = &Struct(64);
+        //~^ ERROR the trait `Trait` is not dyn compatible
+        assert_eq!(dyn_object.dyn_method_ref(100), 164);
+        assert_eq!(
+            (Trait::dyn_method_ref as unsafe extern "C" fn(_, ...) -> u64)(dyn_object, 100),
+            164
+        );
+    }
+}

tests/ui/c-variadic/not-dyn-compatible.stderr

@@ -0,0 +1,21 @@
+error[E0038]: the trait `Trait` is not dyn compatible
+  --> $DIR/not-dyn-compatible.rs:27:30
+   |
+LL |         let dyn_object: &dyn Trait = &Struct(64);
+   |                              ^^^^^ `Trait` is not dyn compatible
+   |
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility>
+  --> $DIR/not-dyn-compatible.rs:14:26
+   |
+LL | trait Trait {
+   |       ----- this trait is not dyn compatible...
+...
+LL |     unsafe extern "C" fn dyn_method_ref(&self, mut ap: ...) -> u64 {
+   |                          ^^^^^^^^^^^^^^ ...because method `dyn_method_ref` is C-variadic
+   = help: consider moving `dyn_method_ref` to another trait
+   = help: only type `Struct` implements `Trait`; consider using it directly instead.
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0038`.