allow @var phpdocs before return statements

[bendavies]

This PR stops phpdocs before return statements from being converted to comments, if they are a @var definition.

This is a common requirement with SA tooling to assist the tool with the return type.
e.g:
https://psalm.dev/r/0de99e28de

<?php

interface EncoderInterface
{
    /**
     * @return bool|float|int|string
     */
    public function encode(array $data);
}

function returnsString(EncoderInterface $encoder): string
{
    /** @var string */
    return $encoder->encode(
        ['invoice_number' => 1],
    );
}

We don't want /** @var string */ converted to // @var string in this instance.

Thanks.

[julienfalque]

Alternative proposal related to this: #4649.

[ostrolucky]

Similar was rejected before #3611

[bendavies]

seems like a slightly different use case?
no 3rd party annotations here.

[ostrolucky]

It's not a problem of 3rd party annotations, but interpretation of phpDocumentor specs. In linked ticket, it was interpreted that not specifying structural element in inline phpDoc, it's not conforming with phpDocumentor specs. In case of @var, this is even more strictly prohibitted. From https://docs.phpdoc.org/latest/references/phpdoc/tags/var.html

Syntax

@var [“Type”] [$element_name] [<description>]

The @var tag MUST contain the name of the element it documents.

[bendavies]

I see. Thanks for the clarification

…

On Tue, 11 Aug 2020, 17:52 Gabriel Ostrolucký, ***@***.***> wrote: It's not a problem of 3rd party annotations, but interpretation of phpDocumentor specs. In linked ticket, it was interpreted that not specifying structural element in inline phpDoc, it's not conforming with phpDocumentor specs. In case of @var, this is even more strictly prohibitted. From https://docs.phpdoc.org/latest/references/phpdoc/tags/var.html Syntax @var [“Type”] [$element_name] [<description>] The @var tag MUST contain the name of the element it documents. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYV4CR7PCBCTCZXAWK5DTSAFZLPANCNFSM4P3DDDQA> .

[bendavies]

I would just point that that phpstorm, phpstan and psalm all support this syntax.

[GrahamCampbell]

I think this fixer should just be turned off tbh, if you want that. This fixer was designed to enforce PSR-5 doc positioning. If you want to document types for static analyzers, then you probably don't want this fixer. I no longer use it, myself.

[fluffycondor]

In case of @var, this is even more strictly prohibitted. https://docs.phpdoc.org/latest/references/phpdoc/tags/var.html
The @var tag MUST contain the name of the element it documents.

But on the next line there is following:

An exception to this is when property declarations only refer to a single property. In this case the name of the property MAY be omitted.

A return statement operating only single property at once, so there is no need in name.

[GrahamCampbell]

A return statement is not a single property. A single property is this:

class Foo
{
    /** @var string */
    public $foo; // <- i am a single property
}

The phpdocumentor spec does not allow documentation above return statements.

[GrahamCampbell]

As I said before, in a different issue, at length, we have to decide if we want to follow phpdocumentor or static analyzers. phpdocumentor will never support stuff that makes the code easier to analyzem because its job is to document the public interface of stuff, not the internals. phpdocumentor says phpdoc can only appear before structural elements.

[mfn]

You could argue that static analyzers started to miss-use phpdoc for this, but there's really no alternative and that's how "it evolved".

The practical choice would be to go separate paths, but what about interop then? Competitors alternatives to php-cs-fixer might come up with different ideas/approaches and then the fragmentation gets worse.

PS: I'm leaning towards being practical, but…

[muglug]

I think PHP-CS-Fixer should follow the fairly-widely-adopted community standard.

But I also think it's a shame that @var was chosen for inline things – something like

function returnsString(EncoderInterface $encoder): string
{
    /** @infer string */
    return $encoder->encode(
        ['invoice_number' => 1],
    );
}

is much more appropriate.

[ondrejmirtes]

People are used to write @var above any line of code expecting it to work or at least do something they want. In PHPStan I had to add support for @var above if statements, whiles, throws, switches, returns, echoes...

I dislike inline @vars because they shouldn't be necessary at all - the source should already have the correct type when obtaining the value. They're also used for different reasons which makes it impossible to validate them - sometimes they're used to narrow down types, but sometimes also to widen or even change the types totally in case the source's PHPDoc is wrong...

[ondrejmirtes]

They're also used to will nonexistent variables into existence when used in the global scope, typically view files. A total mess :)

[muglug]

Yeah, that's why I think /** @infer string */ should be used instead as it doesn't muddy the waters (I'm happy to add support for this in an upcoming version of Psalm).

[fluffycondor]

I dislike inline @vars because they shouldn't be necessary at all - the source should already have the correct type when obtaining the value

If there is typehinted a parent class as return value in a parent method's signature, how would I tell to PHPStan that I 100% sure that in the subclass I will get a subclass of return value?
It is widely used in Repository pattern.

// find() is overwritten above to ignore soft-deleted entities
public function findDeleted($id, $lockMode = null, $lockVersion = null): ?Project
{
    // it returns object|null by default,
    // I will get "should return Project|null but returns object|null" from PHPStan
    return parent::find($id, $lockMode, $lockVersion);
}

[ondrejmirtes]

@fluffycondor You have many tools at your disposal:

• Generics
• Stub files
• Dynamic return type extensions
• Conditional types

All of them are better than copying @var over your codebase and them becoming inconsistent over time...

[kubawerlos]

This PR changes the behaviour for a fixer, not fixing an issue, for many people the "fix" that this PR disables may be very much expected.

I would suggest adding an option ignore_before_return (defaults to false to keep backward compatibility) for anyone wanting changes from this PR.

[julienfalque]

IMO this is a de facto standard so we should support it. But I agree with @kubawerlos, we should add an option for this behavior change.

[julienfalque]

This condition will be true for any tag that contains @var, e.g. @variable. Please add such tag as a new test case to make sure it is still changed.

[SpacePossum]

I agree this is a de facto standard so we should support it, I see little value in having it behind some option as I consider this a bug fix.

[GrahamCampbell]

I agree now, too. Disregard my earlier comments, or consider them in the context of 2020. ;)

[keradus]

closing due to merge of #7402