denzyldick / phanalist

Performant static analyzer for PHP, which is extremely easy to use. It helps you catch common mistakes in your PHP code.
https://denzyldick.github.io/phanalist/
MIT License
127 stars 5 forks source link

feature: support null coalescing expression and array assignment #88

Closed Fabccc closed 1 month ago

Fabccc commented 2 months ago

This PR is supposed to fix issue #87 and #86

Will you need additional tests ?

PS: My very first pull request ever.

Fabccc commented 2 months ago

Nice, Welcome to the team.

It's ok by me. I will let @SerheyDolgushev merge it. E12 was created by him.

Let me know if you want to create more Rules. 💪🏿

If I witness anything that's missing i'll be happy to make another pull request !

SerheyDolgushev commented 2 months ago

Hi @Fabccc!

First of all, a huge thank you for your contribution! It looks amazing!

I tested it locally and found an interesting "false positive" case. This change leads to E12 violation for the following cases:

<?php

namespace App\Service\e12;

class Test
{
    private array $variables = [
        'var1' => 'test1',
        'var2' => 'test2',
    ];

    public function getVariable(string $key): ?string
    {
        return $this->variables[$key] ?? null;
    }
}

and

<?php

namespace App\Service\e12;

class Test
{
    private array $variablesSet1 = [
        'var1' => 'test1',
    ];

    private array $variablesSet2 = [
        'var2' => 'test2',
    ];

    public function getVariable(string $key): string
    {
        return $this->variablesSet1[$key] ?? $this->variablesSet2[$key];
    }
}

Can you please adjust your changes to not trigger violations for such cases? And maybe cover these cases with unit tests?

Thanks a lot for your PR! :)

Fabccc commented 1 month ago

Hi @SerheyDolgushev

Thank you (and @denzyldick) for the warm welcome. Interesting false positive. While I was trying to correct those issues, I found that it was better to highlight the idea of nested checking. I couldn't find any other solution other than to check for array inside the assignment expression method. I choose to move to a separate function both assignment expression and arithmetic expression.

Thank you for taking time to review my PR!

SerheyDolgushev commented 1 month ago

@Fabccc thanks a lot for the updates! It looks great!

I ran another round of testing for the most recent changes, and found only one "false positive":

<?php

namespace App\Service\e12;

class Test
{
    public function __construct(private ?string $guid = null)
    {
    }

    public function getGuidNotNull(): string
    {
        return $this->guid ?? throw new \RuntimeException('Missing guid');
    }
}

Is it something that you can kindly have a look at?

Fabccc commented 1 month ago

Thank you for your feedback @SerheyDolgushev ,

I added your tests and did some changes, I also added static counterpart to the tests suite.

SerheyDolgushev commented 1 month ago

@Fabccc amazing job! huge thank you for the contribution! 🎉

SerheyDolgushev commented 1 month ago

@denzyldick can you please merge this? And I will compile the binaries afterward.