Prevent reporting `property.inInterface` error when PHP version is `8.4` or later #3656

jakubtobiasz · 2024-11-23T07:33:04Z

After reading Ondrej's tweet and playing with PHPStan 2.0 on PHP 8.4, I've found out it reports the property.inInterface error and it is non-ignorable. My first thought was to allow ignoring this error, but I've ended up not reporting this issue at all once the PHP version is 8.4 or later.

staabm · 2024-11-23T08:21:52Z

You need a bit more research on the topic, e.g. it seems the properties on interfaces are only allowed when public

https://3v4l.org/TBaXF

and you need tests for the new cases

staabm · 2024-11-23T08:23:15Z

src/Rules/Properties/PropertiesInInterfaceRule.php


 /**
 * @implements Rule<ClassPropertyNode>
 */
 final class PropertiesInInterfaceRule implements Rule
 {

+	private const PHP_8_4_VERSION_ID = 80400;


PhpVersion id-checks need to go into a new method of PhpVersion class

jakubtobiasz · 2024-11-23T08:39:25Z

Hi @staabm,

Thanks for leaving your review 🤠.

You need a bit more research on the topic, e.g. it seems the properties on interfaces are only allowed when public

Right, I knew about it but missed it here.

So in case of protected/private properties:

a) We should just report Interfaces may not include properties.
b) We should report Interfaces may include only public properties. and leave the property.inInterface
c) We should create a separate class with message Interfaces may include only public properties. and id property.nonPublicInInterface
d) anything else

I bet the option c), but I prefer to ask as it's my first time contrib to phpstan :).

At the same time, I guess we should leave this implementation as-is (so property.inInterface is never reported on PHP 8.4 and above) and just create a new rule as mentioned in c).

ondrejmirtes · 2024-11-23T08:44:59Z

Please wait for my review before further steps 😊

ondrejmirtes

Please note that in PHP 8.4 not all properties in interface are allowed. Please read the Interfaces section in https://wiki.php.net/rfc/property-hooks.

Only public properties can be in an interface
Only properties with get or set or both hooks can be in an interface.
Property hook in an interface cannot have a body.

The rule should account for all of this.

Please note that the original error message "Interfaces may not include properties." is not exactly correct on PHP 8.4 so all of these situations should have new messages.

Thanks.

src/Rules/Properties/PropertiesInInterfaceRule.php

jakubtobiasz · 2024-11-23T15:42:32Z

tests/PHPStan/Rules/Properties/NonEmptyPropertyHookRuleTest.php

+		if (PHP_VERSION_ID < 80400) {
+			$this->markTestSkipped('This test requires at least PHP 8.4.');
+		}


I'm not 100% sure about this approach. In theory it is valid; however, maybe we should mock PhpVersion or set PHP_VERSION_ID by hand.

jakubtobiasz · 2024-11-23T16:24:39Z

@ondrejmirtes I've implemented all suggestions, and I'm ready for the next ones (if needed 😅).

ondrejmirtes

We don't need to do this in multiple rules. It kind of makes it hard to see what situations are covered or not. A decision tree (series of if-else conditions) in a single rule would be much more readable and understandable. So please do this in a single rule. The right moment to split a rule might come later but not now.
There are cases not covered by your added code. Properties can be abstract even in classes, meaning at least one of their hooks must not have a body.

jakubtobiasz · 2024-11-23T20:50:53Z

I was focused on interfaces and fixing the issue with 2.0.x not working with PHP 8.4 while creating this PR. Of course, if you decide this PR should cover property hook as a whole, just let me know, and I'll adjust the PR.

However, from my POV, the first and second bullets are conflicted. If we want to add handling abstract classes with the logic inside the NonEmptyPropertyHookRule, I'd recommend leaving it as a separate class (as PropertyInInterface doesn't sound good in case we add abstract classes there).

In my humble opinion, we should stay with split classes as they follow SRP and new features seem to not fit the Property in Interface as we are in the property hook context.

So my suggestion is to leave the separation and cover abstract classes as a separate PR, as in such a case this PR could be merged as a bugfix for 2.0, which would be nice (at least from my perspective) once I want to use 8.4 as soon as possible.

Of course, I'm waiting for your decision 🖖🏻.

ondrejmirtes · 2024-11-23T21:02:07Z

I don't want this PR to completely implement property hooks, that's a much bigger project. I just want it to check what I said I want it to check. So to report a wrong declaration that's detectable just by looking at it.

I don't even want this rule to check missing implementation of an abstract hook from a parent. That's a job for a different rule.

Here's a complete list of things that should be reported by this (single) rule class after this PR:

On PHP before 8.4: interfaces can't have properties
On PHP before 8.4: properties can't have hooks
(All following items are on 8.4+). Properties in interfaces must have at least one hook, and all hooks must not have a body.
Non-abstract properties in classes must only have hooks with bodies.
Abstract property can only be in an abstract class.
Abstract property must have at least one hook without a body.
Property in an interface must be public.

