#2824: Automatically add to DocBlock comments the thrown \Throwables.

[Aerendir]

#2824: Automatically add to DocBlock comments the thrown \Throwables.

Was PR #2828

• Support methods
• Support functions

Todo in next release

• Support \Throwables assigned to variables ($value = new \Exception())[https://github.com/#2824: Automatically add to DocBlock comments the thrown \Throwables. #2833#discussion_r377556485]
• Support \Throwables generated by methods or functions (throw $this->createException() and throw createException())
• Aliased imports (two exceptions with the same name, but different namespaces and one of them is aliased)

[Aerendir]

@TomasVotruba , I did a mess trying to sync the branch with master and I recreated it from scratch.

It seems that it doesn't work anymore: the tests that once passed, now don't pass anymore.

It is seems that the modifications are not get by Rector.

The $node I return seems to be modified with the update DocComment, but the test fails anyway.

Any ideas about why?

[TomasVotruba]

Can you allow me to push to the branch?

[TomasVotruba]

I don't know how to push to your external branch.
I'll make you a collab, so we can cooperate more easily

[TomasVotruba]

I sent you an invite

[Aerendir]

@TomasVotruba in the mean time I understood how to use AttributeKey::PHP_DOC_INFO so I'm now able to update the doc comment: the test now pass! 🎉

I don't know exactly what happened, but for sure the failing of the test was related to the fact I "hardly" updated the docblock comment without using the AttributeKey::PHP_DOC_INFO.

Now I'm going to check again the other tests that once passed...

[TomasVotruba]

Great, I was making working demo for php doc info. It's only 5 lines instead of 40 :)

I'm glad you made it work! Could you push it?

[Aerendir]

@TomasVotruba just pushed!

[Aerendir]

Just run composer complete-check: there are some tests that fails that are not related to my PR.

[TomasVotruba]

Just run composer complete-check: there are some tests that fails that are not related to my PR.

Have you run composer update? You might have old deps

[Aerendir]

@TomasVotruba , I think the PR is complete.

I'm fixing the checks in CI.

Can you, please, check the code is ok?

[Aerendir]

@TomasVotruba , I've fixed as much errors as I can, but there are some for which I need your guidance as they are strictly related to Rector and I think there is already a "standard" approach to fix them.

I'm waiting for your feedback.

[Aerendir]

@TomasVotruba , I read all your comments: I will work on them ASAP.

[Aerendir]

@TomasVotruba , regarding variables and factory method the things are really complex.

Imagine this code:

<?php

function createHttpException(int $statusCode) {
    switch ($statusCode) {
        case 404:
            return new NotFoundException();
        case 500:
            return new InternalServerErrorException();
        default:
            return new ConnectionException();
    }
}

/**
 * @param string $url
 * 
 * @throws ... What?
 */
function connect(string $url)
{
    // do connection things
    $response = ...;
    
    // Imagine is 404
    $status = $response->getStatus();
    
    if (200 !== $status) {
        throw createHttpException(404);
    }
}

Which is the thrown exception? All? And for more complex logics? And how can we understand the code to annotate the DocBlock?

I've done an example with factory methods, but the same applies also to \Throwables assigned to variables.

WDYT?

[TomasVotruba]

What is the thrown exception? All? And for more complex logics? And how can we understand the code to annotate the DocBlock?

I don't use it. What is common approach?

[TomasVotruba]

Maybe we could solve complex annotations like this in next PR, so there is smaller scope to focus on and finished work

[Aerendir]

I don't use it. What is common approach?

Ok, let me think a bit about that and so some tests with Psalm and prepare some failing tests...

I'll also test the current version of the Rector on my app to see what happens.

Then we will make a decision.

[Aerendir]

Psalm test: https://psalm.dev/r/d74ddf52b7

[TomasVotruba]

We run on PHPStan, so Psalm test is useless here

[Aerendir]

It is a memo to not be forced to rewrite again the code each time.

PHPStan doesn't alert about missing @throws tags, so Psalm is useful to me to understand its behaviour beacuse this helps me in finding the behavior to give to the rector.

Having the snippet linked here makes me able to edit it instead of rewriting it each time and fastly clarify what happens in different situations instead of having a project on my computer just to test a bunch of lines of code.

I hope you understand my point of view.

[Aerendir]

This is the same code run on PHPStan: https://phpstan.org/r/c6491906-2837-44fa-b0c7-8400338f977a

No alert about missed @throws tags :(

[TomasVotruba]

I understand the idea. Just that you'll have to figure out how to make that work with Rector and PHPStan anyway, so Psalm might lead you astrey.

[Aerendir]

@TomasVotruba , I reverted the branch to a stable state: check if it is ready to be merged.

Then I will open other PRs to implement the other features...

[Aerendir]

PS
I've also added the folder Throw_ as actually the rector doesn't operate on methods anymore, but also on functions.

[TomasVotruba]

CI needs to pass though

[Aerendir]

CI needs to pass though

It doesn't trigger as I updated the docs/AllRectorsOverview.md and it now conflicts with the one in master (another rector added?)...

[TomasVotruba]

Just change some space/file and commit

[TomasVotruba]

Rebase on master is needed too. That's why it's better to create small PR and merge it in 1-2 days then re-iterate, that do this tedius tyding up work.

[Aerendir]

@TomasVotruba , I've fixed what I can, but there is the conflict on docs/AllRectorsOverview.md that I don't know how to solve and another issue with PHPStan that complains about the line 110 (return $alreadyAnnotatedThrowTag->value->type->name;) saying that:

vendor/bin/phpstan analyse --ansi --error-format symplify
Note: Using configuration file /Users/Aerendir/Documents/JooServer/_ProjectsIContributeTo/rector/phpstan.neon.
 1602/1602 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------------------------------------------------------------------------
 packages/coding-style/src/Rector/Throw_/AnnotateThrowablesRector.php:110
 ------------------------------------------------------------------------
 - '#Access to an undefined property PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocTagValueNode\:\:\$type#'
 ------------------------------------------------------------------------

There are also other similar issues...

[TomasVotruba]

Just rebuild docs with composer docs

[TomasVotruba]

The phpstan issue can be solved with instanceof

If that's the only blocker, I can merge it and resolve

[Aerendir]

Just rebuild docs with composer docs

It didn't worked: I already rebuilt docs, but there is still a conflict...

The phpstan issue can be solved with instanceof

It should...

If that's the only blocker, I can merge it and resolve

For sure you have more familiarity with the repository 😅

[Aerendir]

As told some days ago, on my local fork there are also some failing tests not related to this PR and I don't know how to solve them but, mostly, if I have to...

[TomasVotruba]

Is it possible you didn't sync your fork first?

I'll look on it