Add phpdoc type annotations for psalm #221
Conversation
Updated the PHPDoc comments and typing for `AbstractMatcher` class to enhance clarity and ensure type safety. Added detailed doc comments for class properties, methods, and their parameters, aligning with psalm's type annotations.
Added two new methods `matchesClass` and `matchesMethod` to improve annotation matching by utilizing `ReflectionClass` and `ReflectionMethod`. Updated annotations and documentation for better code clarity and type safety.
Introduced generics to `MethodInvocation`, `Invocation`, and `Joinpoint` interfaces to enhance type safety and flexibility. Updated return type annotations and added psalm mutation-free annotation to `Joinpoint::getThis`.
Add `FakeMarker` annotation in `AnnotatedMatcherTest` and update test case. Enhance type annotations and improve code documentation in `ReflectiveMethodInvocation` for better type safety and clarity.
Updated the docblock in ReflectiveMethodInvocationTest to include a type parameter for better clarity and type hinting. This helps IDEs and static analysis tools to better understand the expected type, improving development efficiency and reducing potential errors.
Added a Psalm suppression to prevent false positives for DocblockTypeContradiction in the `getUnionType` method. This suppression ensures the static analysis tool does not raise incorrect errors related to type documentation.
Enhanced PHP docblocks for better clarity and added missing documentation annotations. This update includes type hints, param descriptions, and purpose explanations for methods and properties, improving code readability and maintainability.
This commit updates the PHP version in the psalm configuration from 8.2 to 8.3. No other changes or additions to the configuration file were made.
Added detailed PHPDoc annotations and improved type hints throughout the Bind class. This enhances code readability, maintainability, and provides better context for the data structures and parameters used. The visibility and structure of certain methods were also clarified for consistency.
This annotation specifies that the getBindings method does not mutate the state of the object, which can improve static analysis and ensure better code safety. This change makes the code more robust and maintainable.
Simplified the logic in bindInterceptors method by restructuring the ternary operator for better readability. This change improves code maintainability without altering the method's functionality.
A period was added to a docstring for consistency and a redundant type annotation was removed from a return statement in the `weave` method. This cleanup improves code readability and maintains coding standards.
Enhanced type annotations using Psalm for better static analysis and added detailed doc comments to clarify method functionalities. Also refined error handling and class instantiation assertions.
Include non-empty-string type hints for classDir and tmpDir parameters to ensure they are not empty. Additionally, enhance tmpDir default setting logic to fall back to '/tmp' if system temp directory is unavailable.
Updated `MatcherArguments` to use `array<array-key, mixed>` for clarity and added a new `Arguments` type definition. The changes ensure consistency in type annotations and improve code readability.
Updated the type annotations in AbstractMatcher and related classes to use `MatcherArguments`. This enhances code clarity and ensures better type safety across the matcher-related classes. Additionally, removed final modifier from AnnotatedMatcher to allow further extension.
Add a new psalm-type `Arguments` that represents an array with mixed values. Update the method parameter type annotation to use `Arguments` for better code clarity and consistency.
Imported multiple Psalm types from related classes to enhance type safety and clarity in MethodMatch.php. Updated the type hint for the $pointcuts parameter to use the imported Pointcuts type.
This annotation indicates that the getNamedArguments method is mutation-free, thereby improving code documentation and potential static analysis tools. It ensures that consumers of this method understand its immutability properties.
Added a class member to check for ReflectionUnionType support to improve readability. Also included docblocks and psalm annotations to enhance code documentation and static analysis.
Changed `@readonly` to `@re` for `$object` variable. Added `@psalm-readonly-allow-private-mutation` annotation for `$interceptors` variable to improve clarity and handle specific scenarios correctly.
Add the @readonly annotation to a private property and remove an outdated assert statement. Also, mark the getThis method as psalm-external-mutation-free for better static analysis.
Updated the `AopPostfixClassName` class to include `@readonly` annotations for the `fqn` and `postFix` properties. This change enhances code clarity and indicates that these properties should not be modified after initialization.
This change introduces the @readonly annotation to various class properties across several files, enhancing immutability and indicating that these properties should not be modified after initialization. This ensures better code clarity and helps prevent unintended side effects.
Enhance code by adding @psalm-external-mutation-free annotations, ensuring immutability guarantees within methods. Additionally, document the AopCode class and update existing comments for better clarity.
Annotated the 'any' and 'startsWith' methods in the Matcher class with @psalm-mutation-free. This ensures static analysis tools recognize these methods as not altering object state, enhancing code quality and reliability.
WalkthroughThe pull request introduces changes across multiple files, primarily focusing on enhancing type safety and documentation through the use of PHPDoc annotations and type definitions. Key updates include modifying the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #221 +/- ##
=============================================
+ Coverage 91.20% 100.00% +8.79%
+ Complexity 230 215 -15
=============================================
Files 27 27
Lines 591 558 -33
=============================================
+ Hits 539 558 +19
+ Misses 52 0 -52 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (44)
src/Annotation/AbstractAssisted.php (1)
17-18: Consider enhancing type safety further.The addition of
@readonlyis good for immutability. However, since this is a type-focused PR, consider these additional improvements:- public $values; + /** @var string[] */ + public readonly array $values;This would:
- Add native PHP type declaration (
array)- Use PHP 8.1+ native
readonlykeyword- Keep the PHPDoc for more specific typing (
string[])src/Pointcut.php (1)
Line range hint
9-24: Consider enhancing type safety with readonly properties.While marking the class as immutable is good, you can further strengthen type safety by making the properties readonly since they are only set in the constructor.
Here's how you can enhance it:
/** @psalm-immutable */ class Pointcut { - /** @var AbstractMatcher */ + /** @var AbstractMatcher */ + public readonly AbstractMatcher $classMatcher; - /** @var AbstractMatcher */ + /** @var AbstractMatcher */ + public readonly AbstractMatcher $methodMatcher; - /** @var array<MethodInterceptor|class-string<MethodInterceptor>> */ + /** @var array<MethodInterceptor|class-string<MethodInterceptor>> */ + public readonly array $interceptors;src/AopPostfixClassName.php (4)
12-14: Enhance class-level documentation for better static analysis.Consider adding more comprehensive PHPDoc annotations to improve static analysis:
- Add
@psalm-immutablesince the class has readonly properties- Add more detailed description about the purpose and behavior
/** - * Fully qualified name including postfix + * Generates a unique fully qualified class name by appending a postfix. + * The postfix is generated based on file modification time, bindings, and class directory. + * + * @psalm-immutable */
17-20: Improve type annotation for $fqn property.Since this property holds a class name with a postfix, we should use a more specific type annotation.
/** - * @var string + * @var class-string-with-aop-postfix * @readonly */
23-26: Add format documentation for $postFix property.The property documentation could be more descriptive about the format of the postfix.
/** * @var string + * @psalm-var non-empty-string + * @psalm-format _[0-9]+ * @readonly */
Runtime checks for file operations are necessary
The verification confirms that
AopPostfixClassNameis used in critical path operations in bothWeaver::weave()andCompilerclass, where it's used to generate class names for AOP operations. Given that:
- The class is used in core weaving/compilation logic
- No existing error handling for file operations is present
- Failed file operations could silently create invalid class names
The suggested changes to add explicit checks and throw exceptions for file operation failures are appropriate to prevent runtime issues.
src/Weaver.php: UsesAopPostfixClassNamefor weaving operationssrc/Compiler.php: Uses it during compilation phase🔗 Analysis chain
Line range hint
31-35: Add runtime checks for file operations.The constructor performs file operations that could fail:
filemtime()returnsfalseif the file doesn't existReflectionClass::getFileName()returnsfalsefor internal classesConsider adding explicit checks to handle these edge cases.
public function __construct(string $class, string $bindings, string $classDir) { - $fileTime = (string) filemtime((string) (new ReflectionClass($class))->getFileName()); + $reflection = new ReflectionClass($class); + $fileName = $reflection->getFileName(); + if ($fileName === false) { + throw new \RuntimeException("Cannot get filename for class: {$class}"); + } + $fileTime = filemtime($fileName); + if ($fileTime === false) { + throw new \RuntimeException("Cannot get modification time for file: {$fileName}"); + } - $this->postFix = '_' . crc32($fileTime . $bindings . $classDir); + $this->postFix = '_' . crc32((string)$fileTime . $bindings . $classDir); $this->fqn = $class . $this->postFix; }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this class is used with internal PHP classes rg -l "new \s*AopPostfixClassName.*PHP_INTERNAL" .Length of output: 52
Script:
#!/bin/bash # Let's check how AopPostfixClassName is used in the codebase rg "new AopPostfixClassName" -A 3 # Also check if there are any tests that might reveal usage patterns fd "test.*\.php" --exec rg "AopPostfixClassName" {} # Check the class definition and its usage context rg "class AopPostfixClassName" -B 2 -A 5Length of output: 980
Script:
#!/bin/bash # Let's check what kind of classes are passed to AopPostfixClassName rg "new AopPostfixClassName\((.*?)\)" -p # Check if there's any error handling around the usage rg "new AopPostfixClassName" -B 3 -A 3 # Look at the Weaver and Compiler class context to understand the usage better ast-grep --pattern 'class Weaver { $$$ function $name($$$) { $$$ new AopPostfixClassName($$$) $$$ } $$$ }' ast-grep --pattern 'class Compiler { $$$ function $name($$$) { $$$ new AopPostfixClassName($$$) $$$ } $$$ }'Length of output: 1245
src/MethodInvocation.php (1)
Line range hint
19-19: Update interface definition to include generic type parameter.The interface definition should be updated to match the PHPDoc annotations.
Apply this diff:
-interface MethodInvocation extends Invocation +interface MethodInvocation<T of object> extends Invocation<T>src/MethodInterceptor.php (1)
22-22: LGTM! Well-structured type annotations.The addition of the generic type parameter
Tand its constraint to objects enhances type safety while maintaining backward compatibility. The changes align well with psalm's type checking capabilities and follow PHPDoc standards.This change is part of a good architectural pattern where:
- Generic types allow for better type safety at compile time
- The constraint
T of objectensures type consistency- The documentation remains clear and maintainable
Also applies to: 26-27
src/AbstractMatcher.php (3)
12-17: Consider consolidating identical type definitions.The
MatcherArgumentsandArgumentstypes are defined identically but with different names. This could lead to confusion about their intended purposes. Consider either:
- Using a single type definition if they serve the same purpose
- Adding documentation to clarify the semantic difference between these types
23-25: Enhance constructor documentation.The current constructor documentation is empty. Consider documenting:
- The expected arguments and their types
- The variadic nature of the constructor
- Usage examples if applicable
Example improvement:
/** * Constructor + * + * @param mixed ...$arguments Matching condition arguments that will be used + * for evaluating matcher conditions */
54-54: Consider using consistent type aliases.The method uses the
Argumentstype alias for its return type annotation while the rest of the class usesMatcherArguments. Consider using the same type alias consistently throughout the class to avoid confusion./** - * @return Arguments + * @return MatcherArguments */Also applies to: 56-56
src/BuiltinMatcher.php (1)
Line range hint
31-38: Enhance type safety in matcher instantiation.While the implementation is functional, it could benefit from additional type safety measures.
Consider applying these improvements:
- assert(class_exists($matcherClass)); - $matcher = (new ReflectionClass($matcherClass))->newInstance(); + assert(class_exists($matcherClass) && is_subclass_of($matcherClass, AbstractMatcher::class)); + $matcherReflection = new \ReflectionClass($matcherClass); + $matcher = $matcherReflection->newInstance();src/Matcher.php (2)
50-51: LGTM! Consider adding parameter type for$prefixThe
@psalm-mutation-freeannotation is correctly applied. However, to further enhance type safety, consider adding a parameter type for$prefix.- public function startsWith($prefix): AbstractMatcher + public function startsWith(string $prefix): AbstractMatcher
Line range hint
1-89: Consider applying@psalm-mutation-freeconsistentlyFor consistency, consider adding the
@psalm-mutation-freeannotation to other methods that follow the same pattern of only creating and returning new instances without modifying state:logicalOr,logicalAnd, andlogicalNot.src/ReflectionMethod.php (2)
Line range hint
25-39: LGTM: Well-typed reflection method with proper null checks.The method is well-implemented with proper type assertions and psalm annotations. Consider adding a
@throwsannotation to document potential exceptions fromReflectionClassconstruction./** * @return ReflectionClass<object> * + * @throws \ReflectionException When the class does not exist * @psalm-external-mutation-free * @psalm-suppress MethodSignatureMismatch */
Line range hint
41-51: Consider adding error handling for non-existent methods.The method checks for class existence but should also handle cases where the method name doesn't exist in the class.
public function getAnnotations(): array { assert(class_exists($this->class)); + assert(method_exists($this->class, $this->name)); /** @var list<object> $annotations */ $annotations = ServiceLocator::getReader()->getMethodAnnotations(new \ReflectionMethod($this->class, $this->name)); return $annotations; }src/CompilerInterface.php (2)
35-35: Consider making ConstructorArguments more specific.The
ConstructorArgumentstype is currently defined aslist<mixed>. Consider making it more specific by defining the expected argument types if they are known at compile time.-@psalm-type ConstructorArguments = list<mixed> +@psalm-type ConstructorArguments = list<scalar|array|object>
Line range hint
54-64: Add return type hint to newInstance method.While the PHPDoc provides the return type information, the method signature lacks a return type hint which is recommended in PHP 8.x for better type safety.
- public function newInstance(string $class, array $args, BindInterface $bind); + public function newInstance(string $class, array $args, BindInterface $bind): object;src/ReflectionClass.php (1)
33-34: Add return type annotation for better type safety.The
@psalm-external-mutation-freeannotation is correct as the method doesn't modify any external state. However, to further improve type safety, consider adding a return type annotation.Add this annotation:
* {@inheritDoc} * * @psalm-external-mutation-free + * @return object|nullsrc/Weaver.php (1)
39-39: Consider adding runtime validation for non-empty-string.While the
@param non-empty-stringannotation helps with static analysis, consider adding runtime validation to ensure the contract is enforced:/** @param non-empty-string $classDir */ public function __construct(BindInterface $bind, string $classDir) { + if ($classDir === '') { + throw new \InvalidArgumentException('Class directory path cannot be empty'); + } $this->bind = $bind; $this->bindName = (string) $bind; $this->compiler = new Compiler($classDir); $this->classDir = $classDir; }src/Bind.php (2)
42-53: Consider removing unnecessary empty line.The changes look good, but there's an extra empty line at line 51 that could be removed for better code consistency.
public function bind(string $class, array $pointcuts): BindInterface { $pointcuts = $this->getAnnotationPointcuts($pointcuts); $reflectionClass = new ReflectionClass($class); - $methods = $reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC);
94-95: Consider enhancing the docblock with serialization details.While the current documentation is clear, it could be more helpful to include details about the serialized format.
/** - * Get serialized representation of bindings + * Get serialized representation of bindings + * + * @return string Serialized array of method interceptor bindings */src/TypeString.php (4)
18-20: Enhance class-level PHPDoc annotationsConsider adding
@psalm-immutableand@finalannotations to better document the class's immutable nature and final status./** + * @psalm-immutable + * @final * TypeString converts ReflectionType instances to their corresponding string representations. */
41-42: Add specific return type annotationConsider adding a more specific return type annotation for better type safety.
+ /** * @psalm-external-mutation-free + * @return string Empty string for null type, type string representation otherwise */
81-83: Add @psalm-external-mutation-free to getUnionType methodFor consistency with other methods in the class, consider adding the annotation.
+ /** @psalm-external-mutation-free */ public function getUnionType(ReflectionUnionType $type): string
Line range hint
85-87: Enhance array type annotation specificityThe array type annotation could be more specific about the intersection types.
- /** @var array<ReflectionNamedType> $types */ + /** @var array<int, ReflectionNamedType> $types */tests/ReflectiveMethodInvocationTest.php (1)
14-14: LGTM! Consider adding @psalm-immutable to the test class.The addition of the generic type parameter
<FakeClass>toReflectiveMethodInvocationimproves type safety and makes the relationship between the invocation andFakeClassexplicit. This change aligns well with the PR's objective of enhancing PHPDoc annotations for psalm.Since this PR focuses on psalm annotations, consider adding
@psalm-immutableto the test class as all properties are only modified insetUp():+/** @psalm-immutable */ class ReflectiveMethodInvocationTest extends TestCasesrc/ReflectiveMethodInvocation.php (2)
16-22: LGTM! Consider adding usage examples in the class documentation.The template and type definitions are well-structured and improve type safety. The use of
@psalm-typefor custom types enhances code maintainability.Consider adding a code example in the class documentation to demonstrate proper usage with the template type T:
/** * @template T of object * @psalm-type ArgumentList = ArrayObject<int, mixed> * @psalm-type NamedArguments = ArrayObject<non-empty-string, mixed> * @psalm-type InterceptorList = array<MethodInterceptor> * @implements MethodInvocation<T> + * + * @example + * ```php + * class MyService {} + * $invocation = new ReflectiveMethodInvocation<MyService>( + * new MyService(), + * 'methodName', + * ['arg1', 'arg2'] + * ); + * ``` */
56-59: Consider adding runtime validation for non-empty-string.The parameter type annotations are accurate and well-documented. However, since
non-empty-stringis a static analysis type, consider adding runtime validation.Consider adding this validation:
public function __construct( object $object, string $method, array $arguments, array $interceptors = [] ) { + if ($method === '') { + throw new InvalidArgumentException('Method name cannot be empty'); + } $this->object = $object; $this->method = $method;src/MethodSignatureString.php (1)
Line range hint
1-144: Consider redesigning to embrace immutability throughout the class.The class currently mixes immutable and mutable patterns through the use of reference parameters (
&$signatureParts) while trying to mark methods as mutation-free. Consider redesigning the class to:
- Consistently use immutable patterns by returning new arrays instead of modifying references
- Chain method calls instead of accumulating changes through references
- Consider using a builder pattern if the current reference-based approach is performance-critical
This would make the code more predictable and truly align with the
@psalm-external-mutation-freeannotations.src/Compiler.php (3)
35-38: Consider making the property private.While the type annotations and
@readonlyare good additions, consider making the$classDirproperty private to better encapsulate the class's internal state. If the directory path needs to be accessed externally, you could add a getter method.- public $classDir; + private $classDir; + + public function getClassDir(): non-empty-string + { + return $this->classDir; + }
85-85: Fix typo in @sideeffect annotation.There's a typo in the annotation: "Genaerates" should be "Generates".
- * @sideEffect Genaerates a new class file + * @sideEffect Generates a new class file
Line range hint
90-113: Consider simplifying the early return pattern.The current implementation uses
gotofor the return path. While this works, a simpler approach would be more maintainable:- /** @var class-string<T> $class */ - return $class; + return $class; // @var class-string<T> $class } $className = new AopPostfixClassName($class, (string) $bind, $this->classDir); if (class_exists($className->fqn, false)) { - goto return_fqn; + /** @var class-string<T> $className->fqn */ + return $className->fqn; } try { $this->requireFile($className, new ReflectionClass($class), $bind); // @codeCoverageIgnoreStart } catch (ParseError $e) { $msg = sprintf('class:%s Compilation failed in Ray.Aop. This is most likely a bug in Ray.Aop, please report it to the issue. https://github.com/ray-di/Ray.Aop/issues', $class); throw new CompilationFailedException($msg); // @codeCoverageIgnoreEnd } - return_fqn: - $fqn = $className->fqn; // phpcs:ignore SlevomatCodingStandard.Variables.UselessVariable.UselessVariable - /** @var class-string<T> $fqn */ - - return $fqn; + /** @var class-string<T> $className->fqn */ + return $className->fqn;src/AopCode.php (5)
26-31: Consider enhancing class documentation with immutability details.The class documentation clearly explains the purpose, but given the extensive use of
@psalm-external-mutation-freeannotations throughout the class, it would be helpful to document the class's immutability characteristics./** * Aop Code * * AopCode is responsible for generating code for Aspect-Oriented Programming (AOP) * by adding interceptors and modifying class definitions. + * + * This class maintains immutability in its operations, with all public methods + * being external-mutation-free. */
50-54: Consider adding a more specific return type annotation.While the parameter types are well-documented, the return type could be more specific using
@return non-empty-stringsince the method always returns generated code./** * @param ReflectionClass<object> $sourceClass + * @return non-empty-string * * @psalm-external-mutation-free */
64-68: Consider adding parameter type annotation.The method's documentation could benefit from a parameter type annotation to specify the expected string type.
/** * @return void + * @param string $text The code text to append * * @psalm-external-mutation-free */
202-206: Fix typo in method name: "Intercepter" should be "Interceptor".The method name contains a spelling error. While the code works, it's better to use the correct spelling for consistency and clarity.
- private function addIntercepterTrait(): void + private function addInterceptorTrait(): void
Line range hint
208-219: Enhance method documentation with return type and description.The method could benefit from more detailed documentation about its purpose and return value constraints.
-/** @psalm-external-mutation-free */ +/** + * Returns the generated code text, ensuring all curly braces are properly closed. + * + * @return non-empty-string The complete generated code + * @psalm-external-mutation-free + */src/AnnotatedMatcher.php (2)
16-18: Enhance type safety by using typed properties for$annotation.Since PHP 7.4, you can declare typed properties. Consider specifying the type of
$annotationexplicitly for better type safety:Apply this diff to update the property declaration:
- /** - * @var class-string - * @readonly - */ public $annotation; + /** + * @var class-string + * @readonly + */ + public readonly string $annotation;This change uses the native
stringtype hint while retaining the@var class-stringannotation for static analysis tools like Psalm.
22-24: Consider enforcing the array shape of$argumentsin the constructor.The constructor expects
$argumentsto be an array with a specific shape:array{0: class-string}. To improve type safety and clarity, consider enforcing this array shape in the parameter:If possible, you could use a value object or a dedicated type to represent the arguments, or add runtime checks to ensure the array conforms to the expected structure.
public function __construct(string $matcherName, array $arguments) { if (!isset($arguments[0]) || !is_string($arguments[0])) { throw new \InvalidArgumentException('Expected $arguments[0] to be a class-string.'); } parent::__construct($matcherName, $arguments); $this->annotation = $arguments[0]; }This addition ensures that
$arguments[0]is set and is a string, preventing potential runtime errors.src/Aspect.php (4)
27-39: Separate Psalm Type Definitions for ClarityThe Psalm type definitions are currently placed within the class docblock. It's better to place these type definitions outside the class docblock to improve readability and maintainability.
Consider moving the type definitions above the class declaration:
+/** + * @psalm-type MethodInterceptors = array<array-key, MethodInterceptor> + * @psalm-type MethodBindings = array<string, MethodInterceptors> + * @psalm-type ClassBindings = array<class-string, MethodBindings> + * @psalm-type MatcherConfig = array{ + * classMatcher: AbstractMatcher, + methodMatcher: AbstractMatcher, + interceptors: MethodInterceptors + } + * @psalm-type Arguments = array<array-key, mixed> + */ /** - * Aspect class manages aspect weaving and method interception - * - * @psalm-type MethodInterceptors = array<array-key, MethodInterceptor> - * @psalm-type MethodBindings = array<string, MethodInterceptors> - * @psalm-type ClassBindings = array<class-string, MethodBindings> - * @psalm-type MatcherConfig = array{ - * classMatcher: AbstractMatcher, - methodMatcher: AbstractMatcher, - interceptors: MethodInterceptors - } - * @psalm-type Arguments = array<array-key, mixed> - */ + * Aspect class manages aspect weaving and method interception */ final class Aspect
94-100: Improve Exception Message inweave()MethodThe exception message in the
weave()method can be more informative to guide users on how to resolve the issue.Update the exception message:
if (! extension_loaded('rayaop')) { - throw new RuntimeException('Ray.Aop extension is not loaded. Cannot use weave() method.'); + throw new RuntimeException('The "rayaop" extension is not loaded. Please install and enable it to use the weave() method.'); }This provides clearer instructions for the user.
208-215: Enhance Docblock with Proper Generics innewInstance()MethodThe
newInstance()method's docblock can be improved for better static analysis support.Ensure that the
@templateannotation is placed correctly:+ /** + * Create new instance with woven aspects + * + * @template T of object + * @param class-string<T> $className Target class name + * @param list<mixed> $args Constructor arguments + * + * @return T New instance with aspects + * + * @throws RuntimeException When temporary directory is not set for PHP-based AOP. + */This helps tools like Psalm and PHPStan to accurately infer return types.
Line range hint
231-247: Correct Type Annotations innewInstanceWithPecl()MethodThe
newInstanceWithPecl()method's docblock has a mismatch in parameter types.Update the docblock to reflect the correct parameter type:
/** * Create instance using PECL extension * - * @param class-string<T> $className - * @param Arguments $args + * @template T of object + * @param class-string<T> $className + * @param list<mixed> $args * * @return T * - * @template T of object */This ensures consistency with the method signature and aids static analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (29)
psalm.xml(1 hunks)src/AbstractMatcher.php(4 hunks)src/AnnotatedMatcher.php(1 hunks)src/Annotation/AbstractAssisted.php(1 hunks)src/AopCode.php(6 hunks)src/AopPostfixClassName.php(1 hunks)src/Aspect.php(8 hunks)src/Bind.php(3 hunks)src/BuiltinMatcher.php(2 hunks)src/Compiler.php(4 hunks)src/CompilerInterface.php(1 hunks)src/InterceptTrait.php(1 hunks)src/Invocation.php(1 hunks)src/Joinpoint.php(2 hunks)src/Matcher.php(2 hunks)src/MethodInterceptor.php(1 hunks)src/MethodInvocation.php(1 hunks)src/MethodMatch.php(2 hunks)src/MethodSignatureString.php(4 hunks)src/Pointcut.php(1 hunks)src/PriorityPointcut.php(1 hunks)src/ReflectionClass.php(1 hunks)src/ReflectionMethod.php(1 hunks)src/ReflectiveMethodInvocation.php(4 hunks)src/TokenIterator.php(2 hunks)src/TypeString.php(4 hunks)src/Weaver.php(1 hunks)tests/AnnotatedMatcherTest.php(2 hunks)tests/ReflectiveMethodInvocationTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- psalm.xml
- src/PriorityPointcut.php
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/Aspect.php
[warning] 73-73: src/Aspect.php#L73
Added line #L73 was not covered by tests
[warning] 107-107: src/Aspect.php#L107
Added line #L107 was not covered by tests
[warning] 114-114: src/Aspect.php#L114
Added line #L114 was not covered by tests
[warning] 175-176: src/Aspect.php#L175-L176
Added lines #L175 - L176 were not covered by tests
[warning] 202-202: src/Aspect.php#L202
Added line #L202 was not covered by tests
🔇 Additional comments (56)
tests/AnnotatedMatcherTest.php (2)
8-8: LGTM!
The import statement follows PSR-4 autoloading standard.
17-18: LGTM! Verify FakeMarker annotation.
The test modification improves type safety by using ::class constant instead of string literals. Let's verify the FakeMarker annotation class exists and is properly implemented.
✅ Verification successful
FakeMarker annotation is properly implemented and used
The FakeMarker annotation class is correctly implemented in tests/Fake/Annotation/FakeMarker.php with:
- Proper PHP 8 attribute and Doctrine annotation declarations
- Method-level targeting
- Constructor with typed property
- Used extensively in tests for method annotations
The test modification to use ::class constant is safe and maintains type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify FakeMarker annotation class exists and is properly implemented
# Test 1: Check if FakeMarker annotation class exists
echo "Checking FakeMarker annotation class..."
rg -l "class FakeMarker"
# Test 2: Verify it's used as an annotation
echo "Verifying FakeMarker is used as an annotation..."
ast-grep --pattern 'class FakeMarker {
$$$
}' -l
Length of output: 649
Script:
#!/bin/bash
# Let's check the content of FakeMarker class and its usage
echo "Checking FakeMarker class implementation:"
cat tests/Fake/Annotation/FakeMarker.php
echo -e "\nChecking usage of FakeMarker in tests:"
rg "FakeMarker" tests/ -C 2
Length of output: 18888
src/Pointcut.php (1)
7-7: LGTM! The @psalm-immutable annotation correctly reflects the class design.
The annotation aligns with the class implementation where properties are only set in the constructor.
src/MethodInvocation.php (1)
14-15: LGTM: Well-structured generic type annotations.
The addition of generic type parameter T and its constraint to objects, along with proper extension of Invocation<T>, enhances type safety.
src/TokenIterator.php (1)
18-22: LGTM! Accurate type annotation and mutation-free marking.
The @psalm-external-mutation-free annotation correctly indicates that this method doesn't modify any external state, and the return type annotation precisely describes the [int, string] tuple return value.
src/Invocation.php (1)
15-16: LGTM! Good enhancement of type safety.
The addition of generic type support through @template T of object and @extends Joinpoint<T> improves type safety while maintaining backward compatibility. This change aligns well with modern PHP practices and the PR's objective of enhancing type annotations.
Let's verify that the Joinpoint interface exists and supports generics:
✅ Verification successful
Let me verify the Joinpoint interface implementation with additional context.
Verified: The generic type implementation is correctly aligned
The Joinpoint interface properly supports generics with @template T of object and correctly uses the generic type parameter in its method signature (@return T). The changes in Invocation.php to extend this generic interface are consistent and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Joinpoint interface exists and supports generics
# Expected: Find Joinpoint interface with generic type support
# Search for Joinpoint interface definition
ast-grep --pattern 'interface Joinpoint'
# Search for template annotation in Joinpoint
rg '@template' "$(fd -t f Joinpoint.php)"
Length of output: 125
Script:
#!/bin/bash
# Get the full content of Joinpoint.php to verify its interface definition and generic support
cat src/Joinpoint.php
Length of output: 1546
src/InterceptTrait.php (3)
19-22: LGTM! The $isAspect property type and mutability are well-defined.
The boolean type annotation and readonly modifier are appropriate for this control flag.
13-22: Consider impact of readonly properties in traits.
Readonly properties in traits can be problematic if the trait is used in multiple classes with different initialization requirements. Ensure all classes using this trait properly initialize these properties.
#!/bin/bash
# Find all classes using InterceptTrait
echo "Classes using InterceptTrait:"
rg -l "use\s+.*InterceptTrait" src/
# Check initialization patterns
echo -e "\nProperty initialization patterns:"
ast-grep --pattern 'class $_ {
use Ray\Aop\InterceptTrait;
$$$
}'13-16: Verify the immutability of $bindings array elements.
While @readonly prevents reassignment of the $bindings property itself, the array elements can still be modified. Consider using @psalm-immutable to ensure deep immutability of the interceptor collection.
src/AbstractMatcher.php (2)
20-20: LGTM! Property type annotation enhances type safety.
The @var MatcherArguments annotation properly leverages the psalm type definition, improving static analysis capabilities.
34-35: LGTM! Method parameter annotations are consistent and well-documented.
The parameter annotations properly use the MatcherArguments type and maintain clear documentation.
Also applies to: 44-45
src/BuiltinMatcher.php (2)
14-17: LGTM! Well-structured PHPDoc annotations.
The added PHPDoc annotations improve type safety and properly handle Psalm warnings.
26-27: Verify usage of BuiltinMatcher constructor across the codebase.
The parameter type change from array to MatcherArguments might affect existing code.
✅ Verification successful
Let me gather more information about the MatcherArguments type to understand the impact better.
Type change is safe and consistent with the codebase
The MatcherArguments is actually a psalm type alias for array<array-key, mixed> defined in AbstractMatcher.php. The constructor parameter is correctly typed as array in the signature while having a @param MatcherArguments docblock annotation. All usages across the codebase pass array arguments which is compatible with this type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instantiations of BuiltinMatcher to verify compatibility with new type
# Search for new BuiltinMatcher instantiations
echo "Searching for BuiltinMatcher instantiations..."
rg "new BuiltinMatcher\(" -A 2
# Search for classes extending BuiltinMatcher
echo "Searching for classes extending BuiltinMatcher..."
ast-grep --pattern 'class $_ extends BuiltinMatcher'
Length of output: 1573
Script:
#!/bin/bash
# Search for MatcherArguments class/type definition and usage
echo "Searching for MatcherArguments definition..."
rg "MatcherArguments" -A 3
# Search for any type hints or docblocks using MatcherArguments
echo "Searching for type hints and docblocks..."
ast-grep --pattern '@param MatcherArguments'
Length of output: 1549
src/Joinpoint.php (2)
24-24: LGTM! Good enhancement of type safety.
The addition of the generic template T of object and updating the return type of getThis() to T improves type safety and makes the interface more precise. This change aligns well with modern PHP type system practices and enhances static analysis capabilities.
Also applies to: 45-45
47-47: LGTM! Good documentation of method behavior.
The @psalm-mutation-free annotation appropriately documents that getThis() is a pure getter method that doesn't modify state, which helps Psalm's analysis.
src/Matcher.php (1)
16-17: LGTM! Correct usage of @psalm-mutation-free
The annotation accurately reflects that the any() method doesn't modify any object state and only returns a new instance.
src/ReflectionMethod.php (2)
Line range hint 16-23: LGTM: Well-typed property and setter method.
The new property and its setter method are properly typed and implemented according to PHP 8 standards.
Line range hint 53-77: LGTM: Well-typed generic method with proper psalm annotations.
The method is properly typed using PHP 8 generics syntax and includes appropriate psalm annotations.
src/CompilerInterface.php (3)
8-11: LGTM! Clean type imports from Aspect.
The type imports are well-structured and follow PSR standards.
12-34: LGTM! Well-structured type definitions.
The complex types are clearly defined with proper nesting and type constraints, improving type safety and code clarity.
39-49: LGTM! Well-documented compile method.
The method signature and documentation provide clear type safety with proper template usage and return type specification.
src/ReflectionClass.php (2)
Line range hint 1-100: Implementation looks good with proper type safety.
The class is well-structured with appropriate type annotations:
- Uses generics correctly with
@template T of object - Methods have proper psalm annotations
- Return types are properly specified
33-34: Verify annotation consistency across similar methods.
Let's check if similar methods in related classes have consistent psalm annotations.
✅ Verification successful
Let me check the implementation of getAnnotation methods to verify annotation consistency.
Annotation is consistently applied across similar methods
The @psalm-external-mutation-free annotation is correctly and consistently applied to the getAnnotation method in both ReflectionClass and ReflectionMethod classes. Both implementations follow the same pattern with similar docblocks and annotations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @psalm-external-mutation-free annotations
# in similar Reader interface implementations
# Find all classes implementing Reader interface and check their getAnnotation methods
rg -l "implements.*Reader" | xargs rg -l "getAnnotation"
# Check for existing @psalm-external-mutation-free annotations in these files
rg "@psalm-external-mutation-free" --type php
Length of output: 1550
Script:
#!/bin/bash
# Get the implementation of getAnnotation methods from both files
ast-grep --pattern 'function getAnnotation($$$) { $$$ }'
# Also get the docblocks for these methods
rg -B 5 "function getAnnotation" src/ReflectionClass.php src/ReflectionMethod.php
Length of output: 1339
src/Weaver.php (2)
15-36: LGTM! Good use of @readonly annotations.
Adding @readonly annotations to these properties is a good practice as it:
- Enforces immutability after construction
- Makes the code more maintainable and safer
- Aligns with modern PHP practices
15-39: Verify consistent usage of type annotations across the codebase.
Let's verify if these type safety improvements are consistently applied across related classes.
✅ Verification successful
Let me gather more information to verify the type safety improvements.
Type safety improvements are consistently applied across the codebase
The verification shows that:
- The
non-empty-stringtype hint is consistently used across related classes (Compiler, Aspect, ReflectiveMethodInvocation, etc.) - The Compiler class also uses
@readonlyannotation for its properties - All classes instantiating Weaver are using valid directory paths that satisfy the non-empty-string requirement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in type annotations across related classes
# Test 1: Check if other classes instantiating Weaver are aware of non-empty-string requirement
echo "Checking Weaver instantiations:"
rg "new\s+Weaver\s*\(" -A 2
# Test 2: Check if related classes (like Compiler) have similar @readonly annotations
echo -e "\nChecking related classes for @readonly annotations:"
ast-grep --pattern $'/**
* @readonly
*/'
Length of output: 1237
Script:
#!/bin/bash
# Check for non-empty-string type hints and docblocks in related files
echo "Checking for non-empty-string type hints:"
rg "@param\s+non-empty-string" -B 2 -A 2
# Check property definitions in Weaver and related classes
echo -e "\nChecking property definitions with readonly:"
rg "@readonly\s*\n\s*private" -B 1 -A 1
# Check Compiler class specifically as it's referenced in Weaver
echo -e "\nChecking Compiler class definition:"
rg "class\s+Compiler" -A 10 -B 2
Length of output: 3251
src/Bind.php (6)
11-17: LGTM! Well-structured type imports and documentation.
The class-level docblock effectively imports necessary types and defines a custom type for Pointcuts, improving type safety and code clarity.
20-24: LGTM! Property type annotation is consistent.
The property docblock correctly uses the imported MethodBindings type, maintaining type safety.
35-35: LGTM! More precise return type for __sleep.
The return type list<'bindings'> is more specific than list<string>, providing better type safety.
67-76: LGTM! Improved type annotations and formatting.
The method documentation now correctly uses imported types, and the code formatting enhances readability.
82-86: LGTM! Appropriate type annotations and mutation-free designation.
The method correctly uses the imported MethodBindings type and is appropriately marked as mutation-free.
104-106: LGTM! Consistent use of custom type.
The method correctly uses the custom Pointcuts type, and its private visibility is appropriate for an internal helper method.
src/MethodMatch.php (3)
13-20: LGTM! Well-structured type imports.
The added Psalm type imports enhance type safety and make type dependencies explicit, improving code maintainability.
33-34:
Critical: Type mismatch between PHPDoc and method signature.
The PHPDoc annotation specifies Pointcuts but the method signature still uses array. This mismatch needs to be resolved:
- If
Pointcutsis the intended type:- Update the method signature to use
Pointcuts - Ensure the implementation supports the
Pointcutsinterface
- Update the method signature to use
- If array is correct:
- Update the PHPDoc to reflect the array type
Let's verify the intended type:
✅ Verification successful
Based on the results, I can now provide the final response:
PHPDoc @param Pointcuts is correct but needs array type hint
The Pointcuts is defined as a psalm type alias in Bind.php: @psalm-type Pointcuts = array<Pointcut>. The implementation correctly uses array operations since Pointcuts is just a type alias for an array of Pointcut objects. To maintain consistency:
- Update the method signature to include the array type hint:
public function __invoke(ReflectionClass $class, \Ray\Aop\ReflectionMethod $method, array $pointcuts) - Keep the PHPDoc as
@param Pointcuts $pointcutssince it's a valid psalm type import - Consider adding
@psalm-param Pointcuts $pointcutsfor more precise static analysis
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definition and usage of Pointcuts type
# Check if Pointcuts is defined as a type alias or class
ast-grep --pattern 'type Pointcuts = $_'
ast-grep --pattern 'class Pointcuts'
# Check how Pointcuts is used in other files
rg -t php "Pointcuts" -A 3 -B 3
Length of output: 2438
Line range hint 34-46: Implementation uses array operations on $pointcuts.
The current implementation uses array operations (foreach, unset, array access) on $pointcuts. If changing to Pointcuts type, ensure that:
- The
Pointcutstype supports these array operations - The implementation is updated to use appropriate methods from the
Pointcutsinterface
Let's check the Pointcuts implementation:
src/TypeString.php (1)
23-33: LGTM! Properties are well-documented with proper type annotations
The properties are correctly marked as readonly and have appropriate type annotations.
src/ReflectiveMethodInvocation.php (2)
92-95: LGTM! Verify mutation-free method implementations.
The mutation-free annotations are appropriate for these getter methods.
Let's verify that these methods don't modify any state:
#!/bin/bash
# Description: Verify that methods marked as mutation-free don't modify properties
# Search for property assignments within these methods
ast-grep --pattern $'function getArguments() {
$$$
$this->$_ = $$$
$$$
}'
ast-grep --pattern $'function getThis() {
$$$
$this->$_ = $$$
$$$
}'Also applies to: 139-140
25-52: LGTM! Verify callable signature compatibility.
The property type annotations are precise and the immutability constraints are well-defined. The use of @psalm-readonly-allow-private-mutation for $interceptors correctly allows modification in the proceed() method.
Let's verify that the callable type signature matches all method invocations in the codebase:
✅ Verification successful
Based on the results we've gathered so far, we can see that:
- The callable is used in a
call_user_func_arraywith$this->argumentscast to array - The property is initialized in the constructor
- The type annotation is
@var callable(mixed...): mixed
Since we've found the actual usage which matches the type signature (using variadic arguments via call_user_func_array and returning the result directly), I can now provide the final response:
✓ Callable type signature is correctly annotated and matches usage
The callable(mixed...): mixed type signature accurately reflects its usage in the codebase where it's invoked via call_user_func_array with the arguments array, allowing for variadic parameters and mixed return type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify callable usage matches the type signature (mixed...): mixed
# Search for method invocations using call_user_func_array with $this->callable
rg -U "call_user_func_array.*?\\\$this->callable.*?\\\$this->arguments" -g "*.php"
Length of output: 156
Script:
#!/bin/bash
# Let's search for the actual usage of the callable property in the codebase
rg -U "\\\$this->callable" -g "*.php" -A 3
# Also search for the constructor to see how the callable is initialized
ast-grep --pattern 'public function __construct($_) {
$$$
$this->callable = $$$
$$$
}'
Length of output: 659
Script:
#!/bin/bash
# Let's see the constructor definition to understand how callable is initialized
ast-grep --pattern 'public function __construct($$$) {
$$$
}'
# Also check the class definition to see template parameters
ast-grep --pattern 'class ReflectiveMethodInvocation$$$'
Length of output: 124
Script:
#!/bin/bash
# Let's try a different pattern to find the class and constructor
rg -U "class ReflectiveMethodInvocation.*?\{" -g "*.php" -A 10
# Also get the proceed method implementation where callable is used
rg -U "public function proceed\(\).*?\{.*?return" -g "*.php" -A 5
Length of output: 132
src/MethodSignatureString.php (3)
39-39: LGTM! Correct annotation for immutable method.
The @psalm-external-mutation-free annotation is appropriate here as the method only reads from the ReflectionMethod parameter and doesn't modify any external state.
50-54: LGTM! Well-documented immutable method.
The docblock is properly formatted and the annotations are accurate. The method is correctly marked as mutation-free as it only performs read operations.
118-119: LGTM! Appropriate annotation for pure string formatting method.
The @psalm-external-mutation-free annotation is correct as the method only performs string operations without modifying any external state.
src/Compiler.php (2)
24-32: LGTM! Well-documented class with proper type imports.
The class documentation is comprehensive and the @psalm-import-type annotation correctly imports the ConstructorArguments type.
41-41: LGTM! Proper parameter type annotation.
The non-empty-string type annotation for $classDir parameter matches the property type.
src/AopCode.php (2)
84-88: LGTM!
The method documentation is complete with appropriate parameter type constraints and mutation-free annotation.
150-154: LGTM!
The method documentation is well-defined with appropriate interface-string type constraint and mutation-free annotation.
src/AnnotatedMatcher.php (5)
37-40: Simplify the instantiation logic of Ray\Aop\ReflectionClass.
The current code checks if $class is an instance of Ray\Aop\ReflectionClass before creating a new instance:
$rayClass = $class instanceof \Ray\Aop\ReflectionClass ? $class : new \Ray\Aop\ReflectionClass($class->getName());Consider always creating a new instance for consistency and simplicity since Ray\Aop\ReflectionClass extends ReflectionClass:
Apply this diff to simplify the code:
- $rayClass = $class instanceof \Ray\Aop\ReflectionClass ? $class : new \Ray\Aop\ReflectionClass($class->getName());
+ $rayClass = new \Ray\Aop\ReflectionClass($class->getName());This change ensures that $rayClass is always an instance of Ray\Aop\ReflectionClass, reducing complexity.
Ensure this change does not impact performance or functionality by confirming that no additional overhead or side effects are introduced.
50-53: Simplify the instantiation logic of Ray\Aop\ReflectionMethod.
Similarly, in the matchesMethod method, you can simplify the instantiation:
$rayMethod = $method instanceof \Ray\Aop\ReflectionMethod ? $method : new \Ray\Aop\ReflectionMethod($method->class, $method->getName());Consider always instantiating a new Ray\Aop\ReflectionMethod object:
Apply this diff:
- $rayMethod = $method instanceof \Ray\Aop\ReflectionMethod ? $method : new \Ray\Aop\ReflectionMethod($method->class, $method->getName());
+ $rayMethod = new \Ray\Aop\ReflectionMethod($method->class, $method->getName());This streamlines the code and ensures consistency.
Verify that this modification does not adversely affect method behavior or introduce any unexpected side effects.
48-56: Verify that matchesMethod correctly overrides the parent method signature.
Similarly, confirm that matchesMethod(ReflectionMethod $method, array $arguments): bool properly overrides the corresponding method in the parent class with matching parameters and return type.
Use the following script to check the method signatures:
#!/bin/bash
# Description: Check method signature consistency for matchesMethod
# Use PHP's ReflectionMethod to inspect the parent method signature
php -r '
$child = new ReflectionMethod("Ray\Aop\AnnotatedMatcher", "matchesMethod");
$parent = $child->getPrototype();
echo "Child method signature: ";
print_r($child->getParameters());
echo "\nParent method signature: ";
print_r($parent->getParameters());
'This will ensure that matchesMethod is correctly overriding the parent's method without signature conflicts.
35-43: Verify that matchesClass correctly overrides the parent method signature.
Ensure that the signature of matchesClass(ReflectionClass $class, array $arguments): bool matches the method it's overriding in the parent class. Any discrepancies in the method signature could lead to unexpected behavior.
Run the following script to compare method signatures:
This script compares the parameters of matchesClass in both the child and parent classes to ensure they match.
✅ Verification successful
Method signature matches the parent abstract class correctly
The implementation in AnnotatedMatcher perfectly matches the abstract method signature from AbstractMatcher:
- Child:
public function matchesClass(ReflectionClass $class, array $arguments): bool - Parent:
abstract public function matchesClass(ReflectionClass $class, array $arguments)
The only difference is the explicit return type : bool in the child class, which is a valid PHP type declaration that doesn't break the contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check method signature consistency for matchesClass
# Use PHP's ReflectionClass to inspect the parent method signature
php -r '
$child = new ReflectionMethod("Ray\Aop\AnnotatedMatcher", "matchesClass");
$parent = $child->getPrototype();
echo "Child method signature: ";
print_r($child->getParameters());
echo "\nParent method signature: ";
print_r($parent->getParameters());
'
Length of output: 331
Script:
#!/bin/bash
# Let's try to find the parent class and inspect the method signatures using ast-grep and ripgrep
# First, find the parent class that AnnotatedMatcher extends
ast-grep --pattern 'class AnnotatedMatcher extends $_ {'
# Then find the matchesClass method in both files
rg "matchesClass.*\(.*\).*:.*bool" -A 5
Length of output: 5034
Script:
#!/bin/bash
# Let's find the base/abstract class that defines the interface for matchesClass
rg "abstract class.*Matcher" -A 5
# Also find any interface that might define this method
rg "interface.*Matcher" -A 10
Length of output: 855
Script:
#!/bin/bash
# Let's check the AbstractMatcher class for the matchesClass method definition
rg "matchesClass" src/AbstractMatcher.php -A 5
# Also check if there's any method signature in the interface
rg "matchesClass" src/MatcherInterface.php -A 5
Length of output: 281
13-13: Confirm the removal of the final keyword from AnnotatedMatcher class.
The AnnotatedMatcher class is no longer declared as final, allowing it to be subclassed. Ensure that this change is intentional and aligns with the design principles of your application, as it affects the extensibility of the class.
To verify if any subclasses of AnnotatedMatcher exist or if subclassing is intended, you can run the following script:
This script will help identify any classes that now extend AnnotatedMatcher after the removal of the final keyword.
✅ Verification successful
The removal of final keyword appears to be an intentional change
The git history shows this was an explicit change from final class AnnotatedMatcher to class AnnotatedMatcher. However, after analyzing the codebase:
- No classes currently extend
AnnotatedMatcher AnnotatedMatcheris only instantiated internally by theMatcherclass through itsannotatedWith()method- The class is used in a composition pattern within
LogicalAndMatcher,LogicalOrMatcher, andLogicalNotMatcherrather than inheritance
The removal of final appears to be a deliberate design choice to allow for potential extension, though it's not currently being utilized. This change aligns with the library's design principles as other matcher classes in the codebase are also non-final.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for subclasses of AnnotatedMatcher
# Find all PHP files that define classes extending AnnotatedMatcher
fd -e php --exec grep -EnH 'class\s+\w+\s+extends\s+AnnotatedMatcher' {}
Length of output: 76
Script:
#!/bin/bash
# Let's first check the git history to see if 'final' was indeed removed
git log -p src/AnnotatedMatcher.php | grep -A 5 -B 5 "class AnnotatedMatcher"
# Also check current usage and implementation of AnnotatedMatcher
rg "AnnotatedMatcher" -A 5 -B 5
# Check if there are any test files or documentation mentioning inheritance
fd -e php -e md | xargs grep -l "AnnotatedMatcher"
Length of output: 8155
src/Aspect.php (7)
73-73: Add Tests for Constructor Logic
Line 73, where $this->tmpDir is assigned, is not covered by tests.
Consider adding unit tests to cover scenarios where $tmpDir is provided as null, empty string, or a valid path, ensuring all branches of the constructor are tested.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: src/Aspect.php#L73
Added line #L73 was not covered by tests
107-110: Add Tests for scanDirectory() Method
Line 107, which calls the scanDirectory() method, is not covered by tests.
Ensure that unit tests are added for scanDirectory() to verify that it correctly scans directories and processes classes as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 107-107: src/Aspect.php#L107
Added line #L107 was not covered by tests
175-176: Confirm Method Type Annotation
In the processClass() method, lines 175-176 annotate $methods as an array of ReflectionMethod.
This annotation enhances code clarity and aids static analysis. No changes needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 175-176: src/Aspect.php#L175-L176
Added lines #L175 - L176 were not covered by tests
259-260: Add Tests for Exception in newInstanceWithPhp()
Line 260 throws an exception when $this->tmpDir is not set.
Add a unit test to verify that the exception is thrown when $tmpDir is null. This improves test coverage and ensures the exception handling works as intended.
45-46: 🛠️ Refactor suggestion
Add Property Type and Visibility for $tmpDir
The property $tmpDir lacks an explicit visibility modifier and type hint. Adding these improves code clarity and enables better static analysis.
Apply this diff to specify the visibility and type:
- private $tmpDir;
+ private ?string $tmpDir;Since the docblock specifies @var non-empty-string|null, consider enforcing this with a type assertion or stricter type hint if possible.
Likely invalid or redundant comment.
202-202: Verify Availability of method_intercept Function
Line 202 suppresses an undefined function warning for method_intercept.
Ensure that the method_intercept function is available when the rayaop extension is loaded. If it's conditionally available, the suppression is acceptable. Otherwise, consider adding a check or handling the absence appropriately.
Run the following script to verify the function's availability:
✅ Verification successful
method_intercept Function Suppression is Valid
The @phpstan-ignore-line suppression for method_intercept is correct. The codebase shows:
- The function is provided by the optional PECL extension
rayaopas documented in README.md - The code properly checks for extension availability with
extension_loaded('rayaop')in multiple places insrc/Aspect.php - A mock implementation exists in
src-mock/method_intercept.phpfor development/testing - The extension is not a required dependency in composer.json, confirming its optional nature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the function 'method_intercept' exists when 'rayaop' extension is loaded.
# Test: Check for the function definition. Expect: Function should be found if extension is loaded.
php -r "if (extension_loaded('rayaop') && function_exists('method_intercept')) { echo 'Function exists'; } else { echo 'Function does not exist'; }"
Length of output: 245
Script:
#!/bin/bash
# Let's search for any documentation, configuration, or extension requirements
rg -i "rayaop|method_intercept" --type md --type txt
# Search for any extension loading or function checks in PHP files
rg -i "extension_loaded\(.rayaop.\)|function_exists\(.method_intercept.\)" --type php
# Look for any composer requirements or extension dependencies
fd composer.json -x cat {}
Length of output: 4022
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-202: src/Aspect.php#L202
Added line #L202 was not covered by tests
59-60: 🛠️ Refactor suggestion
Specify Property Type and Visibility for $bound
Similarly, the $bound property should have explicit visibility and type annotations.
Apply this diff:
- private $bound = [];
+ /** @var ClassBindings */
+ private array $bound = [];This clarifies that $bound is of type ClassBindings.
Likely invalid or redundant comment.
Comment out the GotoStatement rule in the phpmd.xml configuration file. This change aims to avoid unnecessary rule checks related to the use of 'goto' statements in our codebase.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated several packages in the composer.lock file to their latest versions, including composer/pcre, composer/semver, and composer/xdebug-handler. These updates include compatibility adjustments and additional features.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
phpmd.xml (1)
Disabling UnusedPrivateField rule requires additional static analysis coverage
Based on the verification:
- PHPStan is configured at
level: maxwhich includes unused private field detection- However, PHPStan's unused property rules are less strict than PHPMD's as they allow properties used in serialization/framework contexts
- While this provides some coverage, it's recommended to:
- Document the specific reason for disabling PHPMD's UnusedPrivateField rule in phpmd.xml
- Consider enabling Psalm's unused property detection (
possiblyUnusedProperty) for additional coverageThe current setup might miss some cases of genuinely unused private fields that could contribute to code bloat.
🔗 Analysis chain
Line range hint
33-33: Verify the rationale for disabling the UnusedPrivateField rule.While disabling this rule might be justified if:
- Fields are used via reflection/serialization
- Fields are required by framework conventions
- Psalm already provides better detection for unused fields
Please ensure that disabling this rule doesn't lead to accumulation of dead code.
Let's verify if Psalm covers this case:
Let me gather more information about Psalm's configuration and usage in the project.
Let me check for any other static analysis configurations and actual usage of private fields in the codebase.
Let me check PHPStan's configuration since we found it exists, and look for its unused field detection capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Psalm's configuration for unused field detection echo "Checking Psalm configuration for unused field detection:" rg -i 'unused.*field|unused.*property' psalm.xml || echo "No unused field detection rules found in psalm.xml" # Look for potential unused private fields that might be missed echo -e "\nSearching for private fields:" ast-grep --pattern 'private $_ = $_' --lang phpLength of output: 409
Script:
#!/bin/bash # Check if Psalm is actually used in the project echo "Checking for Psalm configuration and usage:" fd psalm.xml # Check composer.json for Psalm dependency echo -e "\nChecking composer.json for Psalm:" rg -i "vimeo/psalm" composer.json # Look for other static analysis tools that might handle unused fields echo -e "\nChecking composer.json for other static analyzers:" cat composer.json | grep -i "require-dev" -A 10 # Check for any existing unused private fields echo -e "\nSearching for private fields in the codebase:" ast-grep --pattern 'private $_' --lang phpLength of output: 856
Script:
#!/bin/bash # Check for other static analysis configurations echo "Checking for static analysis configurations:" fd -e xml -e yaml -e yml | grep -i 'static\|analysis\|phpstan\|phan' # Look for actual private field usage patterns echo -e "\nChecking for private field usage patterns:" ast-grep --pattern 'class $_ { private $_; $$$ }' --lang php # Check PHPStan configuration if it exists echo -e "\nChecking PHPStan configuration:" fd phpstan.neon # Check actual PHPMD rules being used echo -e "\nChecking PHPMD rules in use:" cat phpmd.xmlLength of output: 2721
Script:
#!/bin/bash # Check PHPStan's configuration for unused field detection echo "Checking PHPStan configuration:" cat phpstan.neon # Check if PHPStan is actually installed and its version echo -e "\nChecking composer.json for PHPStan:" rg -i "phpstan" composer.json # Look for any vendor-bin PHPStan configuration echo -e "\nChecking for vendor-bin PHPStan config:" fd phpstan -p vendor-binLength of output: 729
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
phpmd.xml(1 hunks)src/CompilerInterface.php(1 hunks)src/ReflectiveMethodInvocation.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CompilerInterface.php
- src/ReflectiveMethodInvocation.php
🔇 Additional comments (1)
phpmd.xml (1)
24-24: Verify the rationale for disabling the GotoStatement rule.
While disabling this rule might be necessary, it's important to ensure:
- There's a legitimate use case for goto statements
- Psalm provides equivalent or better checks for this case
Let's check if there are any goto statements in the codebase and if Psalm covers this:
✅ Verification successful
Let me try another search strategy to verify the presence of goto statements and Psalm's coverage.
Disabling the GotoStatement rule is acceptable in this codebase
The verification reveals:
- Only one
gotostatement exists insrc/Compiler.php, used in a controlled manner for optimization in the compilation process - Psalm is configured with a strict error level (1) and covers the entire codebase, though it doesn't have explicit goto-specific rules
- The single usage is a common optimization pattern in compilers, where
gotocan improve performance in specific scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for goto statements in the codebase
echo "Searching for goto statements:"
rg '\bgoto\b' --type php
# Check if Psalm has any rules for goto
echo -e "\nChecking Psalm configuration for goto-related rules:"
rg -i 'goto' psalm.xml || echo "No goto-related rules found in psalm.xml"
Length of output: 409
Script:
#!/bin/bash
# Search for goto statements with more context
echo "Searching for goto statements with context:"
rg '\bgoto\b' --type php -C 2
# Check if psalm.xml exists and show its content
echo -e "\nChecking psalm configuration:"
if [ -f psalm.xml ]; then
cat psalm.xml
elif [ -f psalm.xml.dist ]; then
cat psalm.xml.dist
else
echo "No psalm.xml or psalm.xml.dist found"
fi
# Look for any PHP files that might contain goto statements
echo -e "\nListing all PHP files:"
fd -e php
Length of output: 6303
This commit introduces a new test method `testTmpDir` in `AspectTest.php`. It ensures that an `Aspect` object can be correctly instantiated with a path to a temporary directory. This enhances the robustness of our tests by covering more initialization cases.
The check for a null temporary directory in the newInstanceWithPhp method is redundant and therefore removed. This cleanup simplifies the code as the temporary directory handling is assumed to be managed elsewhere in the program.
These annotations were added to multiple private methods to exclude them from code coverage analysis. This helps ensure that only critical paths and publicly accessible methods are considered for testing coverage metrics.
The @var annotation for the $tmpDir property has been updated to `non-empty-string` to remove the possibility of it being null. This change ensures that the temporary directory for generated proxy classes must always be a non-empty string.
Deleted the GitHub Actions workflow for generating PHP class diagrams and the corresponding link in the README. This change streamlines the repository by removing an unnecessary automated job and simplifies the README content.
Refactored `addAccessModifiers` and `addMethodSignature` methods to return modified arrays instead of altering the input directly. This change improves method purity and readability by ensuring functions return new modified arrays instead of mutating their arguments.
Enhance code quality and static analysis by adding `@psalm-pure` annotations to several methods. This minimizes mutation side-effects and enforces immutability where applicable.
This change adds the @psalm-external-mutation-free annotation to the isBuiltinMethod method in AnyMatcher.php. This helps improve code analysis and ensures the method is correctly recognized as free from external mutations.
This adds the @psalm-pure annotation to the getUnionType method in the TypeString class. This ensures static analysis tools like Psalm can better assess the purity of the method, leading to improved code quality and correctness.
This changes adds @readonly annotations to the $matcherName and $matcher properties in the BuiltinMatcher class. Including these annotations helps clarify that these properties should not be modified after initialization, improving code clarity and maintainability.
The annotations `psalm-mutation-free` were unnecessary and were removed for better clarity and consistency in `MethodSignatureString.php`. The `psalm-pure` annotation was retained to accurately describe the functions' properties.
Added `readonly` annotation to `methodMatch` in `Bind.php` and `tmpDir` in `Aspect.php` to indicate these properties should not be modified after initialization. This change enhances code clarity and ensures better adherence to immutability constraints.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/Matcher/AnyMatcher.php (1)
65-68: LGTM: Correct usage of @psalm-external-mutation-free with potential enhancementThe
@psalm-external-mutation-freeannotation is correctly applied as the function only reads from static state without mutations.Consider adding
@psalm-readonlyto the static$builtinMethodsproperty for additional type safety:- /** @var string[] */ + /** + * @var string[] + * @psalm-readonly + */ private static $builtinMethods = [];src/Aspect.php (2)
280-282: Consider adding runtime type validationThe type cast
@psalm-var Tassumes thenewInstancemethod returns the correct type. Consider adding a runtime type check to ensure type safety.- /** @psalm-var T */ - return $weaver->newInstance($className, $args); + $instance = $weaver->newInstance($className, $args); + assert($instance instanceof $className); + /** @psalm-var T */ + return $instance;
299-304: Remove redundant code coverage annotationThe
@codeCoverageIgnoreannotation on thecontinuestatement is unnecessary as it's part of the normal control flow.foreach ($methods as $method) { if (! $matcher['methodMatcher']->matchesMethod($method, $matcher['methodMatcher']->getArguments())) { - continue; // @codeCoverageIgnore + continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/AopPostfixClassName.php(1 hunks)src/Aspect.php(7 hunks)src/Bind.php(3 hunks)src/BuiltinMatcher.php(1 hunks)src/Matcher/AnyMatcher.php(1 hunks)src/MethodSignatureString.php(4 hunks)src/TypeString.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/AopPostfixClassName.php
- src/Bind.php
- src/BuiltinMatcher.php
- src/MethodSignatureString.php
- src/TypeString.php
🔇 Additional comments (4)
src/Matcher/AnyMatcher.php (1)
59-62: LGTM: Correct usage of @psalm-pure annotation
The @psalm-pure annotation is appropriately used here as the function is deterministic, has no side effects, and doesn't access any global state.
src/Aspect.php (3)
27-39: Well-structured type definitions for Psalm!
The class-level PHPDoc annotations are comprehensive and well-organized, providing clear type definitions for complex data structures used throughout the class.
42-61: Property types are well-defined and immutable where appropriate!
The property type definitions are thorough and align well with the class-level type definitions. The @readonly annotation on $tmpDir ensures immutability, which is a good practice.
207-213: Consider using a more type-safe approach for method interception
The current implementation has multiple type suppression comments and relies on undefined functions. Consider creating an interface for the method interception functionality to improve type safety and testability.
Summary by CodeRabbit
Release Notes
New Features
AbstractMatcher,AnnotatedMatcher, andInvocation.@readonlyand@immutableannotations to several properties, ensuring immutability after initialization.AnnotatedMatcherandAspect.AnyMatcherto improve method matching behavior.Bug Fixes
AnnotatedMatcherandReflectiveMethodInvocation.Documentation
Tests
Aspectto validate temporary directory handling.