fix(macro): address PR review feedback extphprs#616

kakserpom · claude · kakserpom · commit 9fde1b8634f5 · 2025-12-22T02:55:51.000+07:00
- Remove 'resource' and 'numeric' from PHP_TYPE_KEYWORDS as they are allowed for class names per PHP source code - Add EnumCase validation to prevent reserved keywords like 'true'/'false' from being used as enum case names - Replace assert! with syn::Error for proper macro error handling with span information for better error messages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
diff --git a/crates/macros/src/class.rs b/crates/macros/src/class.rs
@@ -47,7 +47,7 @@ pub fn parser(mut input: ItemStruct) -> Result<TokenStream> {
     let name = attr
         .rename
         .rename(ident_to_php_name(ident), RenameRule::Pascal);
-    validate_php_name(&name, PhpNameContext::Class);
+    validate_php_name(&name, PhpNameContext::Class, ident.span())?;
     let docs = get_docs(&attr.attrs)?;
     input.attrs.retain(|attr| !attr.path().is_ident("php"));
 
@@ -97,7 +97,17 @@ fn parse_fields<'a>(fields: impl Iterator<Item = &'a mut syn::Field>) -> Result<
             let docs = get_docs(&attr.attrs)?;
             field.attrs.retain(|attr| !attr.path().is_ident("php"));
 
-            result.push(Property { ident, attr, docs });
+            let name = attr
+                .rename
+                .rename(ident_to_php_name(ident), RenameRule::Camel);
+            validate_php_name(&name, PhpNameContext::Property, ident.span())?;
+
+            result.push(Property {
+                ident,
+                name,
+                attr,
+                docs,
+            });
         }
     }
 
@@ -107,21 +117,11 @@ fn parse_fields<'a>(fields: impl Iterator<Item = &'a mut syn::Field>) -> Result<
 #[derive(Debug)]
 struct Property<'a> {
     pub ident: &'a syn::Ident,
+    pub name: String,
     pub attr: PropAttributes,
     pub docs: Vec<String>,
 }
 
