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

ReturnTypeFromStrictTernaryRector: Support complex ternaries #4515

Merged
merged 14 commits into from
Jul 18, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 14, 2023

idea is to allow ReturnTypeFromStrictTernaryRector to infer the native return type on complex typed expressions


namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;

final class SkipPhpdocs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test fails to skip right now. If I understood the "strict" rules correctly, they should not rely on phpdoc, right?

if so, any idea how to achieve that?

Copy link
Member

Choose a reason for hiding this comment

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

You can compare type vs native type, like in ExprAnalyzer:

$nativeType = $scope->getNativeType($expr);
if ($nativeType instanceof MixedType && ! $nativeType->isExplicitMixed()) {
return true;
}
$type = $scope->getType($expr);
if ($nativeType instanceof UnionType) {
return ! $nativeType->equals($type);
}
return ! $nativeType->isSuperTypeOf($type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. I added a native type check and now I am wondering why ternary_var_call.php.inc is failling.
rector type resolving tells me the native type of the ternary is MixedType but running the same code directly in PHPStan returns a correct native type..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the failling test for now, as I don't see it important enough to block the actual useful change

continue;
}

$onlyStmt = $classMethod->stmts[0] ?? null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

before this PR this rector only supported call-methods with a single line.
I wided the scope to call-likes with a single return

@staabm staabm marked this pull request as ready for review July 17, 2023 09:47
@staabm staabm requested a review from TomasVotruba as a code owner July 17, 2023 09:47
@staabm
Copy link
Contributor Author

staabm commented Jul 18, 2023

should be ready for review/merge @samsonasik

@TomasVotruba
Copy link
Member

Looks good, thank you 👍

@TomasVotruba TomasVotruba merged commit b75b5d3 into rectorphp:main Jul 18, 2023
@staabm staabm deleted the ternary branch July 18, 2023 09:20
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

3 participants