vimeo/psalm

A static analysis tool for finding errors in PHP applications

psalm's Issues

Casting class to string should raise error

Should be errors:

class A {}
echo (new A);

class A {
    function __toString() : void { }
}
echo (new A);

class A {
    function __toString() { }
}
echo (new A);

should not be an error:

class A {
    function __toString() : string {
        return 'hello';
    }
}
echo (new A);

Emit issue on invalid parent call to non-static method from static context

class A {
    public function foo(){}
}

class B extends A {
    public static function bar(){
        parent::foo();
    }
}

Support a little algebra

Not currently supported by Psalm (or Hack, or Flow)

function do_algebra() : string {
    $a = rand(0, 10) > 5 ? "a" : null;
    $b = rand(0, 10) > 5 ? "b" : null;

    if ($a === null && $b === null) {
        return "all null";
    } elseif ($a === null) {
        return $b;
    } else {
        return $a;
    }
}

Add support for aliased types

/** @psalm-type CoolType = A|B|null **/

...

/** @return CoolType */
function foo() {
...
}

/** @param CoolType $a **/
function bar ($a) : void { }

bar(foo());

TypeDoesNotContainType issues with is_numeric() and strings

I have the following code that's parsing annotations for error codes, by looking for (\SomeError\Class::CASE) or (1337). Psalm seems to be having issues with the is_numeric check I'm doing to verify if the found error code is a numeric, or a FQN class name.

if (preg_match(self::REGEX_ERROR_CODE, $doc, $matches)) {
    $error_code = substr($matches[1], 1, -1);
    if (is_numeric($error_code)) {
        $parsed['error_code'] = $error_code;
    } else {
        if (!defined($error_code)) {
            throw UncallableErrorCodeException::create($this->docblock, $this->controller, $this->method);
        }

        $parsed['error_code'] = constant($error_code);
    }

    $doc = trim(preg_replace(self::REGEX_ERROR_CODE, '', $doc));
}

