Fix NumericReturnTypeFromStrictScalarReturnsRector for non-natively-typed parameters

[staabm]

No description provided.

[staabm]

how should this rector behave according to phpdoc typed params?

does the "strict" in the rector name mean it should only trust native types?

[staabm]

I am thinking about a test like

<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\NumericReturnTypeFromStrictScalarReturnsRector\Fixture;

final class SkipPhpDoc
{
    /**
     * @param 0|1 $first
     */
    public function resolve($first, int $second)
    {
        return $first + $second;
    }
}

[TomasVotruba]

Docblocks are skipped as non-strict types.

[TomasVotruba]

Could you add test fixture with this code sample to make sure its still skipped?

[samsonasik]

There is ExprAnalyzer for detect variable from non typed param for that

rector-src/src/NodeAnalyzer/ExprAnalyzer.php

Line 36 in 93a279d

public function isNonTypedFromParam(Expr $expr): bool

[samsonasik]

@staabm could you verify latest change of ExprAnalyzer->isNonTypedFromParam() works on this? Thank you.

[staabm]

it seems it works - thank you

[TomasVotruba]

How can we verify this takes into account only direct integers? I'd prefer to be explicit and keep/extend the original code, instead of hoping it avoid docblock integer types, that are interprested as strict types by PHPStan.

[staabm]

I thought I did by adding more tests. Do you have something concrete in mind which should be tested in addition?

I think we can't just add instanceof IntegerRangeType because there is also a union of integers in the game.

[TomasVotruba]

I recall doc typed properties and external method/property calls are false positives. Could you add those too?

[staabm]

I reverted my implementation changes for now, as you are right in that phpdoc typed properties are a problem.
these are a problem even before the changes I suggested before this initial PR.

I re-calibrated the goal of this PR to just add more test-cases for now, so this knowledge about edge cases of this rector is not lost.

---

I got the feeling the only real way this rector could work reliably is if it would use treatPhpDocTypesAsCertain: false. is this something we could configure on a per rector basis?

[samsonasik]

Rector can specify phpstan file to be used:

$rectorConfig->phpstanConfig(__DIR__ . '/phpstan-for-rector.neon');

[staabm]

I guess this setting would then affect all rector rules not just a one.

I wonder whether rector should have a "mode" in which it does not at all trust phpdoc, so it can be very sure about the things it is doing. just applying treatPhpDocTypesAsCertain: false would regress a lot of existing functionality I guess.

[staabm]

I have reduced the PR to the important test-cases as I think the initial goal would require some things we can't do?

[TomasVotruba]

Thanks 👍

Lets merge this as it is and iterate from there. Checking only variables or disabling doc types might be an option.

[staabm]

disabling doc types might be an option

how can this be achieved?

[samsonasik]

@staabm I think that need failing test case first, to check the rule that affected, and we can see possible solution to compare type vs native type. On this case, verify if the data is property fetch, then check property type itself via Reflection.

[TomasVotruba]

@staabm I was referring to PHPStan option: #4380 (comment)
Not sure how it's implemented in PHPStan, it's been couple of years since I looked into this parameter.