Enhance code quality with Psalm annotations #294
Conversation
Add @psalm-pure and @psalm-immutable annotations to various classes to improve static analysis and enforce immutability. These changes ensure better code quality and maintainability by providing more explicit type information for tools like Psalm.
Cleaned up code by removing the default value for `Named::$value` and `Inject::$optional`, ensuring they must be explicitly set upon instantiation. The `psalm-pure` annotation was also removed from the `getStringName` function, reflecting the function's possible side effects or method calls. These changes improve code clarity and enforce proper initialization practices.
WalkthroughThis pull request introduces several changes across multiple classes in the Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@koriym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/di/MultiBinding/LazyInstance.php (1)
8-10: Improve type safety and method implementationConsider the following improvements:
- Add constraints to the generic type T
- Remove the unused $injector parameter or document why it's needed
/** - * @template T of mixed + * @template T of object * @psalm-immutable */ final class LazyInstance implements LazyInterface { // ... - public function __invoke(InjectorInterface $injector) + public function __invoke(InjectorInterface $injector): mixed { - unset($injector); - return $this->instance; }Also applies to: 29-34
src/di/Di/Set.php (1)
15-15: Consider strengthening immutability guaranteesWhile the
@psalm-immutableannotation is appropriate for the intended usage, the class could better enforce immutability at runtime by:
- Making the properties readonly (PHP 8.1+) or private with getters
- Adding
__setmagic method to prevent dynamic property additionsIf you're targeting PHP 8.1+, consider this improvement:
- /** @var ''|class-string<T> */ - public $interface; + /** @var ''|class-string<T> */ + public readonly string $interface; - /** @var string */ - public $name; + /** @var string */ + public readonly string $name;Alternative approach for PHP 7.4+:
- /** @var ''|class-string<T> */ - public $interface; + /** @var ''|class-string<T> */ + private string $interface; - /** @var string */ - public $name; + /** @var string */ + private string $name; + public function getInterface(): string + { + return $this->interface; + } + + public function getName(): string + { + return $this->name; + }src/di/Di/Inject.php (1)
16-16: Consider strengthening immutability guaranteesWhile the
@psalm-immutableannotation is appropriate, the immutability could be further enforced by:
- Making the
$optionalproperty readonly or private with a getter- Adding
@psalm-readonlyto the property- public $optional; + private readonly bool $optional;src/di/Exception/Unbound.php (1)
Line range hint
34-47: Consider adding @psalm-pure to buildMessage methodThe
buildMessagemethod appears to be pure as it:
- Only depends on its input parameters
- Uses pure operations (sprintf, array_pop, array_reverse, implode)
- Has no side effects
Consider adding the annotation:
/** * @param array<int, string> $msg + * @psalm-pure */ private function buildMessage(self $e, array $msg): string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/di/Argument.php(1 hunks)src/di/Di/Inject.php(2 hunks)src/di/Di/Named.php(1 hunks)src/di/Di/Set.php(1 hunks)src/di/Exception/Unbound.php(1 hunks)src/di/MultiBinding/LazyInstance.php(1 hunks)src/di/Name.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/di/Name.php
🔇 Additional comments (7)
src/di/Di/Named.php (2)
16-16: LGTM! The @psalm-immutable annotation is correctly applied.
The class meets all immutability requirements:
- Final class prevents inheritance
- Property is only set in constructor
- No methods modify state
22-22: Property initialization change strengthens immutability.
Removing the default value ensures the property must be set through the constructor, which is ideal for immutable objects.
Let's verify that this class is used correctly throughout the codebase:
✅ Verification successful
Let me gather more information about the Named class to verify its immutability.
Property initialization change is correctly implemented for immutability
The verification confirms that:
- The
Namedclass is marked as@psalm-immutable - The property is only set through the constructor
- All usages across the codebase properly provide the value parameter through attributes (#[Named]) or annotations (@nAmed)
- The class is final, preventing inheritance-based modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Named class instantiation patterns
# Expect: All instantiations should provide a value parameter
# Check for Named class instantiations
rg -t php "new Named\(" -A 1
# Check for attribute usage
rg -t php "#\[Named\(" -A 1
# Check for annotation usage
rg -t php "@Named\(" -A 1
Length of output: 2079
Script:
#!/bin/bash
# Check the Named class implementation
ast-grep --pattern 'class Named {
$$$
}'
# Check for any direct property assignments
rg -t php "->value =" -A 1
Length of output: 478
Script:
#!/bin/bash
# Check the Named class implementation
cat src/di/Di/Named.php
# Check for property assignments with escaped arrow
rg -t php '\->value =' -A 1
Length of output: 862
src/di/MultiBinding/LazyInstance.php (1)
11-11: LGTM: Appropriate use of @psalm-immutable
The class correctly qualifies as immutable since:
- All properties are private
- Properties are set only during construction
- No methods modify the internal state
src/di/Di/Set.php (1)
15-15: Verify usage of Set class across the codebase
Let's ensure this immutability annotation doesn't conflict with existing usage patterns.
✅ Verification successful
Let me gather more context about the Set class implementation and its usage.
Let me check the actual Set class implementation to make a final determination.
The @psalm-immutable annotation is correctly applied
The Set class is indeed immutable:
- Properties are only set in the constructor
- No setter methods exist
- Properties are only assigned once during instantiation
- The class is marked as final, preventing inheritance-based mutability
- All property assignments found in the codebase occur only in constructors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct property modifications of Set class instances
# which would violate immutability
# Look for direct property assignments
rg -t php '\$\w+->(?:interface|name)\s*=' --no-heading
# Look for Set class usage patterns
ast-grep --pattern 'new Set($$$)'
Length of output: 604
Script:
#!/bin/bash
# Check the Set class implementation and usage
ast-grep --pattern 'class Set {
$$$
}'
# Look for methods that might modify Set properties
rg -t php "function.*Set" -B 1 -A 5
# Check for Set class instantiation and method calls
rg -t php "Set::" -A 2
Length of output: 12639
Script:
#!/bin/bash
# Get the Set class implementation
cat src/di/Di/Set.php
# Check for any methods that might modify Set properties after construction
ast-grep --pattern 'class Set {
$$$
public function set$_($$$) {
$$$
}
$$$
}'
Length of output: 790
src/di/Di/Inject.php (1)
26-26: 🛠️ Refactor suggestion
Add type declaration for better type safety
The $optional property lacks a type declaration. Consider adding one to improve type safety and static analysis:
- public $optional;
+ public bool $optional;Let's check for any potential backwards compatibility issues with this property:
src/di/Exception/Unbound.php (1)
51-53: LGTM! The @psalm-pure annotation is correctly applied.
The getMainMessage method meets all criteria for a pure function:
- Output solely depends on input parameter
- Uses only pure operations (sprintf, get_class)
- No side effects or state mutations
Let's verify consistent annotation usage across similar string formatting methods:
✅ Verification successful
@psalm-pure annotation usage is consistent and correctly applied
The verification shows that @psalm-pure is consistently used across similar pure methods in the codebase:
parseName()in Name.php - pure string parsing functiongetType()in Argument.php - pure type resolution functiongetMainMessage()in Exception/Unbound.php - pure string formatting function
All annotated methods are side-effect free and their output solely depends on their input parameters, confirming correct annotation usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar string formatting methods that might benefit from @psalm-pure
# Look for methods using sprintf/get_class without side effects
ast-grep --pattern 'private function $METHOD($_) {
return sprintf($$$);
}'
# Also check existing psalm-pure annotations for consistency
rg -l "@psalm-pure"
Length of output: 163
Script:
#!/bin/bash
# Let's examine the content of these files to verify consistent annotation usage
# and check for any similar methods that could benefit from @psalm-pure
# Check Name.php
echo "=== Name.php ==="
rg "psalm-pure|private function" src/di/Name.php -B 1 -A 3
echo -e "\n=== Argument.php ==="
rg "psalm-pure|private function" src/di/Argument.php -B 1 -A 3
echo -e "\n=== Exception/Unbound.php ==="
rg "psalm-pure|private function" src/di/Exception/Unbound.php -B 1 -A 3
Length of output: 1256
src/di/Argument.php (1)
174-176: LGTM! The @psalm-pure annotation is correctly applied.
The annotation accurately reflects that getType is a pure function - its output depends solely on its input parameter, it has no side effects, and doesn't mutate any state.
Let's verify that the method maintains purity by checking for any state mutations or side effects in the codebase:
✅ Verification successful
The @psalm-pure annotation is correctly applied to the getType method
The implementation confirms that getType is pure:
- Takes a
ReflectionParameteras input - Only performs read-only operations (
getType(),getName()) - Uses pure operations (
instanceof,in_array) - Returns a string based solely on input
- No state mutations or side effects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getType maintains purity by checking:
# 1. No property mutations within the method
# 2. No method calls that could have side effects
# Search for any property assignments within getType
ast-grep --pattern 'function getType($_) {
$$$
$this->$_ = $$$
$$$
}'
# Search for method calls within getType that might have side effects
ast-grep --pattern 'function getType($_) {
$$$
$this->$_($$$)
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Let's try a different approach to verify the purity of getType method
# 1. First, let's see the actual implementation of getType
rg -U "function getType.*?\{.*?\}" -A 20 --multiline src/di/Argument.php
# 2. Let's also check if this method is called anywhere else in the codebase
# to ensure it's not part of any side-effect chain
rg "->getType\("
Length of output: 498
Script:
#!/bin/bash
# Let's try again with proper escaping and simpler patterns
# 1. Get the implementation of getType method
rg "function getType" -A 20 src/di/Argument.php
# 2. Check for usage of getType method
rg "getType\("
Length of output: 682
Corrected the spelling of "LazyInteterface" to "LazyInterface" in all relevant files to ensure consistent naming and prevent potential interface reference errors. This change impacts class implementations and annotations that depended on the misspelled interface name.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/di/MultiBinding/Map.php (1)
33-33: LGTM! Consider additional type safety improvementsThe constructor parameter annotation correctly matches the property type. For enhanced type safety, consider:
- Adding a psalm-assert annotation to validate the array values at runtime
- Using psalm-param to provide more specific type constraints
Add the following annotation for better type safety:
/** * @param array<array-key, LazyInterface> $lazies + * @psalm-assert array<array-key, LazyInterface> $lazies */src/di/MultiBinder.php (1)
Line range hint
13-99: Consider adding Psalm annotations to enhance code qualitySince this PR aims to enhance code quality with Psalm annotations, consider adding relevant annotations to this class:
@psalm-immutablefor the class if it's intended to be immutable@psalm-purefor methods that have no side effects@psalm-paramand@psalm-returnannotations for better type informationExample additions:
+ /** @psalm-immutable */ final class MultiBinder { // ... + /** @psalm-pure */ public static function newInstance(AbstractModule $module, string $interface): self { return new self($module, $interface); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/di/MultiBinder.php(2 hunks)src/di/MultiBinding/LazyInstance.php(1 hunks)src/di/MultiBinding/LazyInterface.php(1 hunks)src/di/MultiBinding/LazyProvider.php(1 hunks)src/di/MultiBinding/LazyTo.php(1 hunks)src/di/MultiBinding/Map.php(1 hunks)src/di/MultiBinding/MultiBindings.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/di/MultiBinding/LazyInterface.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/di/MultiBinding/LazyInstance.php
🔇 Additional comments (9)
src/di/MultiBinding/MultiBindings.php (2)
12-12: LGTM! PHPDoc type correction looks good.
The fix for the interface name from LazyInteterface to LazyInterface in the generic type definition is correct.
12-12: Verify consistent interface naming across the codebase.
Let's ensure this interface name correction is consistent across all files and that the interface exists.
✅ Verification successful
Interface naming is consistent and properly used across the codebase
The verification shows that:
- The
LazyInterfaceis properly defined in./src/di/MultiBinding/LazyInterface.php - It's consistently implemented by three classes:
LazyTo,LazyProvider, andLazyInstance - The interface is correctly imported where needed
- No instances of misspelled "LazyInteterface" were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface naming consistency and existence
# Check for any remaining instances of the old interface name
echo "Checking for any remaining instances of LazyInteterface..."
rg "LazyInteterface"
# Verify the existence of LazyInterface
echo "Verifying LazyInterface existence..."
fd -e php -x grep -l "interface LazyInterface"
# Check for any usage of the interface
echo "Checking interface usage..."
rg "implements.*LazyInterface|use.*LazyInterface"
Length of output: 747
src/di/MultiBinding/LazyTo.php (2)
Line range hint 1-31: Implementation looks good!
The class implementation is well-structured with:
- Proper type safety through PHP DocBlocks and strict types
- Correct usage of templating with
@template T of object - Clean lazy loading pattern implementation
12-12:
Verify LazyInterface implementation
The interface name correction from LazyInteterface to LazyInterface fixes a critical typo. However, I notice that LazyInterface is not imported in the use statements.
Let's verify the interface existence and its proper import:
Please add the missing import statement:
namespace Ray\Di\MultiBinding;
use Ray\Di\InjectorInterface;
+ use Ray\Di\LazyInterface;src/di/MultiBinding/LazyProvider.php (2)
Line range hint 1-31: LGTM! Well-structured class implementation
The class follows PHP best practices with:
- Strict typing enabled
- Proper PHPDoc template annotations for type safety
- Clear method signatures and return type documentation
- Final class declaration preventing inheritance where not needed
13-13: Verify LazyInterface implementation
The interface name correction from LazyInteterface to LazyInterface is appropriate, but let's verify the interface exists and is properly imported.
✅ Verification successful
Interface implementation is correct and properly used
The verification confirms that:
- Both
LazyInterfaceandLazyProviderare in the same namespaceRay\Di\MultiBinding LazyInterfaceexists and is properly defined insrc/di/MultiBinding/LazyInterface.php- The interface is consistently used across the codebase by multiple implementations (
LazyInstance,LazyTo,LazyProvider) - The interface is properly referenced in the dependency injection system (
MultiBinder)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LazyInterface exists and is properly defined
# Expected: Find the LazyInterface definition and its import statements
# Search for LazyInterface definition
echo "Searching for LazyInterface definition..."
rg --type php "interface LazyInterface"
# Search for LazyInterface imports
echo "Searching for LazyInterface imports in LazyProvider..."
rg --type php "use.*LazyInterface" "src/di/MultiBinding/LazyProvider.php"
Length of output: 389
Script:
#!/bin/bash
# Let's verify the namespace and location of both files to ensure they're in the same namespace
echo "Checking namespace of LazyInterface..."
rg --type php "^namespace" "src/di/MultiBinding/LazyInterface.php"
echo "Checking namespace of LazyProvider..."
rg --type php "^namespace" "src/di/MultiBinding/LazyProvider.php"
# Let's also check if there are any other uses of LazyInterface in the codebase
echo "Checking other uses of LazyInterface..."
rg --type php "LazyInterface" -C 2
Length of output: 2997
src/di/MultiBinding/Map.php (1)
26-26: Verify LazyInterface existence and usage
The type annotation correction from LazyInteterface to LazyInterface appears to be fixing a typo.
Let's verify the interface existence and its usage:
✅ Verification successful
Type annotation correction is valid and properly referenced
The correction from LazyInteterface to LazyInterface is fixing a typo in the type annotation. The LazyInterface is properly defined in src/di/MultiBinding/LazyInterface.php and is correctly used within the same namespace (Ray\Di\MultiBinding). The interface matches the usage pattern in Map.php as it defines the __invoke method that's used in the offsetGet and getIterator methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LazyInterface existence and its usage across the codebase
# Check if LazyInterface exists
ast-grep --pattern 'interface LazyInterface'
# Check for any remaining instances of the typo
rg "LazyInteterface"
Length of output: 67
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# Search for LazyInterface in use or import statements
rg "LazyInterface" -l
# Search for files containing 'Lazy' to find potential interface definition
fd -e php -x grep -l "Lazy" {} \;
# Check imports in the specific file
cat "src/di/MultiBinding/Map.php"
Length of output: 2896
Script:
#!/bin/bash
# Let's verify the LazyInterface definition and its relationship with Map.php
cat "src/di/MultiBinding/LazyInterface.php"
Length of output: 263
src/di/MultiBinder.php (2)
83-83: LGTM! Type safety improvement
The method signature has been correctly updated to use LazyInterface, improving type safety while maintaining the existing functionality.
8-8: Verify consistent interface naming across the codebase
The import statement has been corrected from LazyInteterface to LazyInterface. Let's ensure this correction is consistently applied throughout the codebase.
✅ Verification successful
Let me verify the interface usage more thoroughly with additional checks.
Interface naming is consistent across the codebase
The verification shows that:
- The interface is correctly defined as
LazyInterfaceinsrc/di/MultiBinding/LazyInterface.php - All implementations (
LazyProvider,LazyInstance,LazyTo) correctly useLazyInterface - All type hints and docblocks consistently use the correct spelling
- No instances of the misspelled
LazyInteterfaceremain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no instances of the misspelled interface remain
# and all implementations use the correct interface name
# Check for any remaining misspellings
echo "Checking for any remaining misspellings of LazyInteterface:"
rg -l "LazyInteterface"
# Verify implementations
echo -e "\nVerifying interface implementations:"
ast-grep --pattern 'class $_ implements $$$LazyInterface$$$'
Length of output: 304
Script:
#!/bin/bash
# Search for any usage of LazyInterface in the codebase
echo "Checking all occurrences of LazyInterface:"
rg "LazyInterface"
# Check the actual interface definition
echo -e "\nChecking interface definition:"
ast-grep --pattern 'interface LazyInterface'
# Check for any class using LazyInterface as type hint
echo -e "\nChecking type hints:"
ast-grep --pattern '$_($_ LazyInterface $_)'
Length of output: 1083
|
@coderabbitai Approved? |
|
@koriym: After reviewing the changes, the updates enhance the code quality with Psalm annotations and correct typos in the code. Approved. ✅ Actions performedComments resolved. |
Summary by CodeRabbit