$ ./vendor/bin/psalm 
ERROR: TypeDoesNotContainType - src/Parser/Annotations/ThrowsAnnotation.php:116 - Cannot resolve types for $error_code - string does not contain numeric
            if (is_numeric($error_code)) {

Ideas?

Allow negated assignment in else

TypeTest::testNegatedAssignmentInIf():

if (!($row = (rand(0, 10) ? [5] : null))) {
    // do nothing
}  else {
    echo $row[0];
}

This code emits a MixedArrayOffset issue because the type the if condition evaluates to empty, and negating empty produces mixed.

Fix awkward instanceof && not null property

Given

<?php

class A {
  public function hello() {
    return 'hello';
  }
}

class B {
}

class C extends B {
  /** @var A|null */
  public $foo;
}

$maybe_c = rand(0, 10) ? new C() : new B();

,

if (!$maybe_c instanceof C || !$maybe_c->foo) {
  throw new \Exception('OK');
}

echo $maybe_c->foo->hello(); 

fails but

if (!$maybe_c instanceof C) {
  throw new \Exception('OK');
}

if (!$maybe_c->foo) {
  throw new \Exception('OK');
}

echo $maybe_c->foo->hello(); 

passes (even though they're functionally the same

Add support for enums as parameters or return types

You always have to fake enums in PHP since they're not supported. Psalm could help this!

One option would be to do a syntax like this:

/**
 * @return 'A'|'B'|'C'
 */

The quotes would differentiate the strings from classes, and this would fit pretty naturally with the way Psalm currently allows notation with false like

/**
 * @return string|false
 */

Or, you could do something like

/**
 * @return enum{'A', 'B', 'C'}
 */

which would kind of match the current fancy array syntax.

No longer following SemVer?

We no longer following SemVer with this?

• https://github.com/vimeo/psalm/tree/0.2.9.0
• https://github.com/vimeo/psalm/tree/0.3.5.0

Ideally, these would just be 0.2.10 and 0.3.6.

Concatenating with a non-string should emit an InvalidScalarArgument issue

$a = 1;
echo $a . "hello";

Emit issue when defaulting int param to false

function f(int $a = false) { }

Functions in root namespace do not need slash

Currently emits issue:

namespace {
 function foo() { 
 }
}
namespace A {
 foo(); 
}

Emit issue mixing non-arrays and arrays in binary ops

$a = [1] + 1;

Add support for contingent variables

This (bad) code currently emits a PossiblyUndefinedVariable issue. It shouldn't, as Psalm should know that $a is defined whenever $do_thing is !empty.

$do_thing = rand(0, 10) > 5;
if ($do_thing) {
    $a = 1;
}
if ($do_thing) {
    $b = $a;
}

This code should still emit a PossiblyUndefinedVariable issue, because $do_thing is redefined.

$do_thing = rand(0, 10) > 5;
if ($do_thing) {
    $a = 1;
}
$do_thing = rand(0, 10) > 5;
if ($do_thing) {
    $b = $a;
}

Logical issue with instanceof subtraction

class Foo {}
class FooBar extends Foo{}
class FooBarBat extends FooBar{}
class FooMoo extends Foo{}

$a = new Foo();

if ($a instanceof FooBar && !$a instanceof FooBarBat) {
} elseif ($a instanceof FooMoo) {  
}

FailedTypeResolution error is emitted in the elseif.

It works if && !$a instanceof FooBarBat is not included.

Psalm doesn't throw an InvalidDocblock error if a parameter is missing a type

Paste

<?php
/**
 * @param $bar
 * @return void
 */
function fooBarBaz() {
}

into getpsalm.org.

Observed: No errors
Expected: InvalidDocblock because @param $bar is missing a type

Emit issue when optional param before required

function foo (int $a = null, int $b) { }

Exiting with status of 1 with only INFO errors?

psalm/src/Psalm/IssueBuffer.php

Line 191 in 9132883

exit(1);

https://travis-ci.org/paragonie/sodium_compat/builds/191811956

Any chance we can configure a threshold for "worst error level allowed for exiting with a status of nonzero?"

Allow different errorLevels on different files

In the psalm.xml, you can currently set an error level for all files except a few, where it will be suppressed:

        <MissingReturnType errorLevel="info">
            <excludeFiles>
                <directory name="tests" />
            </excludeFiles>
        </MissingReturnType>

And you can set an error level for specific files, and suppress it everywhere else:

        <MissingReturnType errorLevel="info">
            <includeFiles>
                <directory name="src" />
            </includeFiles>
        </MissingReturnType>

But there's no way to have some files set to info, and some set to error. One possible syntax could be:

        <MissingReturnType errorLevel="info" />

        <!-- overrides the previous errorLevel for these files -->
        <MissingReturnType errorLevel="error">
            <includeFiles>
                <directory name="src" />
            </includeFiles>
        </MissingReturnType>

A big plus of this syntax is that it's backwards compatible with the current syntax. Downside is you now have rules for one error in two different blocks.

You could also add a tag like this, to avoid having two blocks and still be backwards compatible:

        <MissingReturnType errorLevel="info">
            <levelOverride errorLevel="error">
                <directory name="src" />
            </levelOverride>
        </MissingReturnType>

In this example, everything would get level 'info' except things in src would get level 'error'.

Open to any other suggestions too.

Closure param types not checked

$closure_int = function(int $i) : int { return $i; };
$closure_string = function(string $s) : string { return $s; };

print $closure_int("string") . "\n";
print $closure_string(42) . "\n";

Emit MethodSignatureMismatch on non-constructor method with more args

class A {
    function f(int $a) {}
}
class B extends A {
    function f(int $a, int $b) {}
}

Check types of operators

Languages like Java don't allow mismatched types on either side of an operator. After discussing offline with @muglug, we think that to keep Psalm from being too obnoxious, there may need to be two different levels of this check.

Looser level:
'the number is ' + 12 is illegal
'the number is ' . 12 is LEGAL
1.5 + 2 is LEGAL

Stricter level:
'the number is ' + 12 is illegal
'the number is ' . 12 is illegal
1.5 + 2 is illegal
1.0 + 2.0 is LEGAL

The looser level could be in the default config.

An example of where the stricter level can catch a bug: Say you were adding money and you had $price1 = 3.5 and $price2_cents = 50. Throwing an error if you did $price1 + $price2_cents would actually catch the bug since you shouldn’t be adding dollars and cents.

Psalm does not currently support varying levels of the same check, so the syntax for that in the XML will need to be determined.

Emit NotSetInConstructor issue

Given code

class A { }
class B {
    /** @var A **/
    public $a;

    public function __construct() {
    }
}

Where property B::$a cannot be null by definition, add an error if that property is not set at the end of the constructor.

Emit issue when encountering noops

e.g.

$a = 5;
$a;

Access levels should never be overridden in subclasses

class A {
  public function foo() { }
}
class B extends A {
  protected function foo() { }
}

this should emit an issue, but does not

FailedTypeResolution emitted for docblock-declared types

/**
 * @param string $foo
 */
function foo($foo)
{
    if (!is_string($foo)) {
        throw new TypeError("test");
    }
}

This is failing in https://github.com/paragonie/sodium_compat

MissingClosureReturnType issues with @return on container element

I have the following closure, and can't seem to get Psalm to recognize it having a return type:

$container['filesystem'] = function (Container $c) {
    $adapter = new Local('/');

    return new \League\Flysystem\Filesystem($adapter);
};

I've tried adding a /** @return \League\Flysystem\Filesystem */ above it, but it's still throwing an error.

ERROR: MissingReturnType - src/Provider/Filesystem.php:18 - Method  does not have a return type
        $container['filesystem'] = function (Container $c) {

Emit issue on function redefinition

function f() {}
function f() {}

Allow a const/variable's value to be used as a type

For example,

class A {
    const CLASS = '\My\Namespace\Class';

    /**
     * @return self::CLASS
     */
    public function foo() 
    {
        // ...
    }
}

Support empty namespace declaration

namespace {
}

now emits an issue (broken by 694da2c#diff-46c13161dbea6102d55a6a3fb3f7c65dL437)

Allow negated assignment

Currently fails:

if (($row = rand(0,10) ? [] : false) !== false) {
   $row[0] = 'good';
   echo $row[0];
}

Emit issues when encountering uncaught throw

Both when

• calling a method that has @throws in its docblock and the callee doesn't also have a matching @throws
• when a method throws an exception and doesn't have a @throws in the docblock.

cc @nickyr

Psalm thinks ReflectionMethod::invoke requires two parameters

Psalm thinks ReflectionMethod::invoke requires two parameters, however the second one is optional. You have to specify the second one as null to get it to pass.

Originally I opened this on the private repo, and you said it got fixed "when we started respecting the CallMap for method calls". But it still seems to be an issue on Psalm 0.3.3

Emit issue when assigning array value on object

class A {}
(new A)["b"] = 1;

Psalm lowercases function names in output

Paste

<?php
/**
 * @param int $bar
 * @return void
 */
function fooBarBaz() {
}

into getpsalm.org.

Observed: fooBarBaz gets lowercased in the error message:

Undefined variable on assignment inside isset

<?php

$array = [];

if (isset($array[$a = microtime()])) {
    print "hello";
}

print $a;

Results in the following error, even if $a is always set here.

ERROR: UndefinedVariable - src/test.php:9 - Cannot find referenced variable $a
print $a;

Class constants not properly hoisted

This is fine

class A { const B = 42;}
$a = A::B;

But this produces an error

class A { const B = 42;}
$a = A::B;
class C {}

as we try to evaluate A::B before the class constants are hoisted.

Instead, Psalm should wait until it sees A::B before evaluating the class contents.

Protected properties in sibling subclasses cannot be accessed

This currently fails:

class A {
  protected $foo;
}

class B extends A { }

class C extends A {
  public function fooFoo() : void {
    $b = new B();
    $b->foo = "hello";
  }
}

Reflected classes do not store parent classes

Currently fails:

$e = rand(0, 10)
  ? new RuntimeException("m")
  : null;

if ($e instanceof Exception) {
  echo "good";
}

Do not emit TypeDoesNotContainType for interfaces

Should not emit error

interface A {
}
interface B {
    function foo();
}
function bar(A $a) {
    if ($a instanceof B) {
        $a->foo();
    }
}

Emit issues on deprecated properties and classes

/** @deprecated */
class A{}
(new A); //emit

and

class A{
  /**
   * @deprecated
   * @var string
   */
  public $foo;
}
echo (new A)->foo; //emit

--update-docblocks lowercases object name in array

I got a return value of array<int, namevalue> instead of array<int, NameValue> after running psalm --update-docblocks on https://github.com/vimeo/omnipay-vindicia/blob/80dbd70973c6beadb6d09d46d232964ac4c06e57/src/Message/HOAAuthorizeRequest.php#L112

Anonymous class method return types not recorded

$class = new class {
  public function f() : int {
    return 42;
  }
};

function g(int $i) : int {
  return $i;
}

$x = g($class->f());

Prevent improper composition

With Config::$use_property_default_for_type = true this emits an issue

<?php

class C1 {
    public $p = 42;
}

trait T1 {
    public $p = 'string';
}

class C2 extends C1 {
    use T1;
}

(borrowed from phan/phan#300)

InvalidPropertyAssignment - file.php:8 - $this->p with declared type 'int' cannot be assigned type 'string'

but should emit a stronger issue InvalidPropertyComposition

Old style SoapClient constructor can't be extended

Support for old style constructors was added in 0c6d07e

However, if you add this to the bottom of the test in https://github.com/vimeo/psalm/blob/master/tests/Php40Test.php :

class C extends SoapClient {
    public function __construct() {
         parent::__construct();
    }
}

it fails with InaccessibleMethod - somefile.php:19 - Cannot access method SoapClient::__construct.

Poked around in the code a bit and it looks like since Psalm doesn't check SoapClient it doesn't know to register the constructor as __construct.

Emit issue on class redefinition

class A {}
class A {}

Add support for parameterised variable checks

This code passes (if $a is an array<mixed> and Foo::bar() is a method)

if ($a['b'] instanceof Foo) {
  $a['b']->bar();
}

but this code (assuming $b is defined) emits a MixedMethodCall issue:

if ($a[$b] instanceof Foo) {
  $a[$b]->bar();
}

It currently fails because Psalm does not allow array fetches with variable-key-offsets to be considered scoped variables. But we should adapt Psalm to allow variable-key array offsets, and then remove those variables from Context::$vars_in_scope when either the array is modified or the key variable is modified.

Related (slightly) to #11

Constants inside functions are not recognised

Test case:

/**
 * @return void
 */
function defineConstant()
{
    define('CONSTANT', 1);
}

defineConstant();

echo CONSTANT;

Output:

ERROR: UndefinedConstant - test-constant.php:13 - Const CONSTANT is not defined
echo CONSTANT;

Constants are always global, so there is no error in the above script.

Suppress errors for one line only

Allow single-line issue suppression e.g.

/** @psalm-suppress UndefinedMethod **/
$foo->bar();

Including and excluding parts of the same directory doesn't work

I'm running the following config, and it doesn't appear to be properly excluding my tests/Stubs directory, because I have tests/ set up for inspection.

<?xml version="1.0"?>
<codeinspector stopOnFirstError="false" useDocblockTypes="true">
    <inspectFiles>
        <directory name="src" />
        <directory name="tests" />
    </inspectFiles>

    <excludeFiles>
        <directory name="tests/Stubs" />
    </excludeFiles>

    <fileExtensions>
        <extension name=".php" />
    </fileExtensions>
</codeinspector>

ERROR: MissingReturnType - tests/Stubs/RepresentationWithTwoFieldsButOnlyOneDocumentedStub.php:9 - Method Vimeo\Docparser\Tests\Stubs\RepresentationWithTwoFieldsButOnlyOneDocumentedStub::main does not have a return type
    protected function main()