-
Notifications
You must be signed in to change notification settings - Fork 574
Introduce ClosureType::isStaticClosure()
#5699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.1.x
Are you sure you want to change the base?
Changes from 10 commits
e88ea63
8563877
ecf42bb
2283773
9913df4
fcff80d
0d6bbb7
69c0da9
afda50d
9486a3f
c5c6d02
94cce39
1dd9c0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,8 @@ class ClosureType implements TypeWithClassName, CallableParametersAcceptor | |
|
|
||
| private Assertions $assertions; | ||
|
|
||
| private TrinaryLogic $isStatic; | ||
|
|
||
| /** | ||
| * @api | ||
| * @param list<ParameterReflection>|null $parameters | ||
|
|
@@ -112,6 +114,7 @@ public function __construct( | |
| ?TrinaryLogic $acceptsNamedArguments = null, | ||
| ?TrinaryLogic $mustUseReturnValue = null, | ||
| ?Assertions $assertions = null, | ||
| ?TrinaryLogic $isStatic = null, | ||
| ) | ||
| { | ||
| if ($acceptsNamedArguments === null) { | ||
|
|
@@ -132,6 +135,7 @@ public function __construct( | |
| $this->callSiteVarianceMap = $callSiteVarianceMap ?? TemplateTypeVarianceMap::createEmpty(); | ||
| $this->impurePoints = $impurePoints ?? [new SimpleImpurePoint('functionCall', 'call to an unknown Closure', false)]; | ||
| $this->assertions = $assertions ?? Assertions::createEmpty(); | ||
| $this->isStatic = $isStatic ?? TrinaryLogic::createMaybe(); | ||
| } | ||
|
|
||
| public function getAsserts(): Assertions | ||
|
|
@@ -268,7 +272,8 @@ public function equals(Type $type): bool | |
| } | ||
|
|
||
| return $this->describe(VerbosityLevel::precise()) === $type->describe(VerbosityLevel::precise()) | ||
| && $this->isPure()->equals($type->isPure()); | ||
| && $this->isPure()->equals($type->isPure()) | ||
| && $this->isStatic->equals($type->isStatic); | ||
| } | ||
|
|
||
| public function describe(VerbosityLevel $level): string | ||
|
|
@@ -277,7 +282,9 @@ public function describe(VerbosityLevel $level): string | |
| static fn (): string => 'Closure', | ||
| function (): string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a private methode could be created to refactor the value and precise level |
||
| if ($this->isCommonCallable) { | ||
| return $this->isPure()->yes() ? 'pure-Closure' : 'Closure'; | ||
| $prefix = $this->isStatic->yes() ? 'static-' : ''; | ||
| $name = $this->isPure()->yes() ? 'pure-Closure' : 'Closure'; | ||
| return $prefix . $name; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also needs to be in toPhpDocNode which needs a test in TypeToPhpDocNodeTest. Also the Also all
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All tasks complete. Here's a summary of what was done to address the review: Changes made (2 commits pushed):Commit 1: Core code changes
Commit 2: Tests
All 12222 tests pass and |
||
| } | ||
|
|
||
| $printer = new Printer(); | ||
|
|
@@ -496,6 +503,11 @@ public function mustUseReturnValue(): TrinaryLogic | |
| return $this->mustUseReturnValue; | ||
| } | ||
|
|
||
| public function isStaticClosure(): TrinaryLogic | ||
| { | ||
| return $this->isStatic; | ||
| } | ||
|
|
||
| public function isCloneable(): TrinaryLogic | ||
| { | ||
| return TrinaryLogic::createYes(); | ||
|
|
@@ -709,6 +721,7 @@ public function traverse(callable $cb): Type | |
| $this->acceptsNamedArguments, | ||
| $this->mustUseReturnValue, | ||
| $this->assertions, | ||
| $this->isStatic, | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -761,6 +774,7 @@ public function traverseSimultaneously(Type $right, callable $cb): Type | |
| $this->acceptsNamedArguments, | ||
| $this->mustUseReturnValue, | ||
| $this->assertions, | ||
| $this->isStatic, | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| <?php | ||
|
|
||
| namespace ClosureStaticType; | ||
|
|
||
| use Closure; | ||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| final class Foo | ||
| { | ||
|
|
||
| public function doFoo(): void | ||
| { | ||
| $static = static function (): void {}; | ||
| assertType('Closure(): void', $static); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this doesn't have |
||
|
|
||
| $nonStatic = function (): void {}; | ||
| assertType('Closure(): void', $nonStatic); | ||
|
|
||
| $staticArrow = static fn (): int => 1; | ||
| assertType('Closure(): 1', $staticArrow); | ||
|
|
||
| $nonStaticArrow = fn (): int => 1; | ||
| assertType('Closure(): 1', $nonStaticArrow); | ||
| } | ||
|
|
||
| public function doBindTo(): void | ||
| { | ||
| $static = static function (): void {}; | ||
| assertType('Closure(): void', $static->bindTo($this)); | ||
|
|
||
| $nonStatic = function (): void {}; | ||
| assertType('Closure(): void', $nonStatic->bindTo($this)); | ||
| } | ||
|
|
||
| public function doBind(): void | ||
| { | ||
| $static = static function (): void {}; | ||
| assertType('Closure(): void', Closure::bind($static, $this)); | ||
|
|
||
| $nonStatic = function (): void {}; | ||
| assertType('Closure(): void', Closure::bind($nonStatic, $this)); | ||
| } | ||
|
|
||
| /** | ||
| * @param Closure(): void $unknownClosure | ||
| */ | ||
| public function doUnknown(Closure $unknownClosure): void | ||
| { | ||
| assertType('Closure(): void', $unknownClosure->bindTo($this)); | ||
| assertType('Closure(): void', Closure::bind($unknownClosure, $this)); | ||
| } | ||
|
|
||
| public function doFromCallable(): void | ||
| { | ||
| $fn = Closure::fromCallable(static function (): void {}); | ||
| assertType('Closure(): void', $fn->bindTo($this)); | ||
|
|
||
| $fn2 = Closure::fromCallable(function (): void {}); | ||
| assertType('Closure(): void', $fn2->bindTo($this)); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this addition.
I think its common to pass a static closure into a function expecting a non-static one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch is clean and up to date. No code changes are needed for this review comment. Here's the analysis:
The
isStaticcheck inequals()is correct and consistent with the existing pattern. Theequals()method checks type identity (are these the exact same type?), not assignability (can one be used where the other is expected?). TheisPure()check — already present before this PR — follows the exact same pattern: two closures with different purity are not equal types, even though a pure closure can be passed where an impure one is expected.The reviewer's concern about assignability is already handled by
isSuperTypeOf(). TheCallableTypeHelper::isParametersAcceptorSuperTypeOf()logic and theisSuperTypeOftests show:maybe-staticclosure (the default for PHPDocClosuretypes) accepts both static and non-static closuresnon-staticclosure types reject static closures, which is correct: a function that explicitly requires a non-static closure may intend tobindTo()it, andbindTo()returnsnullfor static closuresIn practice, function parameters typed via PHPDoc as
Closure(): voidare alwaysmaybe-static, so passing a static closure is accepted. The strictisSuperTypeOf()check only triggers between two code-inferred closures with known static-ness.