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 missing test case for native call #4311

Conversation

SelrahcD
Copy link
Contributor

Hi !

I spotted a bug today. Here is a failling test :)

When using a native call and returning first, here array_map, the ReturnTypeFromStrictTypedCallRector uses it directly and doesn't check that later return might return another type.

I suspect this is because if the if. With a debugger we can see that findCurrentScopeReturns only sees the return in the if scope.

My contribution stops here, I don't know how what would be a good fix for this :)

Have a good day.

When using a native call and returning first, here `array_map`, the `ReturnTypeFromStrictTypedCallRector` uses it directly and doesn't check that later `return` might return another type.
@samsonasik
Copy link
Member

@SelrahcD I cherry-picked your commit at PR #4312

@SelrahcD
Copy link
Contributor Author

Nice ! Thank you. Your going so fast !

I had a look at another rule and I was about to propose using $this->betterNodeFinder->findInstancesOfInFunctionLikeScoped($node, Return_::class);, as in BoolReturnTypeFromStrictScalarReturnsRector.

I checked your PR and see that you are using something that seems to do almost the same thing (didn't check too much). As a learning opportunity for me: why do you choose that solution over reusing findInstancesOfInFunctionLikeScoped?

@samsonasik
Copy link
Member

findInstancesOfInFunctionLikeScoped() doesn't do complex filtering, so require filter after filter, eg: find Return_ with the expr is Expr.

@SelrahcD
Copy link
Contributor Author

Ok, I'm not there yet and I have to compare the two rules a bit more :)
Thank you for your answer.

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.

2 participants