Setup static analysis with Psalm

[greg0ire]

Q	A
Type	improvement
BC Break	no
Fixed issues	n/a

Summary

This PR adds static analysis with Psalm, configured to the most lenient level, and ignores every error.
I am targeting 2.10.x, because this change does not touch code in lib, or dependencies under require.

[greg0ire]

I think it will not show up until merged, but the result can already be preview on this PR I made in my fork

[greg0ire]

Travis fails because the version of Psalm I installed is too recent. I will add something in composer.json to make sure we install something supported on 7.2

[morozov]

I'm surprised that we have so few errors to whitelist of which most are related to the DB extensions. BTW, why doesn't it report the existing mixed[][] return types?

[greg0ire]

I'm surprised that we have so few errors to whitelist of which most are related to the DB extensions. BTW, why doesn't it report the existing mixed[][] return types?

That's probably because I went for the most lenient level (8)

EDIT: or because the tests directory is not included :P

[morozov]

or because the tests directory is not included :P

I see. Is it because we also don't run PHPStan against tests? Do you want to add it in master? Mind that someone will have to up-merge the new dependency and update the baseline. And that someone will be you @greg0ire :-)

[greg0ire]

It was both reasons: the issues is reported starting at level 6.

I can add psalm to all branches when doing the upmerges, But I'm not sure if that's what you meant, I'm under the impression that you would like me to add it in master and then do the upmerge, is that correct? And if yes, why?

[morozov]

No, once this PR gets merged, we'll need to merge 2.10.x → 2.11.x → 3.0.x → master to get the changes propagated. And given that each of the branches is different in the underlying code, each up-merge PR may need to adjust the baseline (e.g. change the paths, include new exceptions or remove the irrelevant ones).

[greg0ire]

Ok, I'll take care of it :)

[morozov]

Great work by the way, @greg0ire! I'm glad that we eventually have Psalm in the build pipeline.

[greg0ire]

Thanks me, too :)

[ondrejmirtes]

@morozov Why is baseline fine for you with Psalm, but not with PHPStan? Linking our discussion here: #3799 (comment)

[greg0ire]

@ondrejmirtes we do have a baseline for PHPStan:

dbal/phpstan.neon.dist

Lines 11 to 74 in 72d9081

	# extension not available
	- '~^(Used )?(Function|Constant) sasql_\S+ not found\.\z~i'
	# removing it would be BC break
	- '~^Constructor of class Doctrine\\DBAL\\Schema\\Table has an unused parameter \$idGeneratorType\.\z~'
	# declaring $tableName in AbstractSchemaManager::_getPortableTableIndexesList() non-optional will be a BC break
	- '~^Parameter #2 \$table of class Doctrine\\DBAL\\Event\\SchemaIndexDefinitionEventArgs constructor expects string, string\|null given\.\z~'
	# changing these would be a BC break, to be done in next major
	- "~^Casting to bool something that's already bool.~"
	- "~^Casting to int something that's already int.~"
	- '~^Method Doctrine\\DBAL\\Driver\\IBMDB2\\DB2Connection::exec\(\) should return int but returns bool\.\z~'
	- '~^Property Doctrine\\DBAL\\Schema\\Table::\$_primaryKeyName \(string\) does not accept (default value of type )?false\.\z~'
	- '~^Property Doctrine\\DBAL\\Schema\\Schema::\$_schemaConfig \(Doctrine\\DBAL\\Schema\\SchemaConfig\) does not accept default value of type false\.\z~'
	- '~^Method Doctrine\\DBAL\\Schema\\ForeignKeyConstraint::onEvent\(\) should return string\|null but returns false\.\z~'
	- '~^Method Doctrine\\DBAL\\Schema\\(Oracle|PostgreSql|SQLServer)SchemaManager::_getPortableTableDefinition\(\) should return array but returns string\.\z~'
	- '~^Method Doctrine\\DBAL\\Driver\\OCI8\\OCI8Connection::lastInsertId\(\) should return string but returns (int|false)\.\z~'
	- '~^Method Doctrine\\DBAL\\Driver\\SQLSrv\\SQLSrvConnection::errorCode\(\) should return string\|null but returns false\.\z~'
	# https://bugs.php.net/bug.php?id=78126
	- '~^Call to an undefined method Doctrine\\DBAL\\Driver\\PDOConnection::sqliteCreateFunction\(\)\.\z~'
	# https://github.com/phpstan/phpstan/issues/1847
	- '~^Parameter #2 \$registeredAliases of static method Doctrine\\DBAL\\Query\\QueryException::unknownAlias\(\) expects array<string>, array<int, int\|string> given\.\z~'
	- '~^Parameter #2 \$registeredAliases of static method Doctrine\\DBAL\\Query\\QueryException::nonUniqueAlias\(\) expects array<string>, array<int, int\|string> given\.\z~'
	# PHPStan is too strict about preg_replace(): https://phpstan.org/r/993dc99f-0d43-4b51-868b-d01f982c1463
	- '~^Method Doctrine\\DBAL\\Platforms\\AbstractPlatform::escapeStringForLike\(\) should return string but returns string\|null\.\z~'
	# legacy variadic-like signature
	- '~^Method Doctrine\\DBAL(\\.*)?Connection::query\(\) invoked with \d+ parameters?, 0 required\.\z~'
	# some drivers actually do accept 2nd parameter...
	- '~^Method Doctrine\\DBAL\\Platforms\\AbstractPlatform::getListTableForeignKeysSQL\(\) invoked with \d+ parameters, 1 required\.\z~'
	# legacy remnants from doctrine/common
	- '~^Class Doctrine\\Common\\(Collections\\Collection|Persistence\\Proxy) not found\.\z~'
	- '~^.+ on an unknown class Doctrine\\Common\\(Collections\\Collection|Persistence\\Proxy)\.\z~'
	# inheritance variance inference issue
	- '~^Method Doctrine\\DBAL\\Driver\\PDOConnection::\w+\(\) should return Doctrine\\DBAL\\Driver\\Statement but returns PDOStatement\.\z~'
	# may not exist when pdo_sqlsrv is not loaded but PDO is
	- '~^Access to undefined constant PDO::SQLSRV_ENCODING_BINARY\.\z~'
	# weird class name, represented in stubs as OCI_(Lob|Collection)
	- '~unknown class OCI-(Lob|Collection)~'
	# https://github.com/JetBrains/phpstorm-stubs/pull/766
	- '~^Method Doctrine\\DBAL\\Driver\\Mysqli\\MysqliStatement::_fetch\(\) never returns null so it can be removed from the return typehint\.$~'
	# The ReflectionException in the case when the class does not exist is acceptable and does not need to be handled
	- '~^Parameter #1 \$argument of class ReflectionClass constructor expects class-string<T of object>\|T of object, string given\.$~'
	# https://github.com/phpstan/phpstan/issues/3132
	-
	message: '~^Call to function in_array\(\) with arguments Doctrine\\DBAL\\Schema\\Column, array<string> and true will always evaluate to false\.$~'
	path: %currentWorkingDirectory%/lib/Doctrine/DBAL/Schema/Table.php
	# https://github.com/phpstan/phpstan/issues/3133
	-
	message: '~^Cannot cast array<string>\|bool\|string\|null to int\.$~'
	path: %currentWorkingDirectory%/lib/Doctrine/DBAL/Tools/Console/Command/RunSqlCommand.php

[ondrejmirtes]

This is not a baseline in the sense we talked about in the linked discussion :) PHPStan's baseline is generated with --generate-baseline: https://phpstan.org/user-guide/baseline

[greg0ire]

TIL :)

[morozov]

Why is baseline fine for you with Psalm, but not with PHPStan? Linking our discussion here: #3799 (comment)

This is a valid question. I didn't think of it because we were using the lowest possible level and the baseline was way smaller than in #3799. Although, it is getting out of hand (I don't remember seeing occurrences="93" in the very first patch).

@greg0ire before we proceed with merging it to master, let's take a pause and see if the baselined errors could be fixed (3.0 allows for BC breaks) and the non-fixable ones are suppressed manually.

[greg0ire]

@morozov I don't know if you noticed but the baseline is way smaller for master: https://github.com/doctrine/dbal/pull/3954/files#diff-043e2f3614861e20eb0fb277052a7693, but sure, I will have a look at 3.0.x

[greg0ire]

The first error I noticed does not imply a BC-break, and gets us rid of ~ 100 errors, so I'm going to have a look at 2.10.x first

[morozov]

I noticed that but I didn't think of them as the finally required baselines. It's a good sign, on the one hand, meaning that we did a good job there. But on the other hand, we still need to get rid of those many repetitive occurrences of the same error. Otherwise, each time we modify something, we'll hit the same error again.

[greg0ire]

Ok so now I'm getting a crash on 2.10.x that I don't get with the deps of 3.0.x… 🤦‍♂️

Uncaught TypeError: Return value of PhpParser\Comment::getFilePos() must be of the type int, null returned in /home/greg/dev/doctrine-dbal/vendor/nikic/php-parser/lib/PhpParser/Comment.php:53
Stack trace:
#0 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Diff/ClassStatementsDiffer.php(78): PhpParser\Comment->getFilePos()
#1 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Diff/AstDiffer.php(53): Psalm\Internal\Diff\ClassStatementsDiffer::Psalm\Internal\Diff\{closure}(Object(PhpParser\Node\Stmt\ClassMethod), Object(PhpParser\Node\Stmt\ClassMethod), '<?php\n\nnamespac...', '<?php\n\nnamespac...', false)
#2 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Diff/ClassStatementsDiffer.php(45): Psalm\Internal\Diff\AstDiffer::calculateTrace(Object(Closure), Array, Array, '<?php\n\nnamespac...', '<?php\n\nnamespac...')
#3 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Diff/NamespaceStatementsDiffer.php(98): Psalm\Internal\Diff\ClassStatementsDiffer::diff('Doctrine\\DBAL\\S...', Array, Array, '<?php\n\nnamespac...', '<?php\n\nnamespac...')
#4 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Diff/FileStatementsDiffer.php(92): Psalm\Internal\Diff\NamespaceStatementsDiffer::diff('Doctrine\\DBAL\\S...', Array, Array, '<?php\n\nnamespac...', '<?php\n\nnamespac...')
#5 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Provider/StatementsProvider.php(179): Psalm\Internal\Diff\FileStatementsDiffer::diff(Array, Array, '<?php\n\nnamespac...', '<?php\n\nnamespac...')
#6 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Scanner/FileScanner.php(68): Psalm\Internal\Provider\StatementsProvider->getStatementsForFile('/home/greg/dev/...', Object(Psalm\Progress\DefaultProgress))
#7 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Scanner.php(586): Psalm\Internal\Scanner\FileScanner->scan(Object(Psalm\Codebase), Object(Psalm\Storage\FileStorage), false, Object(Psalm\Progress\DefaultProgress))
#8 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Scanner.php(367): Psalm\Internal\Codebase\Scanner->scanFile('/home/greg/dev/...', Array, true)
#9 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Scanner.php(475): Psalm\Internal\Codebase\Scanner->Psalm\Internal\Codebase\{closure}(0, '/home/greg/dev/...')
#10 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Scanner.php(325): Psalm\Internal\Codebase\Scanner->scanFilePaths(1)
#11 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Codebase.php(477): Psalm\Internal\Codebase\Scanner->scanFiles(Object(Psalm\Internal\Codebase\ClassLikes), 3)
#12 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(519): Psalm\Codebase->scanFiles(3)
#13 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/src/psalm.php(588): Psalm\Internal\Analyzer\ProjectAnalyzer->check('/home/greg/dev/...', false)
#14 /home/greg/dev/doctrine-dbal/vendor/vimeo/psalm/psalm(2): require_once('/home/greg/dev/...')
#15 {main}
(Psalm 3.11.2@d470903722cfcbc1cd04744c5491d3e6d13ec3d9 crashed due to an uncaught Throwable)

EDIT: Works after composer update nikic/php-parser… moving on.