Implement RequireExtendsPropertiesClassReflectionExtension by staabm · Pull Request #2856 · phpstan/phpstan-src

staabm · 2024-01-05T15:39:45Z

No description provided.

src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php

ondrejmirtes

Please add some regression tests that use these tags to make sure this actually solves the problems:

staabm · 2024-01-05T17:16:29Z

(just pushed a intermediate state to allow me switching to a different computer over the weekend)

tests/PHPStan/Rules/Properties/data/require-implements.php

src/PhpDoc/ResolvedPhpDocBlock.php

src/Reflection/ClassReflection.php

src/Reflection/RequireExtension/RequireExtendsOrImplementsMethodsClassReflectionExtension.php

src/Reflection/ClassReflectionExtensionRegistry.php

src/PhpDoc/ResolvedPhpDocBlock.php

src/Reflection/ClassReflection.php

src/Reflection/RequireExtension/RequireExtendsMethodsClassReflectionExtension.php

ondrejmirtes · 2024-01-06T12:55:07Z

src/Reflection/RequireExtension/RequireExtendsMethodsClassReflectionExtension.php

Is this recursion guard tested?

I tried thinking of a way this could get recursive, but wasn't able to come up with a example.

do you think its impossible and the recursion-guard should be removed?

I'd expect something like this to end up in infinite recursion https://phpstan.org/r/aa768c71-4596-4edf-a900-60e4db58582b

thx for the inspiration. I tried similar things before and I can't force them to endless recurse.

I think this might not happen, because the new extension early exits for non-interfaces and the phpstan-require-extends requires a class as argument.

since I was still not able to force a recursion, I have removed the recursion guards for now

ondrejmirtes · 2024-01-06T12:56:09Z

src/Reflection/RequireExtension/RequireExtendsMethodsClassReflectionExtension.php

This is a point for the rules - @...-require-extends can only be above an interface.

so do you mean we should still try to infer the property across the type hierarchy, even though the require-extends is placed on a invalid location?

Sorry, I don't get it. This comment was just a reminder for you to check whether require-extends is always above an interface.

rules will be topic for a followup PR. thanks for noticing.

src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php

tests/PHPStan/Rules/Properties/data/require-implements.php

src/Reflection/ClassReflection.php

phpstan-bot · 2024-01-06T14:18:24Z

This pull request has been marked as ready for review.

ondrejmirtes · 2024-01-06T18:56:02Z

I merged part of this pull request: 53a61dc

You should be able to rebase this PR cleanly (maybe after you squash all commits).

The reason why I did this is that you can start working on the rules PR in parallel to this PR.

I want to keep 1.10.x releasable and I don't want to merge this PR and then wait several releases for the rules PR to be finished, because I see them as an atomic feature.

You should be able to work on rules in a separate branch/PR without any conflicts.

ondrejmirtes · 2024-01-10T13:09:26Z

Thank you!

ondrejmirtes · 2024-01-10T13:33:18Z

FYI this was a missing part: 9ac6fd1

ondrejmirtes · 2024-01-10T13:44:17Z

About generics - I'd like to support things like:

/**
 * @template T
 * @phpstan-require-extends Model<T>
 */

Which would inform about what T is in case the Model typehints it in the accessed property/called method. I think it might already work with the existing code.
Also when doing class Foo implements Bar extends Model, it should enforce that the same type in @implements Bar<X> is also used in @extends Model<X>.
Also if Model is generic but the user does not declare what T is in @phpstan-require-extends, PHPStan should complain.
It should for example also complain when Model has two @template but the number of type variables isn't 2.

For 3) and 4) there are already existing checks which we should just call.

Do you feel up to the task? If you start working on this and hit a roadblock don't worry, open a draft PR and we'll figure out why it doesn't work. Thanks!

staabm · 2024-01-10T14:14:59Z

FYI this was a missing part: 9ac6fd1

great catch, thank you.

