Skip to content

Add missing test case for native call#4311

Closed
SelrahcD wants to merge 1 commit intorectorphp:mainfrom
SelrahcD:failing-test-ReturnTypeFromStrictTypedCallRector-with-mixed-type
Closed

Add missing test case for native call#4311
SelrahcD wants to merge 1 commit intorectorphp:mainfrom
SelrahcD:failing-test-ReturnTypeFromStrictTypedCallRector-with-mixed-type

Conversation

@SelrahcD
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@SelrahcD
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@SelrahcD
Copy link
Copy Markdown
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