Skip to content
/ rector-src Public

ReturnTypeFromStrictTernaryRector: Support complex ternaries#4515

Merged
TomasVotruba merged 14 commits intorectorphp:mainfrom
staabm:ternary
Jul 18, 2023
Merged

ReturnTypeFromStrictTernaryRector: Support complex ternaries#4515
TomasVotruba merged 14 commits intorectorphp:mainfrom
staabm:ternary

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Jul 14, 2023

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

staabm
staabm commented Jul 14, 2023
View reviewed changes
...TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/Fixture/skip_phpdocs.php.inc

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

final class SkipPhpdocs
Copy link
Copy Markdown
Contributor Author

@staabm staabm Jul 14, 2023

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
Copy Markdown
Member

@samsonasik samsonasik Jul 14, 2023

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:

rector-src/src/NodeAnalyzer/ExprAnalyzer.php

Lines 42 to 52 in f056974

$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
Copy Markdown
Contributor Author

@staabm staabm Jul 16, 2023

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
Copy Markdown
Contributor Author

@staabm staabm Jul 17, 2023

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

staabm
staabm commented Jul 14, 2023
View reviewed changes
rules/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector.php
continue;
}

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

@staabm staabm Jul 14, 2023

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
Copy Markdown
Contributor Author

staabm commented Jul 18, 2023 •
edited
Loading

should be ready for review/merge @samsonasik

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Jul 18, 2023

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

Reviewers

@samsonasik samsonasik samsonasik approved these changes

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@staabm @TomasVotruba @samsonasik