staabm · 2024-01-11T14:00:47Z

@ondrejmirtes should we add the require-extends and require-implements types to $dependenciesReflections similar how its done for MixinTypes?

phpstan-src/src/Dependency/DependencyResolver.php

Lines 489 to 496 in 9c4508d

    
           foreach ($classReflection->getResolvedMixinTypes() as $mixinType) { 
        
           	foreach ($mixinType->getReferencedClasses() as $referencedClass) { 
        
           		if (!$this->reflectionProvider->hasClass($referencedClass)) { 
        
           			continue; 
        
           		} 
        
           		$dependenciesReflections[] = $this->reflectionProvider->getClass($referencedClass); 
        
           	} 
        
           }

ondrejmirtes · 2024-01-11T14:19:29Z

Yes, definitely! Ideally try to simulate in e2e-tests.yml a situation where the result cache gets stale. There arr several examples already.

staabm commented Jan 5, 2024

View reviewed changes

src/Reflection/RequireExtension/RequireExtendsPropertiesClassReflectionExtension.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jan 5, 2024

View reviewed changes

staabm force-pushed the require-extends branch from 7295e4a to e5c4136 Compare January 5, 2024 17:16

staabm commented Jan 5, 2024

View reviewed changes

tests/PHPStan/Rules/Properties/data/require-implements.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jan 5, 2024

View reviewed changes

src/PhpDoc/ResolvedPhpDocBlock.php Outdated Show resolved Hide resolved

src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved

src/Reflection/RequireExtension/RequireExtendsOrImplementsMethodsClassReflectionExtension.php Outdated Show resolved Hide resolved

staabm commented Jan 6, 2024

View reviewed changes

src/Reflection/ClassReflectionExtensionRegistry.php Outdated Show resolved Hide resolved

src/Reflection/ClassReflectionExtensionRegistry.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Jan 6, 2024

View reviewed changes

staabm force-pushed the require-extends branch from 8d326eb to 0b4bf73 Compare January 6, 2024 13:28

ondrejmirtes requested changes Jan 6, 2024

View reviewed changes

src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved

src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved

staabm force-pushed the require-extends branch from 18a9fe7 to b69346e Compare January 6, 2024 13:40

staabm marked this pull request as ready for review January 6, 2024 14:18

staabm force-pushed the require-extends branch from 0f88b00 to 029b523 Compare January 6, 2024 17:51

staabm force-pushed the require-extends branch from d02503b to fcfacb3 Compare January 6, 2024 19:13

Implement require-extends reflection plumbing

1b92082

staabm force-pushed the require-extends branch from fcfacb3 to 1b92082 Compare January 6, 2024 19:14

ondrejmirtes approved these changes Jan 10, 2024

View reviewed changes

ondrejmirtes merged commit 1740286 into phpstan:1.10.x Jan 10, 2024

staabm deleted the require-extends branch January 10, 2024 13:24

staabm mentioned this pull request Jan 11, 2024

require-extends implications on the interface vimeo/psalm#10538

Open

staabm mentioned this pull request Jan 11, 2024

Fix stale result cache for require-extends/require-implements #2866

Merged

Conversation

staabm commented Jan 5, 2024

Uh oh!

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

staabm commented Jan 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

staabm Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

staabm Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

staabm Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

staabm Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phpstan-bot commented Jan 6, 2024

Uh oh!

ondrejmirtes commented Jan 6, 2024

Uh oh!

ondrejmirtes commented Jan 10, 2024

Uh oh!

ondrejmirtes commented Jan 10, 2024

Uh oh!

ondrejmirtes commented Jan 10, 2024

Uh oh!

staabm commented Jan 10, 2024

Uh oh!

staabm commented Jan 11, 2024

Uh oh!

ondrejmirtes commented Jan 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm Jan 6, 2024 •

edited

Loading

staabm Jan 6, 2024 •

edited

Loading