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

Add changes from Psalm/PHPStan #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

muglug
Copy link
Contributor

@muglug muglug commented Dec 30, 2019

No description provided.

Copy link
Member

@TysonAndre TysonAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will cherry-pick these changes when I have the time to do this.

I also need to update the version in this repo to mirror phan at some point, and update this from the 7.3 callmap to the 7.4 callmap.

I've found it easier to try to apply deltas from the last few months in git than copy all deltas that look relevant (but some older changes were probably missed).

@@ -535,7 +536,7 @@
'bcompiler_write_included_filename' => ['bool', 'filehandle'=>'resource', 'filename'=>'string'],
'bcpow' => ['string', 'base'=>'string', 'exponent'=>'string', 'scale='=>'int'],
'bcpowmod' => ['?string', 'base'=>'string', 'exponent'=>'string', 'modulus'=>'string', 'scale='=>'int'],
'bcscale' => ['bool', 'scale='=>'int'],
'bcscale' => ['int', 'scale='=>'int'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -1694,19 +1695,19 @@
'CURLFile::getPostFilename' => ['string'],
'CURLFile::setMimeType' => ['void', 'mime'=>'string'],
'CURLFile::setPostFilename' => ['void', 'name'=>'string'],
'current' => ['mixed|false', 'array_arg'=>'array|object'],
'current' => ['mixed|false', 'array_arg'=>'array'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's deprecated to use current that way, and I wouldn't use it that way, but object is still a valid type for this in php 7

php > $x = (object)['a' => 2, 'b' => 3];
php > var_export(current($x));
2

@@ -1762,14 +1763,14 @@
'DatePeriod::getDateInterval' => ['DateInterval'],
'DatePeriod::getEndDate' => ['?DateTimeInterface'],
'DatePeriod::getStartDate' => ['DateTimeInterface'],
'DateTime::__construct' => ['void', 'time='=>'?string', 'timezone='=>'?DateTimeZone'],
'DateTime::__construct' => ['void', 'time='=>'string', 'timezone='=>'?DateTimeZone'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new DateTime(null) seems to work in 7.3 - I could conceivably pass null to get now in $timezone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the idea is that it's almost always programmer error (sort of like get_class(null))

@@ -1781,7 +1782,7 @@
'DateTime::setTimestamp' => ['static', 'unixtimestamp'=>'int'],
'DateTime::setTimezone' => ['static', 'timezone'=>'DateTimeZone'],
'DateTime::sub' => ['static', 'interval'=>'DateInterval'],
'DateTimeImmutable::__construct' => ['void', 'time='=>'?string', 'timezone='=>'?DateTimeZone'],
'DateTimeImmutable::__construct' => ['void', 'time='=>'string', 'timezone='=>'?DateTimeZone'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, might be programmer error that we want to flag.

'exif_tagname' => ['string|false', 'index'=>'int'],
'exif_thumbnail' => ['string|false', 'filename'=>'string', '&w_width='=>'int', '&w_height='=>'int', '&w_imagetype='=>'int'],
'exit' => ['', 'status'=>'string|int'],
'exp' => ['float', 'number'=>'float'],
'expect_expectl' => ['int', 'expect'=>'resource', 'cases'=>'array', 'match='=>'array'],
'expect_popen' => ['resource', 'command'=>'string'],
'explode' => ['array<int,string>', 'separator'=>'string', 'str'=>'string', 'limit='=>'int'],
'explode' => ['array<int,string>|false', 'separator'=>'string', 'str'=>'string', 'limit='=>'int'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phan is deliberately excluding most of the false return types for anything that doesn't involve disk/network IO - merging these changes would just result in too many false positives with phan --strict-type-checking.

PHP 8 being released and throwing runtime errors for many issues caused by invalid parameters will be a relief (at least for analyzing return types).

@@ -5978,13 +5980,13 @@
'imap_utf8' => ['string', 'mime_encoded_text'=>'string'],
'imap_utf8_to_mutf7' => ['string|false', 'in'=>'string'],
'implode' => ['string', 'glue'=>'string', 'pieces'=>'array'],
'implode\'1' => ['string', 'pieces'=>'array', 'glue='=>'string'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was/was going to be added to the deltas for 8.0 - phan warns about this separately

@@ -6029,9 +6031,9 @@
'ingres_rollback' => ['bool', 'link'=>'resource'],
'ingres_set_environment' => ['bool', 'link'=>'resource', 'options'=>'array'],
'ingres_unbuffered_query' => ['mixed', 'link'=>'resource', 'query'=>'string', 'params='=>'array', 'types='=>'string'],
'ini_alter' => ['string|false', 'varname'=>'string', 'newvalue'=>'string|int|float|bool'],
'ini_alter' => ['string|false', 'varname'=>'string', 'newvalue'=>'string'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@@ -8845,7 +8847,7 @@
'newt_win_message' => ['void', 'title'=>'string', 'button_text'=>'string', 'format'=>'string', 'args='=>'mixed', '...args='=>'mixed'],
'newt_win_messagev' => ['void', 'title'=>'string', 'button_text'=>'string', 'format'=>'string', 'args'=>'array'],
'newt_win_ternary' => ['int', 'title'=>'string', 'button1_text'=>'string', 'button2_text'=>'string', 'button3_text'=>'string', 'format'=>'string', 'args='=>'mixed', '...args='=>'mixed'],
'next' => ['mixed', '&rw_array_arg'=>'array|object'],
'next' => ['mixed', '&rw_array_arg'=>'array'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as current

@@ -15152,7 +15160,8 @@
'VarnishLog::getTagName' => ['string', 'index'=>'int'],
'VarnishStat::__construct' => ['void', 'args='=>'array'],
'VarnishStat::getSnapshot' => ['array'],
'version_compare' => ['int|bool', 'ver1'=>'string', 'ver2'=>'string', 'oper='=>'\'\x3c\'|\'lt\'|\'\x3c=\'|\'le\'|\'\x3e\'|\'gt\'|\'\x3e=\'|\'ge\'|\'==\'|\'=\'|\'eq\'|\'!=\'|\'\x3c\x3e\'|\'ne\''],
'version_compare' => ['bool', 'ver1'=>'string', 'ver2'=>'string', 'oper'=>'\'\x3c\'|\'lt\'|\'\x3c=\'|\'le\'|\'\x3e\'|\'gt\'|\'\x3e=\'|\'ge\'|\'==\'|\'=\'|\'eq\'|\'!=\'|\'\x3c\x3e\'|\'ne\''],
'version_compare\'1' => ['int', 'ver1'=>'string', 'ver2'=>'string', 'oper='=>''],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: The third oper should probably be removed from the alternate signature in psalm.

php > var_export(version_compare(1, 2, null));
true
php > var_export(version_compare(1, 2, false));
true
php > var_export(version_compare(1, 2, ''));
true

Phan currently does a poor job of pattern matching alternate signatures (in the backlog), which is why many of these signatures are combined in Phan's version

'ssh2_sftp_rename' => ['bool', 'sftp'=>'resource', 'from'=>'string', 'to'=>'string'],
'ssh2_sftp_rmdir' => ['bool', 'sftp'=>'resource', 'dirname'=>'string'],
'ssh2_sftp_stat' => ['array|false', 'sftp'=>'resource', 'path'=>'string'],
'ssh2_sftp_symlink' => ['bool', 'sftp'=>'resource', 'target'=>'string', 'link'=>'string'],
'ssh2_sftp_unlink' => ['bool', 'sftp'=>'resource', 'filename'=>'string'],
'ssh2_shell' => ['resource', 'session'=>'resource', 'term_type='=>'string', 'env='=>'array', 'width='=>'int', 'height='=>'int', 'width_height_type='=>'int'],
'ssh2_tunnel' => ['resource', 'session'=>'resource', 'host'=>'string', 'port'=>'int'],
'ssh2_shell' => ['resource|false', 'session'=>'resource', 'term_type='=>'string', 'env='=>'array', 'width='=>'int', 'height='=>'int', 'width_height_type='=>'int'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants