Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Catch implicit conversions in mod operator #2466

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

Open
ryium wants to merge 7 commits into phpstan:1.11.x
base: 1.11.x
Choose a base branch
Loading
from ryium:ryium/issue/8288-op-mod

Conversation

@ryium
Copy link
Contributor

@ryium ryium commented Jun 17, 2023
edited
Loading

Catch deprecated implicit type conversions that occur in PHP 8.1 and later.
The following BinaryOp that may cause deprecation.

This PR is part of phpstan/phpstan#8288 and change regarding the mod operator of #2450

(注記)O = safe, D = deprecated, X = not safe

% Mod

L \ R int float string numeric-string Stringable array
int O D X D X X
float D D X D X X
string X X X X X X
numeric-string D D X D X X
Stringable X X X X X X
array X X X X X X

Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

Copy link
Member

@ondrejmirtes ondrejmirtes Jun 17, 2023

Choose a reason for hiding this comment

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

How the type system responds shouldn't change because it's not actually true. The % operator still returns a number but only on some PHP versions it throws a deprecation notice.

Copy link
Member

@ondrejmirtes ondrejmirtes Jun 17, 2023

Choose a reason for hiding this comment

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

So just change the rule please. The condition should be written differently.

  1. Please verify that all the X from table here Catch implicit conversions in mod operator #2466 are reported with 'Binary operation "%s" between %s and %s results in an error.'.
  2. Please write tests that all the D are reported with 'Deprecated in PHP 8.1: Implicit conversion from %s to %s loses precision.'.
  3. Please note that types can for example be int|float or mixed. You should be able to take advantage of RuleLevelHelper so that something that's always wrong like int % float is always reported, something that might be wrong like int % float|int is reported on level 7 and int % mixed is reported on level 9.

Please see how RuleLevelHelper::findTypeToCheck is used in many existing rules like InvalidCastRule. Basically there's a callback that checks the type and transforms the type based on the rule level. Then there's $type instanceof ErrorType with early return and then there's the callback called again on the transformed type.

@ryium ryium force-pushed the ryium/issue/8288-op-mod branch 2 times, most recently from 340a3ed to 264bbb3 Compare December 15, 2023 14:22
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.

I like this much more! 👍

But the rule should report this only on 8.1+. Please add a new method in PhpVersion and ask about it in the rule.

ryium reacted with thumbs up emoji
@ryium ryium force-pushed the ryium/issue/8288-op-mod branch from e5e12f7 to 02eaab3 Compare February 2, 2024 12:14
@ryium ryium force-pushed the ryium/issue/8288-op-mod branch from fcda140 to 07916c7 Compare April 16, 2024 07:51
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.

AltStyle によって変換されたページ (->オリジナル) /