Skip to content
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

False positive of RemoveUselessVarTagRector #8870

Open
dragosprotung opened this issue Oct 24, 2024 · 7 comments · May be fixed by rectorphp/rector-src#6396
Open

False positive of RemoveUselessVarTagRector #8870

dragosprotung opened this issue Oct 24, 2024 · 7 comments · May be fixed by rectorphp/rector-src#6396
Labels

Comments

@dragosprotung
Copy link

Bug Report

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/c779dd90-39ba-4cf1-8222-984a30d19816

<?php

interface Properties{}

/**
 * @template TProperties of Properties|null
 */
final class DemoFile
{
    /** @var TProperties */
    private Properties|null $properties;
}

Responsible rules

  • RemoveUselessVarTagRector

Expected Behavior

Tag should not be removed

@samsonasik
Copy link
Member

That's expected TProperties is defined as Properties|null, while native property tyep is Properties|null itself, so they are equal, so dead code is expected :)

even child implementor, using @template tag will mark as

Property DemoFile<TPropertyImpl of PropertiesImpl|null>::$properties (TPropertyImpl of PropertiesImpl|null) does not accept PropertiesImpl.

see:

https://phpstan.org/r/abf330cf-4372-49ac-ba92-51ffa77d0c3a

so possibly a phpstan bug on invalid detecting it :)

I suggest to change to just direct property definition, if child is used, see

https://phpstan.org/r/9414d887-ce5c-45c2-9985-b930517127cc

@dragosprotung
Copy link
Author

dragosprotung commented Oct 24, 2024

@samsonasik null must be part of the template. This is because some implementations might always return null (they do not have properties).

Here is another example https://getrector.com/demo/00f806a6-3347-4011-aa9d-6ac589f56d21

@samsonasik
Copy link
Member

You define TProperties of Properties|null, while property has native type already protected Properties|null $properties;, they are same.

@dragosprotung
Copy link
Author

From a native perspective, you are right, they are the same.

But it's not about native types. In the case of a Printer the value will always be null and in the case of a Monitor the value will always be MonitorProperties.

Does Rector only look at the types only from a native perspective ?

@samsonasik
Copy link
Member

You can use direct child implementor of native type as docblock if you don't want it to be removed, see https://getrector.com/demo/59fb3ce1-5118-44e2-a52f-2b3bdcaf66e1

@dragosprotung
Copy link
Author

I need to keep the property in the base abstract class.

I've made the 2 versions for PHPStan:
https://phpstan.org/r/1ab4f06b-5184-46c3-bbd8-c30c6f418027 (with the @var tag)
https://phpstan.org/r/fff02c41-586a-42c6-84f1-66a5696953cf (without the @var tag)

I do not consider this a PHPStan bug.

I also made it in Psalm:
https://psalm.dev/r/00ee6fbeb9 (with the @var tag)
https://psalm.dev/r/447851edd4 (without the @var tag)

@samsonasik
Copy link
Member

Ok, reopen then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants