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

Something weird. #8018

Closed
rzvc opened this issue May 26, 2022 · 7 comments
Closed

Something weird. #8018

rzvc opened this issue May 26, 2022 · 7 comments
Assignees

Comments

@rzvc
Copy link

rzvc commented May 26, 2022

Please rename this issue to something more appropriate, once you figure out what it is. I didn't know what to name it.

/**
* @psalm-type Test = string
*/
class Foo
{
    /** @var array<string, Test> */
	private array $stuff = [];
    
    /**
    * @param non-empty-list<string> $param
    * @return Test
    */
    public function foo(array $param)
    {
        $arr = &$this->stuff[$param[0]];	// This seems to cause Psalm to blackout.
        
        /** @var Test */
        $x = $arr;
        
        // This trace doesn't work, and $x's type appears to be mixed at this point.
        /** @psalm-trace $x */
        return $x;
    }
}

https://psalm.dev/r/f81173405a

Converting $param to string, works: https://psalm.dev/r/718c05c153
Same if we remove the reference on the offending line: https://psalm.dev/r/6ebda981ae

@psalm-github-bot
Copy link

psalm-github-bot bot commented May 26, 2022

I found these snippets:

https://psalm.dev/r/f81173405a
<?php
/**
* @psalm-type Test = string
*/
class Foo
{
    /** @var array<string, Test> */
	private array $stuff = [];
    
    /**
    * @param non-empty-list<string> $param
    * @return Test
    */
    public function foo(array $param)
    {
        $arr = &$this->stuff[$param[0]];	// This seems to cause Psalm to blackout.
        
        /** @var Test */
        $x = $arr;
        
        // This trace doesn't work, and $x's type appears to be mixed at this point.
        /** @psalm-trace $x */
        return $x;
    }
}
Psalm output (using commit b46fb14):

INFO: MixedInferredReturnType - 12:15 - Could not verify return type 'string' for Foo::foo
https://psalm.dev/r/718c05c153
<?php
/**
* @psalm-type Test = string
*/
class Foo
{
    /** @var array<string, Test> */
	private array $stuff = [];
    
    /**
    * @return Test
    */
    public function foo(string $param)
    {
        $arr = &$this->stuff[$param];	// This seems to cause Psalm to blackout.
        
        /** @var Test */
        $x = $arr;
        
        // This trace doesn't work, and $x's type appears to be mixed at this point.
        /** @psalm-trace $x */
        return $x;
    }
}
Psalm output (using commit b46fb14):

ERROR: UnnecessaryVarAnnotation - 17:18 - The @var string annotation for $x is unnecessary

INFO: Trace - 22:9 - $x: string
https://psalm.dev/r/6ebda981ae
<?php
/**
* @psalm-type Test = string
*/
class Foo
{
    /** @var array<string, Test> */
	private array $stuff = [];
    
    /**
    * @param non-empty-list<string> $param
    * @return Test
    */
    public function foo(array $param)
    {
        $arr = $this->stuff[$param[0]];	// This seems to cause Psalm to blackout.
        
        /** @var Test */
        $x = $arr;
        
        // This trace doesn't work, and $x's type appears to be mixed at this point.
        /** @psalm-trace $x */
        return $x;
    }
}
Psalm output (using commit b46fb14):

ERROR: UnnecessaryVarAnnotation - 18:18 - The @var string annotation for $x is unnecessary

INFO: Trace - 23:9 - $x: string

@AndrolGenhald
Copy link
Collaborator

Oh joy, references! References are only somewhat supported. This issue is probably my fault because I actually made references work in most cases instead of being ignored, but that caused a lot of edge cases that were previously fine to be broken.

FYI that code is actually not sound (maybe your actual code is though), PHP references are really weird and imo should be avoided except for trivial cases, but otoh this does look like it should be a trivial case since the reference only exists locally and isn't used in an array or object. It's undoubtedly a bug that everything after that reference is just ignored.

@rzvc
Copy link
Author

rzvc commented May 26, 2022

FYI that code is actually not sound (maybe your actual code is though)

Yup, I just came up with that example to show the issue.

@AndrolGenhald
Copy link
Collaborator

Haven't looked at the code, but I'm guessing the reason this works but your original example doesn't has something to do with the fact that Psalm tracks array offset types for array-key variables, but it doesn't track array offset types when the offset is itself coming from an array.

I'm guessing the solution to this (if we actually want to fix it) is to make it so the rest of the analysis continues without bothering to account for the reference. Maybe we should have some sort of UnsupportedReferenceUsage issue for that case so we don't have people complaining that Psalm isn't working correctly? At least that would be better than the current confusing behavior.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ed1a4b5cdd
<?php
/**
* @psalm-type Test = string
*/
class Foo
{
    /** @var array<string, Test> */
	private array $stuff = [];
    
    /**
    * @param non-empty-list<string> $param
    * @return Test
    */
    public function foo(array $param)
    {
        $tmp = $param[0];
        $arr = &$this->stuff[$tmp];
        
        /** @var Test */
        $x = $arr;
        
        // This trace doesn't work, and $x's type appears to be mixed at this point.
        /** @psalm-trace $x */
        return $x;
    }
}
Psalm output (using commit b46fb14):

ERROR: UnnecessaryVarAnnotation - 19:18 - The @var string annotation for $x is unnecessary

INFO: Trace - 24:9 - $x: string
https://psalm.dev/r/b636ed9fe4
<?php

/** @var array<string, string> */
$arr = [];

/** @var string */
$foo = "foo";

$arr[$foo] = "bar";
/** @psalm-trace $arr, $arr[$foo] */;
Psalm output (using commit b46fb14):

INFO: Trace - 10:37 - $arr: non-empty-array<string, string>

INFO: Trace - 10:37 - $arr[$foo]: 'bar'
https://psalm.dev/r/d4c6d40dab
<?php

/** @var array<string, string> */
$arr = [];

/** @var non-empty-list<string> */
$foo = ["foo"];

$arr[$foo[0]] = "bar";
/** @psalm-trace $arr, $arr[$foo[0]] */;
$bar = $arr[$foo[0]];
/** @psalm-trace $bar */;
Psalm output (using commit b46fb14):

INFO: Trace - 10:40 - $arr: non-empty-array<string, string>

ERROR: UndefinedTrace - 10:40 - Attempt to trace undefined variable $arr[$foo[0]]

INFO: Trace - 12:25 - $bar: string

@AndrolGenhald
Copy link
Collaborator

Actually, any reference to an array is dangerous due to that PHP bug: https://3v4l.org/vaCAp

I think I'll add a warning about that as well.

@rzvc
Copy link
Author

rzvc commented May 26, 2022

I wouldn't mind a warning with an explanation. I came up with the same solution, of copying the value at that index in a local variable.

AndrolGenhald added a commit to AndrolGenhald/psalm that referenced this issue May 26, 2022
orklah added a commit that referenced this issue May 27, 2022
…re-case-for-references

Improve handling of unsupported references (fixes #8018).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants