Implement require-extends rules #2859

staabm · 2024-01-07T09:14:28Z

No description provided.

src/Rules/Classes/RequireExtendsRule.php

staabm · 2024-01-07T09:44:12Z

tests/PHPStan/Rules/PhpDoc/IncompatibleRequireExtendsTypeRuleTest.php

+				'Learn more at https://phpstan.org/user-guide/discovering-symbols',
+			],
+			[
+				'PHPDoc tag @require-extends cannot contain generic type.',


this error messge is a bit misleading. my idea was that in @phpstan-require-extends SomeClass we only name the class by its name, without generics params (generic classes can still be used at the using-class).

that way we don't need to validate the generics and therefore safe phpstan from validating them twice.

.. I think we just need a better error message.. wdyt?

ondrejmirtes · 2024-01-07T12:24:22Z

Before I look at the source code, here's what I'd expect these rules to do:

A rule to check the definition of `@phpstan-require-extends`

That it's only above an interface or a trait (it's fine to hook onto InClassNode and check that, we don't need to care about cases where it's defined for example above a function).
That it's only an ObjectType, to reject @phpstan-require-extends int for example.
That it's an existing and non-final class

A rule to check the definition of `@phpstan-require-implements`

That it's only above a trait - other uses don't make sense IMHO.
That it's only an ObjectType
That it's an existing interface

A rule to check a class that directly implements an interface or directly uses a trait with `@phpstan-require-extends`

This class needs to directly or indirectly extend the class from the tag

A rule to check a class that directly uses a trait with `@phpstan-require-implements`

This class needs to directly or indirectly implement the interface from the tag

Please confirm the rules do all of these things. I think 4 rule classes as outlined above would be ideal.

staabm · 2024-01-07T14:28:35Z

I have implemented 2 rules for require-extends, which I think fullfill your description. if you agree with them I would do the other 2 require-implements in a similar fashion.

one thing I noticed: since we use InClassNode for IncompatibleRequireExtendsTypeRule errors in phpdoc trait definitions will not show up. Should we work with ClassLike instead or maybe need a separate rules for checking the phpdoc on traits directly?

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

src/Rules/PhpDoc/IncompatibleRequireImplementsTypeRule.php

+
+	public function getNodeType(): string
+	{
+		return ClassLike::class;


Ask for InClassNode instead, it contains reflection already. You won't have to get the reflection from ReflectionProvider, and it works for anonymous classes too.

but it doesn't work for trait defintions. it would not error on

/** * @phpstan-require-implements SomeTrait */ trait InvalidTrait1 {}

Two separate rules then please.

For traits there's InTraitNode.

InTraitNode will only error for traits when a class exists which uses them. are you fine with that?

ondrejmirtes · 2024-01-09T12:55:51Z

src/Rules/PhpDoc/IncompatibleRequireImplementsTypeRule.php

+
+		$errors = [];
+		foreach ($implementsTags as $implementsTag) {
+			$type = $implementsTag->getType();


This whole foreach is too complicated. Just check that the type is ObjectType.

src/Rules/PhpDoc/IncompatibleRequireImplementsTypeRule.php

conf/config.level2.neon

src/Rules/Classes/RequireExtendsRule.php

src/Rules/Classes/RequireImplementsRule.php

src/Rules/PhpDoc/IncompatibleRequireExtendsTypeRule.php

phpstan-bot · 2024-01-09T15:15:32Z

This pull request has been marked as ready for review.

ondrejmirtes · 2024-01-09T16:04:34Z

src/Rules/PhpDoc/IncompatibleRequireExtendsTypeTraitRule.php

+
+	public function getNodeType(): string
+	{
+		return InTraitNode::class;


You're right, this one is better served by Node\Stmt\Trait_ - even when the trait is used multiple times, we just want to report the declaration once.

ondrejmirtes

Hey, I just pushed some cosmetic changes.

What still needs to be done here:

Support @psalm- prefixes in PhpDocNodeResolver along with some tests: https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-require-extends
There is a lot of literal duplication between RequireExtendsDefinitionTraitRule and RequireExtendsDefinitionClassRule. We usually solve this by introducing a "check" class that gets injected into rule classes to reuse the common code.

Also please note that after we merge the current two PRs and release them, I'd like you to work on properly introducing generics support for the new tags. But I want that to happen later and to not complicate the current PRs. Thanks.

ondrejmirtes · 2024-01-10T10:34:29Z

tests/PHPStan/Rules/Classes/RequireImplementsRuleTest.php

@@ -56,6 +56,18 @@ public function testRule(): void
 				'Trait IncompatibleRequireImplements\InvalidTrait4 requires using class to implement IncompatibleRequireImplements\SomeClass<T>, but IncompatibleRequireImplements\InvalidTraitUse4 does not.',
 				137,
 			],
+			[
+				'Trait IncompatibleRequireImplements\ValidPsalmTrait requires using class to implement IncompatibleRequireImplements\RequiredInterface2, but AnonymousClass4fea61018e98554a605d5c87523a32cc does not.',


this message is wrong, you need to use getDisplayName on ClassReflection

ondrejmirtes · 2024-01-10T11:31:02Z

Please use it consistently everywhere on ClassReflection instead of getName when building user-facing messages, thanks.

staabm · 2024-01-10T11:36:17Z

sorry missed one occurence

staabm · 2024-01-10T11:37:30Z

src/Rules/PhpDoc/RequireExtendsDefinitionClassRule.php

+		$classReflection = $node->getClassReflection();
+		$extendsTags = $classReflection->getRequireExtendsTags();
+
+		if (count($extendsTags) === 0) {


I think we should error when a class defines 2 or more @*-require-extends

Not error, but support and check all of them.

Oh sorry, I misunderstood you. Please add a test and fix the rule.

ondrejmirtes · 2024-01-10T13:04:29Z

src/Rules/PhpDoc/RequireExtendsCheck.php

@@ -30,6 +31,11 @@ public function __construct(
 	public function checkExtendsTags(Node $node, array $extendsTags): array
 	{
 		$errors = [];
+
+		if (count($extendsTags) > 1) {
+			$errors[] = RuleErrorBuilder::message(sprintf('PHPDoc tag @phpstan-require-extends can only be used once.'))->build();


Oh right, this makes sense because you can't have multiple parents :)

ondrejmirtes · 2024-01-10T13:07:42Z

Thank you!

staabm commented Jan 7, 2024

View reviewed changes

src/Rules/Classes/RequireExtendsRule.php Outdated Show resolved Hide resolved

staabm commented Jan 7, 2024

View reviewed changes

staabm force-pushed the require-rules branch 4 times, most recently from 9309ac3 to 3a87cc0 Compare January 7, 2024 14:16

staabm added 5 commits January 8, 2024 20:03

Implement IncompatibleRequireExtendsTypeRule

7c9fbe3

Implement RequireExtendsRule

ff66719

report final classes

65dde9f

php <8.1 compat

c6f272f

use ClassLike instead of InClassNode for phpdoc validation

b535c9b

staabm force-pushed the require-rules branch from 3a87cc0 to b535c9b Compare January 8, 2024 19:04

staabm added 4 commits January 8, 2024 21:09

implement require-implements rules

06d3343

Update config.level2.neon

a901c62

fix php 7.2 compat

fee7d1a

fix name collision

c44d540

ondrejmirtes requested changes Jan 9, 2024

View reviewed changes

staabm added 8 commits January 9, 2024 14:14

Rules that just have the tag should be in the rules section above.

e6bcbe7

Utilize ClassReflection->implementsInterface/isSubclassOf

2295b93

Use instanceof ObjectType

8d3d9d3

cover anonymous classes in tests

619790d

Work on InClassNode

682c4a0

Separate InClassNode and InTraitNode rules

bb3ee3e

Utilize ObjectType->getClassReflection()

7f7334e

fix php 7.x expectations

1374549

staabm marked this pull request as ready for review January 9, 2024 15:15

ondrejmirtes requested changes Jan 9, 2024

View reviewed changes

staabm and others added 5 commits January 9, 2024 17:33

error on trait phpdocs only once per declaration

a1ed5d8

Rename rule classes

131c5ca

Cosmetic change

df9cf28

Use Trait_

fc125fe

Improve error messages

fdab25b

ondrejmirtes requested changes Jan 10, 2024

View reviewed changes

staabm added 2 commits January 10, 2024 10:45

Support psalm prefix

ec3fc94

Refactoring: extract RequireExtendsCheck

ea57e39

ondrejmirtes requested changes Jan 10, 2024

View reviewed changes

use getDisplayName()

f7bda5c

clxmstaab force-pushed the require-rules branch from 916d7f6 to f7bda5c Compare January 10, 2024 11:34

staabm commented Jan 10, 2024

View reviewed changes

require-extends can only be used once

e31c497

clxmstaab force-pushed the require-rules branch from 9d4cdf6 to e31c497 Compare January 10, 2024 12:54

ondrejmirtes reviewed Jan 10, 2024

View reviewed changes

ondrejmirtes approved these changes Jan 10, 2024

View reviewed changes

ondrejmirtes merged commit 8900a43 into phpstan:1.10.x Jan 10, 2024
423 of 426 checks passed

staabm deleted the require-rules branch January 10, 2024 13:20

staabm mentioned this pull request Jan 11, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement require-extends rules #2859

Implement require-extends rules #2859

staabm commented Jan 7, 2024

staabm Jan 7, 2024 •

edited

ondrejmirtes commented Jan 7, 2024

staabm commented Jan 7, 2024

ondrejmirtes Jan 9, 2024

staabm Jan 9, 2024

ondrejmirtes Jan 9, 2024

ondrejmirtes Jan 9, 2024

staabm Jan 9, 2024

ondrejmirtes Jan 9, 2024

phpstan-bot commented Jan 9, 2024

ondrejmirtes Jan 9, 2024

ondrejmirtes left a comment

ondrejmirtes Jan 10, 2024

ondrejmirtes commented Jan 10, 2024

staabm commented Jan 10, 2024

staabm Jan 10, 2024 •

edited

ondrejmirtes Jan 10, 2024

ondrejmirtes Jan 10, 2024

ondrejmirtes Jan 10, 2024

ondrejmirtes commented Jan 10, 2024

Implement require-extends rules #2859

Implement require-extends rules #2859

Conversation

staabm commented Jan 7, 2024

staabm Jan 7, 2024 • edited

Choose a reason for hiding this comment

ondrejmirtes commented Jan 7, 2024

A rule to check the definition of @phpstan-require-extends

A rule to check the definition of @phpstan-require-implements

A rule to check a class that directly implements an interface or directly uses a trait with @phpstan-require-extends

A rule to check a class that directly uses a trait with @phpstan-require-implements

staabm commented Jan 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phpstan-bot commented Jan 9, 2024

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Jan 10, 2024

staabm commented Jan 10, 2024

staabm Jan 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Jan 10, 2024

staabm Jan 7, 2024 •

edited

A rule to check the definition of `@phpstan-require-extends`

A rule to check the definition of `@phpstan-require-implements`

A rule to check a class that directly implements an interface or directly uses a trait with `@phpstan-require-extends`

A rule to check a class that directly uses a trait with `@phpstan-require-implements`

staabm Jan 10, 2024 •

edited