I'd understand if you decide not to work on this. In that case you can close this PR.

But in my eyes this is the minimal work needed for the rule to make sense if it's supposed to understand "some properties can now be in interface" which is what you want to achieve.

jakubtobiasz · 2024-11-23T21:38:26Z

I'd understand if you decide not to work on this. In that case you can close this PR.

Nah, that's not the case. I'll do my best to deliver this :).

I'll use the list you provided while reimplementing it. I guess around the end of the next week.

Just last thing — where should I put the code touching abstract classes?

jakubtobiasz · 2024-11-26T06:47:44Z

On PHP before 8.4: interfaces can't have properties
On PHP before 8.4: properties can't have hooks
Properties in interfaces must have at least one hook, and all hooks must not have a body.
Property in an interface must be public.

Are covered by the latest changes :). The rest will be covered later this week.

By the way, I'm bumping the previous questions 😅

Just last thing — where should I put the code touching abstract classes?

I don't see any existing rule matching the new rules about property hooks, so it seems natural to create PropertiesInClassRule, but I prefer to make sure before I dive into it 😅.

ondrejmirtes · 2024-11-26T13:34:52Z

Just last thing — where should I put the code touching abstract classes?

Right now you're in a rule about ClassPropertyNode. You can get $node->getClassReflection() and ask it whether it's abstract.

ondrejmirtes · 2024-11-26T13:37:25Z

But yeah, these items:

Non-abstract properties in classes must only have hooks with bodies.
Abstract property can only be in an abstract class.
Abstract property must have at least one hook without a body.

Could be added to a separate rule, like PropertyInClassRule. (I dislike the plural tbh.)

ondrejmirtes · 2024-11-27T14:24:47Z

Do you have any idea how to "omit" it or what I can do with it?

The solution is to skip the test based on PHP_VERSION_ID, or change the expected errors based on PHP_VERSION_ID. Thanks.

jakubtobiasz · 2024-11-27T16:39:54Z

The solution is to skip the test based on PHP_VERSION_ID, or change the expected errors based on PHP_VERSION_ID. Thanks.

But the test is also failing on Tests / Tests (8.4, ubuntu-latest) (pull_request) & Tests / Tests (8.4, windows-latest) (pull_request) for the same reason 🤔. So it seems it's not only a problem of a test being executed on the wrong version.

jakubtobiasz · 2024-11-27T17:19:00Z

From what I see in the \PHPStan\BetterReflection\Reflection\ReflectionProperty::computeImmediateVirtual should be a check whether the property:

is in an interface
is in an abstract class and is marked as abstract

P.S. I let myself resolve your comments; however, if your flow assumes you are resolving them after checking if they are fixed, let me know :).
P.S2 I've double checked, and everything seems to be good on my site 🙈. PHP 7.4 tests are failing due to being not able to parse the file. 8.x are failing due to the reason above 👆🏻.
P.S.3 8.3 is throwing another magic error; no idea why, but it also doesn't seem to be related to my changes 🙈.

ondrejmirtes · 2024-11-27T21:01:30Z

I'll take it from here, thanks!

…terfaces

…asses

ondrejmirtes · 2024-11-28T21:59:40Z

Feel free to check out the commits I added. There was one bug in logic (7eaed3a), the rest is basically my preferences how to organize code.

ondrejmirtes · 2024-11-28T22:00:33Z

Thank you.

jakubtobiasz · 2024-11-29T09:58:37Z

Thanks for the cooperation 🙌🏼

ondrejmirtes · 2024-12-29T21:54:53Z

Please see:

jakubtobiasz force-pushed the allow-properties-in-interface-on-php-84 branch 3 times, most recently from 853a27b to 5196b92 Compare November 23, 2024 07:57

staabm reviewed Nov 23, 2024

View reviewed changes

ondrejmirtes requested changes Nov 23, 2024

View reviewed changes

src/Rules/Properties/PropertiesInInterfaceRule.php Outdated Show resolved Hide resolved

jakubtobiasz commented Nov 23, 2024

View reviewed changes

jakubtobiasz force-pushed the allow-properties-in-interface-on-php-84 branch 7 times, most recently from a4e23d6 to 09f4b1a Compare November 23, 2024 16:17

jakubtobiasz requested a review from ondrejmirtes November 23, 2024 16:24

ondrejmirtes requested changes Nov 23, 2024

View reviewed changes

jakubtobiasz force-pushed the allow-properties-in-interface-on-php-84 branch 2 times, most recently from ddeb66e to 86ff593 Compare November 26, 2024 06:44

jakubtobiasz changed the title ~~Prevent reporting property.inInterface error when PHP version is 8.4 or alter~~ Prevent reporting property.inInterface error when PHP version is 8.4 or later Nov 26, 2024

