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

ConcatAnalyzer improvements and non-falsy-string fixes. #5544

Merged
merged 5 commits into from
Apr 1, 2021

Conversation

AndrolGenhald
Copy link
Collaborator

Deduplicate code.
Improve type inference.
Allow literal type inference when only one side has multiple types (fixes #5483).
Fix invalid type inference with negative int as right operand.

Deduplicate code.
Improve type inference.
Allow literal type inference when only one side has multiple types (fixes vimeo#5483).
Fix invalid type inference with negative int as right operand.
@AndrolGenhald
Copy link
Collaborator Author

I'm tempted to offload most of the analysis here to CastAnalyzer, and since PHP (probably?) handles concatenation by casting everything to string first InvalidCast might be more applicable than InvalidOperand anyway. The only downside I see is that users would have to update their suppressions.

@muglug @weirdan thoughts?

@AndrolGenhald
Copy link
Collaborator Author

Heads up @orklah, since you added the int concatenation to numeric string inference you may want to be aware that this now requires the right operand to be a non-negative int.

@orklah
Copy link
Collaborator

orklah commented Mar 31, 2021

Heads up @orklah, since you added the int concatenation to numeric string inference you may want to be aware that this now requires the right operand to be a non-negative int.

oh nice, didn't thought of that one at the time

@AndrolGenhald
Copy link
Collaborator Author

I stumbled on some issues with non-falsy-string while fixing newly detected issues in the codebase, so I fixed those as well.

@AndrolGenhald AndrolGenhald changed the title ConcatAnalyzer improvements. ConcatAnalyzer improvements and non-falsy-string fixes. Mar 31, 2021
@muglug muglug merged commit d022910 into vimeo:master Apr 1, 2021
@muglug
Copy link
Collaborator

muglug commented Apr 1, 2021

Thanks! This is a great addition/refactor!

I wonder whether it might be better in the long run to make, "empty", "falsy" and "lowercase" attributes of a string type, with rules about which attributes can intersect. That might make the logic more complex though, not less.

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.

assert does not constrain to string union to evaluate accessed value type under array key
3 participants