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

1> Range arithmetics and assertions #6241

Merged
merged 53 commits into from
Sep 20, 2021
Merged

1> Range arithmetics and assertions #6241

merged 53 commits into from
Sep 20, 2021

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Aug 4, 2021

This PR is the continuation of #6210. It should fix some issues introduced in it due to assertions.

However, I underestimated this task. There are a few points I don't know how to fix and I'll probably need help for that.

If we need to make a new release, I think #6210 should be reverted to prevent creating false positives in builds. I'll reintegrate in this PR later

EDIT:
this fixes #5635
this fixes #4972
this fixes #6488
this fixes #6467

@weirdan
Copy link
Collaborator

weirdan commented Aug 12, 2021

I'm planning to cut a new release this weekend. Do you reckon I'll need to revert #6210 ?

@orklah
Copy link
Collaborator Author

orklah commented Aug 13, 2021

Yes, please do. I tried to work on this since then and I have obstacles everywhere. Better do it now...

@orklah orklah force-pushed the range5 branch 3 times, most recently from 7342fe0 to 64979f3 Compare August 15, 2021 15:37
@orklah orklah changed the title Range arithmetics and loops Range arithmetics and assertions Aug 15, 2021
@orklah
Copy link
Collaborator Author

orklah commented Aug 15, 2021

I put aside loop features for now, they're pretty complex and they're not required for now.

I also put aside the TPositiveInt deprecation, this PR should have less impact now because I kept TPositiveInt first for now when applicable.

This PR should be ok, except for a test here:

'dontEraseNullAfterGreaterThanCheck' => [

I don't understand why null should be preserved through the conditional given it's always false: https://3v4l.org/K2Ed1

The next step will be to remove TPositiveInt from assertions while still keeping everythink working.

@orklah orklah requested review from muglug and weirdan and removed request for muglug August 15, 2021 15:57
@orklah
Copy link
Collaborator Author

orklah commented Aug 15, 2021

Note: please merge #6306 before this.

@orklah orklah marked this pull request as ready for review August 15, 2021 19:25
@weirdan
Copy link
Collaborator

weirdan commented Aug 15, 2021

I don't understand why null should be preserved through the conditional given it's always false

That doesn't make sense for me either, not with 0 anyway.

@orklah orklah marked this pull request as draft August 20, 2021 11:16
@orklah orklah marked this pull request as ready for review August 20, 2021 11:37
@orklah
Copy link
Collaborator Author

orklah commented Aug 29, 2021

@weirdan can we maybe do a new release and merge this after?

@weirdan
Copy link
Collaborator

weirdan commented Aug 31, 2021

@orklah sure, will do this weekend.

@orklah orklah force-pushed the range5 branch 4 times, most recently from 7259b1e to 3415888 Compare September 4, 2021 21:56
@orklah
Copy link
Collaborator Author

orklah commented Sep 5, 2021

the CI is currently failing on phpunit.

this is due to a particular construction like this:

function scope(int $a) {
    const A = array('a', 'b');

    $result = A[$i % count(A)];
}

Before this PR, Psalm was infering $i % count(A) to int. Now that the inference has improved, it's infered to int<-1, 1>, because we know the count(A) will return 2.

However, in Psalm A[$b] is tolerated when $b is a positive-int or an int, but it's not when $b is an integer range (this is deliberate as it's one of the way integer range can find new bugs).

The issue in phpunit can be fixed by adding phpdoc to $i that it's a positive-int, this way Psalm will infer the modulo result as int<0, 1> and it will be compatible with expected array-keys 0|1.

I think we can merge this now

@orklah
Copy link
Collaborator Author

orklah commented Sep 5, 2021

I tried to document as much as I could directly in the code, but I'll make a summary here of what the reason for each modified file is for review and future reference purposes:

  • AssertionFinder.php: I introduced two big new assertions: >X and <X to infer ranges. They replaced incomplete previous assertions like hasPositiveNumberCheck and hasZeroCheck.
  • NonDivArithmeticOpAnalyzer.php: I allowed Psalm to make literal arithmetic operations between two integer range and between integer range and other int (this is what improved the modulo operation I was talking about above)
  • IntegerRangeComparator.php: this is an object dedicated to check if an Integer range can be contained by other types. Either in another IntegerRange or even in a bunch of other integer types combined. This is quite a new notion in Psalm to use multiple types to satisfy one
  • ScalarTypeComparator.php: this checks whether another integer type can be contained by an integer range
  • UnionTypeComparator.php: this one is the step before the two above files. One piece of code is designed to call IntegerRangeComparator to handle the multiple ints that can contain an integer range. The other is a special case needed because until now, Psalm considered two types as "contained by" when their atomics could be identical, but this is not exactly true anymore with integer range (see comment for more info)
  • SimpleAssertionReconciler.php: it is the file that process what we found in AssertionFinder. It takes the known type for a variable and will try to apply new assertions. I commented some piece of code related to loops because it's a very complex topic we'll have to come back to after this one is merged. Also there are some places where we could emit issues that I intend to add in another PR
  • TypeCombiner.php: this is the file that is used to make multiple types into one. It remove redundancies and expand existing types instead of just putting them altogether
  • TIntRange.php: I added some useful functions to recalculate bounds or to check if an integer is contained in the range
  • Union.php: hasInt is now true if there is range in the union, isAlwaysTruthy can be true for an IntegerRange(I decommented a piece of code I introduced in a previous PR) and I added a useful function to fetch all ranges

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

LGTM. @orklah, feel free to merge.

@orklah orklah merged commit 90e1662 into vimeo:master Sep 20, 2021
@weirdan
Copy link
Collaborator

weirdan commented Sep 20, 2021

Congrats on your first merge! 🎆

@orklah
Copy link
Collaborator Author

orklah commented Sep 20, 2021

Thanks! 🥳

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
2 participants