Skip to content

Resolve PHP_INT_MAX as positive integer#684

Merged
ondrejmirtes merged 3 commits intophpstan:masterfrom
herndlm:feature/resolve-php-int-max
Sep 22, 2021
Merged

Resolve PHP_INT_MAX as positive integer#684
ondrejmirtes merged 3 commits intophpstan:masterfrom
herndlm:feature/resolve-php-int-max

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Sep 22, 2021 •
edited
Loading

Fixes phpstan/phpstan#5657

This is my first PR, I definitely don't know enough about phpstan internals and might have missed something.

I was no sure what this should resolve to. It would be enough and best to set it to "positive int" but I think this concept is not existing, is it? I then chose the positive range because

  1. Hard-coding it to PHP_INT_MAX feels not correct as that would be the value of the platform phpstan is running on
  2. Setting it to something like 1 could make even more problems
  3. Setting it to the UnionType of what the most common values for 32 and 64 bit might make problems (e.g. the 64 bit code will not work on 32 bit I assume?)

Initially I tried to be smart and dynamically resolve all int constant via constant but realised that this a bad idea because of the already mentioned 1. What do you think?

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 22, 2021 •
edited
Loading

Ok well, looks like I broke things anyway :) I'll look into that. Discovered the makefile too late..
UPDATE: I know what the problem is and figured out the type I need is existing :)

@herndlm herndlm changed the title Resolve PHP_INT_MAX Resolve PHP_INT_MAX as positive integer Sep 22, 2021
ondrejmirtes
ondrejmirtes requested changes Sep 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This PR is mostly fine, thank you :) Also please note there are other constants that would benefit from the same treatment, for example PHP_INT_MIN is definitely one of them but there are many more.

I'd appreciate if you could go through this list and created more specific types:

phpstan-src/conf/config.neon

Lines 113 to 157 in 43c192b

- ICONV_IMPL
- LIBXML_VERSION
- LIBXML_DOTTED_VERSION
- PHP_VERSION
- PHP_MAJOR_VERSION
- PHP_MINOR_VERSION
- PHP_RELEASE_VERSION
- PHP_VERSION_ID
- PHP_EXTRA_VERSION
- PHP_WINDOWS_VERSION_MAJOR
- PHP_WINDOWS_VERSION_MINOR
- PHP_WINDOWS_VERSION_BUILD
- PHP_ZTS
- PHP_DEBUG
- PHP_MAXPATHLEN
- PHP_OS
- PHP_OS_FAMILY
- PHP_SAPI
- PHP_EOL
- PHP_INT_MAX
- PHP_INT_MIN
- PHP_INT_SIZE
- PHP_FLOAT_DIG
- PHP_FLOAT_EPSILON
- PHP_FLOAT_MIN
- PHP_FLOAT_MAX
- DEFAULT_INCLUDE_PATH
- PEAR_INSTALL_DIR
- PEAR_EXTENSION_DIR
- PHP_EXTENSION_DIR
- PHP_PREFIX
- PHP_BINDIR
- PHP_BINARY
- PHP_MANDIR
- PHP_LIBDIR
- PHP_DATADIR
- PHP_SYSCONFDIR
- PHP_LOCALSTATEDIR
- PHP_CONFIG_FILE_PATH
- PHP_CONFIG_FILE_SCAN_DIR
- PHP_SHLIB_SUFFIX
- PHP_FD_SETSIZE
- OPENSSL_VERSION_NUMBER
- ZEND_DEBUG_BUILD
- ZEND_THREAD_SAFE

Integers can be made more specific using IntegerRangeType, strings can have AccessoryNonEmptyStringType and AccessoryNumericStringType attached to them in IntersectionType.

I'd like if you'd find some time to do it in the next PR after this one is merged. Thank you!

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 22, 2021 •
edited
Loading

This PR is mostly fine, thank you :) Also please note there are other constants that would benefit from the same treatment, for example PHP_INT_MIN is definitely one of them but there are many more.

Right, I thought about that, but wanted to keep it simple and small here. How do you prefer the PRs for those? E.g. all in one, or grouped by affected area or type. It depends I guess..

Also: I tried to add PHP_INT_MIN too with the same test case basically but using PHP_INT_MIN instead of PHP_INT_MAX which should then fail. Interestingly phpstan seemed to end up in an endless loop then, or I did no wait long enough, but how would you debug that?

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Sep 22, 2021

The other constants should be tested using NodeScopeResolverTest and the assertType function. You can group them together in the next PR after this one is merged :)

how would you debug that?

No idea why that would be happening. You should isolate the problem to a smallest file possible, run that with bin/phpstan analyse separately and then go through the code and figure out where the infinite run comes from.

@ondrejmirtes ondrejmirtes merged commit 0cde73f into phpstan:master Sep 22, 2021
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Sep 22, 2021

Thank you!

@herndlm herndlm deleted the feature/resolve-php-int-max branch September 22, 2021 19:06
@herndlm herndlm mentioned this pull request Sep 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FP: PHP_INT_MAX cannot be used as a default of a positive-int parameter

2 participants

@herndlm @ondrejmirtes