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

Improved "deprecated" support to work with internal classes and PHP version constraints #801

Merged
merged 4 commits into from
Oct 13, 2021

Conversation

kukulich
Copy link
Collaborator

@kukulich kukulich commented Oct 11, 2021

The "deprecated" support is provided by the @deprecated annotation. There was already support for it, so I've just extend it.

There's just one disadvantage - the doccomment is not the same that is in the stubs but I don't think it's an issue for internal stubs.


There's another possible solution. The support can be provided by special BR Deprecated attribute that will be added in ReflectionSourceStubber or PhpStormSourceStubber.

Advantages:

Waiting for ideas/thoughts :)

@Ocramius
Copy link
Member

here's just one disadvantage - the doccomment is not the same that is in the stubs but I don't think it's an issue for internal stubs.

Not a big deal: internal stubs are not really in use by anyone so far, AFAIK.

If we need parity on internal methods too, we can improve/iterate on that

@Ocramius Ocramius added this to the 5.0.0 milestone Oct 12, 2021
@kukulich
Copy link
Collaborator Author

@Ocramius Is it something I should fix here? I'm not able to find anything...

{
private static ?DocBlockFactory $docBlockFactory = null;

public static function isDeprecated(string $docComment): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm - would be nice if we can make this @psalm-pure if it is static, although I'm not sure if DocBlockFactory::createInstance is pure itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove DocBlockFactory completely. It can be easy regexp I suppose...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can mark it @psalm-pure and then use an inline suppression after inspection of DocblockFactory.

At first glance, DocblockFactory seems quite safe to use, when isolated this way: https://github.com/phpDocumentor/ReflectionDocBlock/blob/0005eb9eaecc2a3a00b8ee34c06643a316ebb228/src/DocBlockFactory.php

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I suppress everything?

ERROR: ImpureStaticProperty - src/Reflection/Deprecated/DeprecatedHelper.php:25:9 - Cannot use a static property in a mutation-free context (see https://psalm.dev/221)
        self::$docBlockFactory ??= DocBlockFactory::createInstance();


ERROR: ImpureMethodCall - src/Reflection/Deprecated/DeprecatedHelper.php:25:53 - Cannot call an impure method from a pure context (see https://psalm.dev/203)
        self::$docBlockFactory ??= DocBlockFactory::createInstance();


ERROR: ImpureStaticProperty - src/Reflection/Deprecated/DeprecatedHelper.php:27:21 - Cannot use a static property in a mutation-free context (see https://psalm.dev/221)
        $docBlock = self::$docBlockFactory->create($docComment);


ERROR: ImpureMethodCall - src/Reflection/Deprecated/DeprecatedHelper.php:27:45 - Cannot call a non-mutation-free method phpDocumentor\Reflection\DocBlockFactory::create from a pure context (see https://psalm.dev/203)
        $docBlock = self::$docBlockFactory->create($docComment);


ERROR: ImpureMethodCall - src/Reflection/Deprecated/DeprecatedHelper.php:29:27 - Cannot call a non-mutation-free method phpDocumentor\Reflection\DocBlock::hasTag from a pure context (see https://psalm.dev/203)
        return $docBlock->hasTag('deprecated');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say so - all of this is literally in-memory operations on a string

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius Ocramius changed the title Improved "deprecated" support Improved "deprecated" support to work with internal classes and PHP version constraints Oct 13, 2021
@Ocramius Ocramius self-assigned this Oct 13, 2021
@Ocramius Ocramius merged commit a3988cc into Roave:5.0.x Oct 13, 2021
@Ocramius
Copy link
Member

Thanks @kukulich! 👍

@kukulich kukulich deleted the deprecated branch October 13, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants