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

Add rules for operands in arithmetic operations #15

Merged
merged 1 commit into from
May 17, 2018

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented May 4, 2018

  • Require array or int/float operands in +.
  • Require int/float operands in -/*///**/%.

Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

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

Maybe we can extend it to +=, -=, ++, etc

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 4, 2018

If this one gets merged, I'd like to also add support for:

  • ++/--/+/-
  • +=/-=/*=//=/**=/%=
  • bitwise operators / shifts / assignments


public static function isValidForArithmeticOperation(Type $leftType, Type $rightType): bool
{
if (!$leftType instanceof IntegerType && !$leftType instanceof FloatType) {

Choose a reason for hiding this comment

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

This needs to use isSubtypeOf instead of instanceof.

Copy link
Contributor Author

@Majkl578 Majkl578 May 4, 2018

Choose a reason for hiding this comment

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

I guess I am a bit lost in this undocumented typesystem. :)

isSubtypeOf() is defined on CompoundType while Rule only accepts Type.

I tried:

$acceptedType = new UnionType([new IntegerType(), new FloatType()]);
return $acceptedType->accepts($leftType) && $acceptedType->accepts($rightType);

With no lucḱ (got bunch of internal errors).

Choose a reason for hiding this comment

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

Sorry, I meant isSuperTypeOf (isSubTypeOf is mostly internal helper used for inversion of control). accepts() will also work if you want to allow mixed.

I tried (…) got bunch of internal errors

That's odd. The code looks fine. What errors did you get?

Copy link
Contributor Author

@Majkl578 Majkl578 May 4, 2018

Choose a reason for hiding this comment

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

It was only happening during tests, didn't find you how to enable debug mode for them...

Anyway updated to isSuperTypeOf, could you review now?

@carusogabriel
Copy link
Contributor

carusogabriel commented May 4, 2018

@Majkl578 Why not refactor it to one single class and deal with each operand?

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 4, 2018

@carusogabriel PHPStan\Rules\Operators\OperandsInArithmeticDivision::getNodeType() requires to return string so it'd be unnecessary work/abstraction for type & message & specific behavior (like +) imho.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 4, 2018 via email

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 5, 2018

Btw. this doesn't support GMP, but PHPStan currently does not support resource scopes/types so it's currently impossible to implement correctly. What to do, ignore for now?;
Edit: GMP is object as of 5.5+ so it's safe to ignore GMP resource. But still can't be implemented since GMP + GMP operation produces ErrorType.

@Majkl578
Copy link
Contributor Author

Majkl578 commented May 6, 2018

Rebased.

Anything needed here? I'd like to continue with other operators (comparison, bitwise).

@ondrejmirtes ondrejmirtes merged commit b8d2ddc into phpstan:master May 17, 2018
@ondrejmirtes
Copy link
Member

Thanks! Looking forward to the next set of rules! 👍 (I also suggest checking for strings on both sides of concat $str1 . $str2)

@Majkl578 Majkl578 deleted the operator-arithmetic branch June 18, 2018 14:33
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.

None yet

4 participants