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

Resolve PHP_INT_MAX as positive integer #684

Merged
merged 3 commits into from Sep 22, 2021
Merged

Resolve PHP_INT_MAX as positive integer #684

merged 3 commits into from Sep 22, 2021

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Sep 22, 2021

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
Contributor Author

herndlm commented Sep 22, 2021

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
Copy link
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:

- 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!

src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved
@herndlm
Copy link
Contributor Author

herndlm commented Sep 22, 2021

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
Member

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
Member

Thank you!

@herndlm herndlm deleted the feature/resolve-php-int-max branch September 22, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants