fix(macro): nullable parameters #538

Author: kakserpom

crates/macros/src/function.rs

@@ -493,7 +493,7 @@ impl<'a> Args<'a> {
 
                     // If the variable is `&[&Zval]` treat it as the variadic argument.
                     let default = defaults.remove(ident);
-                    let nullable = type_is_nullable(ty.as_ref(), default.is_some())?;
+                    let nullable = type_is_nullable(ty.as_ref())?;
                     let (variadic, as_ref, ty) = Self::parse_typed(ty);
                     result.typed.push(TypedArg {
                         name: ident,
@@ -568,18 +568,20 @@ impl<'a> Args<'a> {
     /// # Parameters
     ///
     /// * `optional` - The first optional argument. If [`None`], the optional
-    ///   arguments will be from the first nullable argument after the last
-    ///   non-nullable argument to the end of the arguments.
+    ///   arguments will be from the first optional argument (nullable or has
+    ///   default) after the last required argument to the end of the arguments.
     pub fn split_args(&self, optional: Option<&Ident>) -> (&[TypedArg<'a>], &[TypedArg<'a>]) {
         let mut mid = None;
         for (i, arg) in self.typed.iter().enumerate() {
+            // An argument is optional if it's nullable (Option<T>) or has a default value.
+            let is_optional = arg.nullable || arg.default.is_some();
             if let Some(optional) = optional {
                 if optional == arg.name {
                     mid.replace(i);
                 }
-            } else if mid.is_none() && arg.nullable {
+            } else if mid.is_none() && is_optional {
                 mid.replace(i);
-            } else if !arg.nullable {
+            } else if !is_optional {
                 mid.take();
             }
         }
@@ -690,21 +692,22 @@ impl TypedArg<'_> {
     }
 }
 
-/// Returns true of the given type is nullable in PHP.
+/// Returns true if the given type is nullable in PHP (i.e., it's an `Option<T>`).
+///
+/// Note: Having a default value does NOT make a type nullable. A parameter with
+/// a default value is optional (can be omitted), but passing `null` explicitly
+/// should still be rejected unless the type is `Option<T>`.
 // TODO(david): Eventually move to compile-time constants for this (similar to
 // FromZval::NULLABLE).
-pub fn type_is_nullable(ty: &Type, has_default: bool) -> Result<bool> {
+pub fn type_is_nullable(ty: &Type) -> Result<bool> {
     Ok(match ty {
-        syn::Type::Path(path) => {
-            has_default
-                || path
-                    .path
-                    .segments
-                    .iter()
-                    .next_back()
-                    .is_some_and(|seg| seg.ident == "Option")
-        }
-        syn::Type::Reference(_) => false, /* Reference cannot be nullable unless */
+        Type::Path(path) => path
+            .path
+            .segments
+            .iter()
+            .next_back()
+            .is_some_and(|seg| seg.ident == "Option"),
+        Type::Reference(_) => false, /* Reference cannot be nullable unless */
         // wrapped in `Option` (in that case it'd be a Path).
         _ => bail!(ty => "Unsupported argument type."),
     })

tests/src/integration/defaults/defaults.php

@@ -7,3 +7,16 @@
 assert(test_defaults_multiple_option_arguments() === 'Default');
 assert(test_defaults_multiple_option_arguments(a: 'a') === 'a');
 assert(test_defaults_multiple_option_arguments(b: 'b') === 'b');
+
+// Test that passing null to a non-nullable parameter with a default value throws TypeError
+// (fixes: https://github.com/extphprs/ext-php-rs/issues/538)
+$threw = false;
+try {
+    test_defaults_integer(null);
+} catch (TypeError $e) {
+    $threw = true;
+}
+assert($threw, 'Expected TypeError when passing null to non-nullable parameter with default');
+
+// But passing null to a nullable parameter should still work
+assert(test_defaults_nullable_string(null) === null);