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

TypeDoesNotContainType - psalm considers a property always null even if it may be set in called method #5231

Closed
sad-spirit opened this issue Feb 15, 2021 · 9 comments
Labels

Comments

@sad-spirit
Copy link
Contributor

Sorry for the size of the reproduce example, the error is transient and disappears if I try to simplify it more.

https://psalm.dev/r/2afe4e3c4b

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/2afe4e3c4b
<?php
class UnicodeUnescaper
{
    /**
     * First codepoint of Unicode surrogate pair
     * @var int|null
     */
    private $pairFirst;

    public function unescapeUnicode(string $escaped): string
    {
        $this->pairFirst = null;
        $unescaped       = '';

        foreach (
            preg_split(
                "/(\\\\(?:\\\\|[0-9a-fA-F]{4}|\\+[0-9a-fA-F]{6}))/",
                $escaped,
                -1,
                PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE
            ) ?: [] as $part
        ) {
            if ('\\' === $part[0] && '\\' !== $part[1]) {
                $unescaped .= $this->handlePossibleSurrogatePairs(hexdec(ltrim($part, '\\+')));

            } elseif (null !== $this->pairFirst) {
                break;

            } elseif ('\\' === $part[0]) {
                $unescaped .= '\\';

            } elseif (false !== strpos($part, '\\')) {
                throw new Exception('Invalid Unicode escape');

            } else {
                $unescaped .= $part;
            }
        }

        if (null !== $this->pairFirst) {
            throw new Exception('Unfinished Unicode surrogate pair');
        }

        return $unescaped;
    }

    private function handlePossibleSurrogatePairs(int $codepoint): string
    {
        $utf8              = '';
        $isSurrogateFirst  = 0xD800 <= $codepoint && 0xDBFF >= $codepoint;
        $isSurrogateSecond = 0xDC00 <= $codepoint && 0xDFFF >= $codepoint;

        if ((null !== $this->pairFirst) !== $isSurrogateSecond) {
            throw new Exception("Invalid Unicode surrogate pair");
        } elseif (null !== $this->pairFirst) {
            $utf8 = 'pair';
            $this->pairFirst = null;
        } elseif ($isSurrogateFirst) {
            $this->pairFirst = $codepoint;
        } else {
            $utf8 = 'symbol';
        }

        return $utf8;
    }
}

// this will trigger an Exception psalm claims cannot be triggered
(new UnicodeUnescaper())->unescapeUnicode('wrong: \\db99');
Psalm output (using commit 86ba3b0):

ERROR: TypeDoesNotContainType - 26:23 - Type null for $this->pairFirst is always null

ERROR: TypeDoesNotContainType - 40:13 - Type null for $this->pairFirst is always null

@orklah
Copy link
Collaborator

orklah commented Feb 15, 2021

This can be fixed on your side like this:
https://psalm.dev/r/614a846739

This looks like something rememberPropertyAssignmentsAfterCall should have an impact on, but I think the flow is too complicated. Not sure if it's a bug or not

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/614a846739
<?php
class UnicodeUnescaper
{
    /**
     * First codepoint of Unicode surrogate pair
     * @var int|null
     */
    private $pairFirst;

    public function unescapeUnicode(string $escaped): string
    {
        unset($this->pairFirst);
        $unescaped       = '';

        foreach (
            preg_split(
                "/(\\\\(?:\\\\|[0-9a-fA-F]{4}|\\+[0-9a-fA-F]{6}))/",
                $escaped,
                -1,
                PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE
            ) ?: [] as $part
        ) {
            if ('\\' === $part[0] && '\\' !== $part[1]) {
                $unescaped .= $this->handlePossibleSurrogatePairs(hexdec(ltrim($part, '\\+')));

            } elseif (null !== $this->pairFirst) {
                break;

            } elseif ('\\' === $part[0]) {
                $unescaped .= '\\';

            } elseif (false !== strpos($part, '\\')) {
                throw new Exception('Invalid Unicode escape');

            } else {
                $unescaped .= $part;
            }
        }

        if (null !== $this->pairFirst) {
            throw new Exception('Unfinished Unicode surrogate pair');
        }

        return $unescaped;
    }

    private function handlePossibleSurrogatePairs(int $codepoint): string
    {
        $utf8              = '';
        $isSurrogateFirst  = 0xD800 <= $codepoint && 0xDBFF >= $codepoint;
        $isSurrogateSecond = 0xDC00 <= $codepoint && 0xDFFF >= $codepoint;

        if ((null !== $this->pairFirst) !== $isSurrogateSecond) {
            throw new Exception("Invalid Unicode surrogate pair");
        } elseif (null !== $this->pairFirst) {
            $utf8 = 'pair';
            $this->pairFirst = null;
        } elseif ($isSurrogateFirst) {
            $this->pairFirst = $codepoint;
        } else {
            $utf8 = 'symbol';
        }

        return $utf8;
    }
}

// this will trigger an Exception psalm claims cannot be triggered
(new UnicodeUnescaper())->unescapeUnicode('wrong: \\db99');
Psalm output (using commit bc13001):

No issues!

@sad-spirit
Copy link
Contributor Author

This can be fixed on your side like this:

Well, it is fixed in the sense it doesn't trigger psalm errors, but...

Notice: Undefined property: UnicodeUnescaper::$pairFirst

This looks like something rememberPropertyAssignmentsAfterCall should have an impact on

It was set to true (the default), if I set it to false these particular errors stay.

Not sure if it's a bug or not

In the actual codebase I have two methods with similar flow calling handlePossibleSurrogatePairs(): one triggers the above error and the other doesn't. IMO it should be either both or none.

@orklah
Copy link
Collaborator

orklah commented Feb 15, 2021

Ouch, forgot that unset was completely destroying the property before typed properties.

What I meant when I said I was not sure is that it may be intentional for complex code flow to just ignore a fonction that may be passed.

Do you have the example for the second call? Maybe we can deduce the difference between the two

@sad-spirit
Copy link
Contributor Author

sad-spirit added a commit to sad-spirit/pg-builder that referenced this issue Feb 17, 2021
@muglug muglug added the bug label Feb 24, 2021
@muglug
Copy link
Collaborator

muglug commented Feb 24, 2021

Simplified to https://psalm.dev/r/bdfb5e3ad2

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/bdfb5e3ad2
<?php
class A {
    private ?int $bar = null;

    public function baz(): void
    {
        $this->bar = null;
        
        foreach (range(1, 5) as $part) {
            if ($part === 3) {
                $this->foo();
            }
        }

        if ($this->bar === null) {}
    }
    
    private function foo() : void {
        $this->bar = 5;
    }
}
Psalm output (using commit f8cbb22):

ERROR: RedundantCondition - 15:13 - Type null for $this->bar is always null

@muglug muglug closed this as completed in 924f6b6 Feb 25, 2021
@sad-spirit
Copy link
Contributor Author

Please note that the error is gone in your simplified code, but it is still triggered in the original code I provided.

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants