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.

Sign up for GitHub

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

Jump to bottom

Improve int range parsing #7775

Merged
merged 5 commits into from
Mar 12, 2022
Merged

Improve int range parsing #7775

merged 5 commits into from
Mar 12, 2022

Conversation

fluffycondor
Copy link
Contributor

Fixes #7734

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Mar 11, 2022
AndrolGenhald
AndrolGenhald approved these changes Mar 11, 2022
View reviewed changes
Copy link
Collaborator

@AndrolGenhald AndrolGenhald 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 orklah merged commit c1ad5f6 into vimeo:4.x Mar 12, 2022
@orklah
Copy link
Collaborator

orklah commented Mar 12, 2022

Thanks!

@orklah
Copy link
Collaborator

orklah commented Mar 14, 2022

@fluffycondor the previous code was suboptimal and your enhancement made it apparent. Turns out $param1_union_types[0]->value contains the current namespace along with min or max.

For example, Psalm\max. It showed when we tried to merge this onto master (who's using more integer ranges in Psalm code itself).

This made parsing of int<0, max> fail when we're in a namespace. Could you take a look at it and pushing a fix? :)

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Mar 14, 2022 •
edited

I think the best way to go about this would be to use $parse_tree->children directly when doing the BOUND_MIN/MAX comparison, but I haven't looked into it too deeply. Here's a test that fails.

AndrolGenhald added a commit to AndrolGenhald/psalm that referenced this pull request Mar 30, 2022
@AndrolGenhald
Fix namespaced min/max int range keyword issue introduced in vimeo#7775.
0bdf942
orklah added a commit that referenced this pull request Mar 31, 2022
@orklah
Merge pull request #7829 from AndrolGenhald/bugfix/int-range-namespac…
1fc5c1c 
…ed-keywords

Fix namespaced min/max int range keyword issue introduced in #7775
muglug added a commit that referenced this pull request Apr 27, 2022
@muglug
Fix namespaced min/max int range keyword issue introduced in #7775.
a9e3c1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orklah orklah orklah left review comments

@AndrolGenhald AndrolGenhald AndrolGenhald approved these changes

Assignees
No one assigned
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Psalm treats any string literal as a "max" in an int range
3 participants
@fluffycondor @orklah @AndrolGenhald