Add return types to methods where they can be inferred

[muglug]

Running my static analysis tool with --update-docblocks produces this diff (which I thought may be useful in light of these changes).

[nikic]

Is it possible to disable generation of @return void annotations? I'm not using those anywhere, as I consider the absence of @return in a doc-block to indicate that there is no meaningful return value.

[muglug]

Sure! I've added a config to control whether void return types are added, and I've updated this PR's commit accordingly.

[nikic]

Shouldn't this type infer to int|float, as strlen() returns int?

[muglug]

There was existing logic to support ints being cast to floats in function calls, and it (falsely) trickled into the type simplification logic – fixed here: vimeo/psalm@5ec2a97

[nikic]

Seen above that Psalm also supports something like @psalm-return array{nodeType:string, text:mixed, line:mixed, filePos:mixed}. If this here were @psalm-return array{0:null|Node\Stmt[], 1:null|Error} then the list() unpacking in the method above could see that Error is not a possible return type.

[muglug]

Unfortunately 0 and 1 as keys imply that they are strings, so argument unpacking wouldn't work there. A solution is either to add string array offsets (and change what the method returns, and how it's used above) or just strip out the return types for now.

Obviously PHP 7.1's array key destructuring would simplify that diff, but happy that PhpParser is PHP 5-compatible.

[nikic]

PHP does not distinguish between integer keys and integral string keys. So here all of [$a, $b], [0 => $a, 1 => $b] and ["0" => $a, "1" => $b] are equivalent (both in terms of array creation and unpacking). I'm fine with just stripping the return type, just wondering why Psalm makes this distinction, i.e., why are "shapes" only created for arrays with string keys, rather than also arrays with fixed-structure integral keys? Is this because there might be false positives where such a return value is intended as a normal array (which just happens to have the same non-uniform types on all paths) rather than a shape?

[nikic]

Overall, nice results. There's a few cases where the return type is too broad (e.g. NameResolver::getResolvedClassName() can't return null), but inferring that would be non-trivial (something like CPA could determine that null is only returned if both arguments are null and in this case at most one of them is).

[muglug]

Thanks! To that last point, how about throwing an exception if both $name1 and $name2 are null in PhpParser\Node\Name::concat? It seems like that can never happen, at least in PhpParser's own usage of that method. Doing so would erase the null value from that return type, and solve that particular issue.

[muglug]

Yup, there's no reason why Psalm shouldn't be able to understand that formulation for ObjectLike types when processing ArrayDimFetch and when destructuring lists. I will ticket and add soonish.

Generating that type is considerably less trivial (as there are a bunch of different desired outcomes depending on the different return statements in a function), but I'll ticket that as well.

[nikic]

Noticed while merging: This return type is off. It should be inferred as string|Name|Node\Identifier, as there are return $names and a return new Node\Identifier via ternary.

[muglug]

Ah bugger. Sorry. It's because both classes have a __toString method, and Psalm gets confused when merging the types. Fixing...

[nikic]

Merged via e3b87f4, with more updates in bfea338, where I manually improved some of the types and added descriptions + param annotations for everything.

I did not merge the changes to the Standard pretty printer, as there basically all methods return strings and I think adding explicit doc comments there is more noisy than helpful.

Thanks!