jakubtobiasz force-pushed the allow-properties-in-interface-on-php-84 branch 2 times, most recently from f507f6a to 3a0b632 Compare November 27, 2024 08:54

ondrejmirtes mentioned this pull request Nov 27, 2024

PHP 8.4 Interfaces may not include properties phpstan/phpstan#12133

Closed

jakubtobiasz force-pushed the allow-properties-in-interface-on-php-84 branch 5 times, most recently from ecdd7c8 to 9b8bbc5 Compare November 27, 2024 17:56

ondrejmirtes force-pushed the allow-properties-in-interface-on-php-84 branch from 9b8bbc5 to 64d922a Compare November 28, 2024 20:48

jakubtobiasz and others added 5 commits November 28, 2024 22:13

Implement a logic handling Property Hooks on PHP 8.4 or above in in…

4ac1fc7

…terfaces

Implement a logic handling Property Hooks on PHP 8.4 or above in cl…

0214073

…asses

Improve readability and provide minor fixes

03e1bef

Skip tests on right versions

81177d1

Fix tests

cb53a79

ondrejmirtes force-pushed the allow-properties-in-interface-on-php-84 branch from 64d922a to cb53a79 Compare November 28, 2024 21:19

ondrejmirtes added 6 commits November 28, 2024 22:21

Do not modify collision detector

21c1d1b

Run collision-detector on 8.4

07c90ab

Cosmetics according to my taste

0d3edef

Fix logic

7eaed3a

Cleanup

f8fc25d

Fix duplicate namespaces

ac3dbb3

ondrejmirtes approved these changes Nov 28, 2024

View reviewed changes

ondrejmirtes merged commit e6dc705 into phpstan:2.0.x Nov 28, 2024
425 of 426 checks passed

jakubtobiasz deleted the allow-properties-in-interface-on-php-84 branch November 29, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent reporting `property.inInterface` error when PHP version is `8.4` or later #3656

Prevent reporting `property.inInterface` error when PHP version is `8.4` or later #3656

jakubtobiasz commented Nov 23, 2024

staabm commented Nov 23, 2024 •

edited

Loading

staabm Nov 23, 2024

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

ondrejmirtes commented Nov 23, 2024

ondrejmirtes left a comment

jakubtobiasz Nov 23, 2024

jakubtobiasz commented Nov 23, 2024

ondrejmirtes left a comment

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

ondrejmirtes commented Nov 23, 2024

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

jakubtobiasz commented Nov 26, 2024

ondrejmirtes commented Nov 26, 2024

ondrejmirtes commented Nov 26, 2024

ondrejmirtes commented Nov 27, 2024

jakubtobiasz commented Nov 27, 2024

jakubtobiasz commented Nov 27, 2024 •

edited

Loading

ondrejmirtes commented Nov 27, 2024

ondrejmirtes commented Nov 28, 2024

ondrejmirtes commented Nov 28, 2024

jakubtobiasz commented Nov 29, 2024

ondrejmirtes commented Dec 29, 2024

Prevent reporting property.inInterface error when PHP version is 8.4 or later #3656

Prevent reporting property.inInterface error when PHP version is 8.4 or later #3656

Conversation

jakubtobiasz commented Nov 23, 2024

staabm commented Nov 23, 2024 • edited Loading

staabm Nov 23, 2024

Choose a reason for hiding this comment

jakubtobiasz commented Nov 23, 2024 • edited Loading

ondrejmirtes commented Nov 23, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

jakubtobiasz Nov 23, 2024

Choose a reason for hiding this comment

jakubtobiasz commented Nov 23, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

jakubtobiasz commented Nov 23, 2024 • edited Loading

ondrejmirtes commented Nov 23, 2024

jakubtobiasz commented Nov 23, 2024 • edited Loading

jakubtobiasz commented Nov 26, 2024

ondrejmirtes commented Nov 26, 2024

ondrejmirtes commented Nov 26, 2024

ondrejmirtes commented Nov 27, 2024

jakubtobiasz commented Nov 27, 2024

jakubtobiasz commented Nov 27, 2024 • edited Loading

ondrejmirtes commented Nov 27, 2024

ondrejmirtes commented Nov 28, 2024

ondrejmirtes commented Nov 28, 2024

jakubtobiasz commented Nov 29, 2024

ondrejmirtes commented Dec 29, 2024

Prevent reporting `property.inInterface` error when PHP version is `8.4` or later #3656

Prevent reporting `property.inInterface` error when PHP version is `8.4` or later #3656

staabm commented Nov 23, 2024 •

edited

Loading

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

jakubtobiasz commented Nov 23, 2024 •

edited

Loading

jakubtobiasz commented Nov 27, 2024 •

edited

Loading