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

PhpDocParser: add config for lines in its AST & enable ignoring errors within phpdocs #2807

Merged
merged 9 commits into from
Jan 3, 2024

Conversation

janedbal
Copy link
Contributor

@janedbal janedbal commented Dec 7, 2023

This allows custom rules to properly report exact lines of e.g. @param tags when needed.

image

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

@janedbal janedbal changed the base branch from 1.11.x to 1.10.x December 7, 2023 13:50
@ondrejmirtes
Copy link
Member

I'm looking forward to this! I'd like a proof of concept of using this in some 1st party PHPStan rules here that report things about PHPDocs. It should probably be done through some helper class so that we have logic in a single place that makes sure "Do we have the lines? Report precise lines. Otherwise report the line with the method declaration" that would apply this only to bleeding edge.

One of the challenges we'll face is ignoring errors I think. Right now you can ignore errors like this: https://phpstan.org/r/31387e59-5309-44f9-8a54-eeff03762cc7

But if line 6 gets reported instead of line 8 then we need to do something about ignoring errors too.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Also - after we merge this, please also checkout 1.11.x and make sure that @phpstan-ignore works as expected. It's a new way to ignore errors with identifiers and it automatically decides if the error to ignore should be on the current line, or the next line.

So somehow it should work inside PHPDocs too.

conf/config.neon Outdated Show resolved Hide resolved
src/Rules/PhpDoc/PhpDocLineHelper.php Outdated Show resolved Hide resolved
tests/PHPStan/Rules/PhpDoc/data/invalid-phpstan-doc.php Outdated Show resolved Hide resolved
@janedbal janedbal changed the title PhpDocParser: add config for lines in its AST PhpDocParser: add config for lines in its AST & enable ignoring errors within phpdocs Dec 8, 2023
@janedbal janedbal requested a review from ondrejmirtes December 8, 2023 16:15
@janedbal
Copy link
Contributor Author

janedbal commented Dec 8, 2023

Should I update docs somehow? The broken edgecase was never documented: https://phpstan.org/user-guide/ignoring-errors

/**
 * @phpstan-ignore-next-line
 *
 * This is the legacy behaviour, the next line is meant as next-non-comment-line
 */
fail();

I'd say it even states the fixed one :D

Both @phpstan-ignore-line and @phpstan-ignore-next-line ignore all errors on the corresponding line.

@ondrejmirtes
Copy link
Member

Should I update docs somehow?

No, it's okay, with that test I was just testing the behaviour to prove the fact that "next line" means "next line after this PHPDoc"

@ondrejmirtes
Copy link
Member

@janedbal If you rebase now the build should not fail because of Rector anymore (after I wrote 1.5k lines in simple-downgrader, 3.5k if you include tests 😂 )

@janedbal janedbal force-pushed the allow-phpdoc-parser-lines branch from dda2a9a to 0f1d48c Compare December 18, 2023 09:29
@janedbal
Copy link
Contributor Author

Rebased.

@ondrejmirtes ondrejmirtes force-pushed the allow-phpdoc-parser-lines branch from 0f1d48c to 81abd0d Compare January 3, 2024 18:48
@ondrejmirtes ondrejmirtes merged commit c5ca230 into phpstan:1.10.x Jan 3, 2024
70 checks passed
@ondrejmirtes
Copy link
Member

Awesome, thank you!

@janedbal janedbal mentioned this pull request Jan 4, 2024
@gnutix
Copy link
Contributor

gnutix commented Jan 6, 2024

No, it's okay, with that test I was just testing the behaviour to prove the fact that "next line" means "next line after this PHPDoc"

I think this behavior is now broken @ondrejmirtes. I just upgraded and started getting :

 ------ ---------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   src/.../SomeClass.php                                                                         
 ------ ---------------------------------------------------------------------------------------------------------------------------------------------------- 
  167    No error to ignore is reported on line 167.                                                                                                         
  168    Method App\...\SomeClass::getFunctions() return type has no signature specified for Closure.  
 ------ ---------------------------------------------------------------------------------------------------------------------------------------------------- 

For code :

    /**
     * @phpstan-ignore-next-line I have no idea how this Closure is typed, that's the point...
     *
     * @return array<Closure|DeferredEvaluation>
     */
    private function getFunctions(Context $context): array
    {
         // ...
    }

If I change it to the following, then it works again :

    /**
     * @return array<Closure|DeferredEvaluation>
     */
    /** @phpstan-ignore-next-line I have no idea how this Closure is typed, that's the point... */
    private function getFunctions(Context $context): array

@janedbal
Copy link
Contributor Author

janedbal commented Jan 6, 2024

@gnutix That is intentional. Now (with bleeding edge), next line means actual next line. See phpstan/phpstan#10374

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

Successfully merging this pull request may close these issues.

5 participants