-impl Property<'_> {
-    pub fn name(&self) -> String {
-        let name = self
-            .attr
-            .rename
-            .rename(ident_to_php_name(self.ident), RenameRule::Camel);
-        validate_php_name(&name, PhpNameContext::Property);
-        name
-    }
-}
-
 /// Generates an implementation of `RegisteredClass` for struct `ident`.
 #[allow(clippy::too_many_arguments)]
 fn generate_registered_class_impl(
@@ -137,7 +137,7 @@ fn generate_registered_class_impl(
     let modifier = modifier.option_tokens();
 
     let fields = fields.iter().map(|prop| {
-        let name = prop.name();
+        let name = &prop.name;
         let ident = prop.ident;
         let flags = prop
             .attr
diff --git a/crates/macros/src/constant.rs b/crates/macros/src/constant.rs
@@ -26,7 +26,7 @@ pub fn parser(mut item: ItemConst) -> Result<TokenStream> {
     let name = attr
         .rename
         .rename(ident_to_php_name(&item.ident), RenameRule::ScreamingSnake);
-    validate_php_name(&name, PhpNameContext::Constant);
+    validate_php_name(&name, PhpNameContext::Constant, item.ident.span())?;
     let name_ident = format_ident!("{INTERNAL_CONST_NAME_PREFIX}{}", item.ident);
 
     let docs = get_docs(&attr.attrs)?;
diff --git a/crates/macros/src/enum_.rs b/crates/macros/src/enum_.rs
@@ -83,12 +83,15 @@ pub fn parser(mut input: ItemEnum) -> Result<TokenStream> {
             bail!(variant => "Discriminant must be specified for all enum cases, found: {:?}", variant);
         }
 
+        let case_name = variant_attr.rename.rename(
+            ident_to_php_name(&variant.ident),
+            php_attr.rename_cases.unwrap_or(RenameRule::Pascal),
+        );
+        validate_php_name(&case_name, PhpNameContext::EnumCase, variant.ident.span())?;
+
         cases.push(EnumCase {
             ident: variant.ident.clone(),
-            name: variant_attr.rename.rename(
-                ident_to_php_name(&variant.ident),
-                php_attr.rename_cases.unwrap_or(RenameRule::Pascal),
-            ),
+            name: case_name,
             attrs: variant_attr,
             discriminant,
             docs,
@@ -110,7 +113,7 @@ pub fn parser(mut input: ItemEnum) -> Result<TokenStream> {
         cases,
         None, // TODO: Implement flags support
         discriminant_type,
-    );
+    )?;
 
     Ok(quote! {
         #[allow(dead_code)]
@@ -138,20 +141,20 @@ impl<'a> Enum<'a> {
         cases: Vec<EnumCase>,
         flags: Option<String>,
         discriminant_type: DiscriminantType,
-    ) -> Self {
+    ) -> Result<Self> {
         let name = attrs
             .rename
             .rename(ident_to_php_name(ident), RenameRule::Pascal);
-        validate_php_name(&name, PhpNameContext::Enum);
+        validate_php_name(&name, PhpNameContext::Enum, ident.span())?;
 
-        Self {
+        Ok(Self {
             ident,
             name,
             discriminant_type,
             docs,
             cases,
             flags,
-        }
+        })
     }
 
     fn registered_class(&self) -> TokenStream {
diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs
@@ -49,7 +49,7 @@ pub fn parser(mut input: ItemFn) -> Result<TokenStream> {
     let func_name = php_attr
         .rename
         .rename(ident_to_php_name(&input.sig.ident), RenameRule::Snake);
-    validate_php_name(&func_name, PhpNameContext::Function);
+    validate_php_name(&func_name, PhpNameContext::Function, input.sig.ident.span())?;
     let func = Function::new(&input.sig, func_name, args, php_attr.optional, docs);
     let function_impl = func.php_function_impl();
 
diff --git a/crates/macros/src/impl_.rs b/crates/macros/src/impl_.rs
@@ -190,7 +190,7 @@ impl<'a> ParsedImpl<'a> {
                     let name = attr
                         .rename
                         .rename(ident_to_php_name(&c.ident), self.change_constant_case);
-                    validate_php_name(&name, PhpNameContext::Constant);
+                    validate_php_name(&name, PhpNameContext::Constant, c.ident.span())?;
                     let docs = get_docs(&attr.attrs)?;
                     c.attrs.retain(|attr| !attr.path().is_ident("php"));
 
@@ -206,7 +206,7 @@ impl<'a> ParsedImpl<'a> {
                         ident_to_php_name(&method.sig.ident),
                         self.change_method_case,
                     );
-                    validate_php_name(&name, PhpNameContext::Method);
+                    validate_php_name(&name, PhpNameContext::Method, method.sig.ident.span())?;
                     let docs = get_docs(&attr.attrs)?;
                     method.attrs.retain(|attr| !attr.path().is_ident("php"));
 
diff --git a/crates/macros/src/interface.rs b/crates/macros/src/interface.rs
@@ -201,7 +201,7 @@ impl<'a> Parse<'a, InterfaceData<'a>> for ItemTrait {
         let name = attrs
             .rename
             .rename(ident_to_php_name(ident), RenameRule::Pascal);
-        validate_php_name(&name, PhpNameContext::Interface);
+        validate_php_name(&name, PhpNameContext::Interface, ident.span())?;
         let docs = get_docs(&attrs.attrs)?;
         self.attrs.clean_php();
         let interface_name = format_ident!("{INTERNAL_INTERFACE_NAME_PREFIX}{ident}");
@@ -286,7 +286,11 @@ fn parse_trait_item_fn(
         ident_to_php_name(&fn_item.sig.ident),
         change_case.unwrap_or(RenameRule::Camel),
     );
-    validate_php_name(&method_name, PhpNameContext::Method);
+    validate_php_name(
+        &method_name,
+        PhpNameContext::Method,
+        fn_item.sig.ident.span(),
+    )?;
     let f = Function::new(&fn_item.sig, method_name, args, php_attr.optional, docs);
 
     if php_attr.constructor.is_present() {
@@ -340,7 +344,7 @@ fn parse_trait_item_const(
         ident_to_php_name(&const_item.ident),
         change_case.unwrap_or(RenameRule::ScreamingSnake),
     );
-    validate_php_name(&name, PhpNameContext::Constant);
+    validate_php_name(&name, PhpNameContext::Constant, const_item.ident.span())?;
     let docs = get_docs(&attr.attrs)?;
     const_item.attrs.clean_php();
 
diff --git a/crates/macros/src/parsing.rs b/crates/macros/src/parsing.rs
@@ -117,9 +117,12 @@ const PHP_RESERVED_KEYWORDS: &[&str] = &[
 
 /// Type keywords that are reserved for class/interface/enum names but CAN be
 /// used as method, function, constant, or property names in PHP.
+///
+/// Note: `resource` and `numeric` are NOT in this list because PHP allows them
+/// as class names. See: <https://github.com/php/php-src/blob/master/Zend/zend_compile.c>
 const PHP_TYPE_KEYWORDS: &[&str] = &[
-    "bool", "false", "float", "int", "iterable", "mixed", "never", "null", "numeric", "object",
-    "resource", "string", "true", "void",
+    "bool", "false", "float", "int", "iterable", "mixed", "never", "null", "object", "string",
+    "true", "void",
 ];
 
 /// The context in which a PHP name is being used.
@@ -131,6 +134,8 @@ pub enum PhpNameContext {
     Interface,
     /// An enum name (e.g., `enum Foo {}`)
     Enum,
+    /// An enum case name (e.g., `case Foo;`)
+    EnumCase,
     /// A function name (e.g., `function foo() {}`)
     Function,
     /// A method name (e.g., `public function foo() {}`)
@@ -147,6 +152,7 @@ impl PhpNameContext {
             Self::Class => "class",
             Self::Interface => "interface",
             Self::Enum => "enum",
+            Self::EnumCase => "enum case",
             Self::Function => "function",
             Self::Method => "method",
             Self::Constant => "constant",
@@ -177,36 +183,47 @@ pub fn is_php_reserved_keyword(name: &str) -> bool {
 /// Validates that a PHP name is not a reserved keyword.
 ///
 /// The validation is context-aware:
-/// - For class, interface, and enum names: both reserved keywords AND type keywords are checked
+/// - For class, interface, enum, and enum case names: both reserved keywords AND type keywords are checked
 /// - For method, function, constant, and property names: only reserved keywords are checked
 ///   (type keywords like `void`, `bool`, etc. are allowed)
 ///
-/// # Panics
+/// # Errors
 ///
-/// Panics with a descriptive error message if the name is a reserved keyword in the given context.
-pub fn validate_php_name(name: &str, context: PhpNameContext) {
+/// Returns a `syn::Error` if the name is a reserved keyword in the given context.
+pub fn validate_php_name(
+    name: &str,
+    context: PhpNameContext,
+    span: proc_macro2::Span,
+) -> Result<(), syn::Error> {
     let is_reserved = is_php_reserved_keyword(name);
     let is_type = is_php_type_keyword(name);
 
-    // Type keywords are only forbidden for class/interface/enum names
+    // Type keywords are forbidden for class/interface/enum/enum case names
     let is_forbidden = match context {
-        PhpNameContext::Class | PhpNameContext::Interface | PhpNameContext::Enum => {
-            is_reserved || is_type
-        }
+        PhpNameContext::Class
+        | PhpNameContext::Interface
+        | PhpNameContext::Enum
+        | PhpNameContext::EnumCase => is_reserved || is_type,
         PhpNameContext::Function
         | PhpNameContext::Method
         | PhpNameContext::Constant
         | PhpNameContext::Property => is_reserved,
     };
 
-    assert!(
-        !is_forbidden,
-        "Cannot use '{}' as a PHP {} name: '{}' is a reserved keyword in PHP. \
-         Consider using a different name or the #[php(name = \"...\")] attribute to specify an alternative PHP name.",
-        name,
-        context.description(),
-        name
-    );
+    if is_forbidden {
+        return Err(syn::Error::new(
+            span,
+            format!(
+                "cannot use '{}' as a PHP {} name: '{}' is a reserved keyword in PHP. \
+                 Consider using a different name or the #[php(name = \"...\")] attribute to specify an alternative PHP name.",
+                name,
+                context.description(),
+                name
+            ),
+        ));
+    }
+
+    Ok(())
 }
 
 const MAGIC_METHOD: [&str; 17] = [
@@ -588,41 +605,80 @@ mod tests {
     }
 
     #[test]
-    #[should_panic(expected = "is a reserved keyword in PHP")]
     fn test_validate_php_name_rejects_reserved_keyword() {
         use super::{PhpNameContext, validate_php_name};
-        validate_php_name("class", PhpNameContext::Class);
+        use proc_macro2::Span;
+
+        let result = validate_php_name("class", PhpNameContext::Class, Span::call_site());
+        assert!(result.is_err());
+        let err = result.unwrap_err();
+        assert!(err.to_string().contains("is a reserved keyword in PHP"));
     }
 
     #[test]
-    #[should_panic(expected = "is a reserved keyword in PHP")]
     fn test_validate_php_name_rejects_type_keyword_for_class() {
         use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
         // Type keywords like 'void' cannot be used as class names
-        validate_php_name("void", PhpNameContext::Class);
+        let result = validate_php_name("void", PhpNameContext::Class, Span::call_site());
+        assert!(result.is_err());
+        let err = result.unwrap_err();
+        assert!(err.to_string().contains("is a reserved keyword in PHP"));
+    }
+
+    #[test]
+    fn test_validate_php_name_rejects_type_keyword_for_enum_case() {
+        use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
+        // Type keywords like 'true' cannot be used as enum case names
+        let result = validate_php_name("true", PhpNameContext::EnumCase, Span::call_site());
+        assert!(result.is_err());
+        let err = result.unwrap_err();
+        assert!(err.to_string().contains("is a reserved keyword in PHP"));
+
+        let result = validate_php_name("false", PhpNameContext::EnumCase, Span::call_site());
+        assert!(result.is_err());
     }
 
     #[test]
     fn test_validate_php_name_allows_type_keyword_for_method() {
         use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
         // Type keywords like 'void' CAN be used as method names in PHP
-        validate_php_name("void", PhpNameContext::Method);
-        validate_php_name("true", PhpNameContext::Method);
-        validate_php_name("bool", PhpNameContext::Method);
-        validate_php_name("int", PhpNameContext::Method);
+        validate_php_name("void", PhpNameContext::Method, Span::call_site()).unwrap();
+        validate_php_name("true", PhpNameContext::Method, Span::call_site()).unwrap();
+        validate_php_name("bool", PhpNameContext::Method, Span::call_site()).unwrap();
+        validate_php_name("int", PhpNameContext::Method, Span::call_site()).unwrap();
     }
 
     #[test]
     fn test_validate_php_name_allows_type_keyword_for_function() {
         use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
         // Type keywords CAN be used as function names in PHP
-        validate_php_name("void", PhpNameContext::Function);
+        validate_php_name("void", PhpNameContext::Function, Span::call_site()).unwrap();
     }
 
     #[test]
     fn test_validate_php_name_allows_type_keyword_for_constant() {
         use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
         // Type keywords CAN be used as constant names in PHP
-        validate_php_name("void", PhpNameContext::Constant);
+        validate_php_name("void", PhpNameContext::Constant, Span::call_site()).unwrap();
+    }
+
+    #[test]
+    fn test_validate_php_name_allows_resource_and_numeric_for_class() {
+        use super::{PhpNameContext, validate_php_name};
+        use proc_macro2::Span;
+
+        // 'resource' and 'numeric' are NOT reserved for class names in PHP
+        validate_php_name("resource", PhpNameContext::Class, Span::call_site()).unwrap();
+        validate_php_name("numeric", PhpNameContext::Class, Span::call_site()).unwrap();
     }
 }
