Unexpected changes by ReturnTypeFromStrictScalarReturnExprRector

[RobertMe]

Hi,

Quite new using Rector and still finding my feet. In general it does seem awesome. But running it on a code base of ~10 years old is, well, overwhelming. So, as seems to be the general advice, I'm applying rule by rule and checking / validating the results (does it do anything unexpected, possibly introduce bugs, etc).

At the moment I'm checking the results of adding ReturnTypeFromStrictScalarReturnExprRector, but there are more changes then I would have assumed / expected.
See for example: https://getrector.com/demo/24d86086-9520-4c46-9569-3f25b7b5083f
With only ReturnTypeFromStrictScalarReturnExprRector enabled I wouldn't expect that this code would add a return type to bar()

<?php

function foo(): string
{
    return 'test';
}

function bar(string $bar, int $baz)
{
    return $baz < 5 ? foo() : $bar;
}

Personally I would expect that with ReturnTypeFromStrictScalarReturnExprRector enabled it would only add return types when the return is a literal value. The result I'm currently getting would, from my expectations, only be possible when also having rules named like ReturnTypeFromStrictParamRector, ReturnTypeFromStrictTypedCallRector and ReturnTypeFromStrictTernaryRector enabled. As this code returns a value based on a ternary which then involves both a typed method call and a typed parameter, and not (/never) a literal value.

Do mind that I find it awesome that Rector is able to do this. But it's not what I expected to happen when enabling just this rule. And it makes it harder to review the changes made.
Furthermore it makes me wonder what the other mentioned rules like ReturnTypeFromStrictParamRector, ReturnTypeFromStrictTypedCallRector and ReturnTypeFromStrictTernaryRector will do, as the things I would expect them to do based on the name, are already involved by the ReturnTypeFromStrictScalarReturnExprRector rule. This is also confirmed by for example using the example for ReturnTypeFromStrictTernaryRector in the demo with ReturnTypeFromStrictScalarReturnExprRector, the result is identical 😄

So I'm not sure whether this would be a lack of understanding on my end (/having different expectations based on the name), possibly oversimplified documentation (which then creates the misunderstanding) or maybe even a "bug" in that it does (way) more then it was intended to.

[TomasVotruba]

Hi,
thanks for such a detailed investigation. I think your assumptions are correct.

As the complexity of rules grows, some of the rules might overlap. Yet, this doesn't seem like a bug, but rather 2 rules doing some work together.

If you figure out a fix to make them unique and separate, please send PR 👍

[TomasVotruba]

Sounds good! Any new rule that completes Rector features is welcomed 👍

[samsonasik]

For return $this, there is already ReturnTypeFromStrictFluentReturnRector https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#returntypefromstrictfluentreturnrector for that

[RobertMe]

So going over the code once more (when copy/pasting into a ReturnTypeFromLiteralScalarReturnRector 😃) I now understand what the "Scalar" stands for in ReturnTypeFromStrictScalarReturnExprRector: it must be a scalar return type. But the expression can be anything, as long as it thus (only) results in a scalar type.

So in my copy/pasting I reused the StrictScalarReturnTypeAnalyzer and just changed the condition from instanceof Expr to instanceof Scalar (which is inherited by String_, LNumber, DNumber, the magic constants etc).

So I don't think there is much need for such a rule in SetList::TYPE_DECLARATION as it will 100% overlap with the existing ReturnTypeFromStrictScalarReturnExprRector (as Scalar extends Expr). But if desired I will go ahead and create a PR for it, otherwise I'll just use the current implementation (temporarily) in my project and "drop" it again when it is feasible to "upgrade" to ReturnTypeFromStrictScalarReturnExprRector (i.e.: do a pass on the whole project just adding typehints to functions which only return a hardcoded value, which shouldn't need a review, and do a phased run with ReturnTypeFromStrictScalarReturnExprRector. This as I've already seen some "unwanted" behavior with the latter, like a return ceil(...) which gets typehinted as a float (which is correct) while an int would be fine (so manually adding a cast)).

[RobertMe]

For return $this, there is already ReturnTypeFromStrictFluentReturnRector https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#returntypefromstrictfluentreturnrector for that

Thanks, that indeed seems to do the same, but has a few more tricks up its sleeve. So I might as well replace my own rule with this. But it also is/was nice to just execute a single rule which does a very predictable and consistent change. It seems like the rule you mention also sets the return type to self / static when new self/static() is returned (and possibly some other things). So obviously nice to have, but I prefer a quick glance over those. So now that 99% of the return $this; cases have been changed (which were hundreds) executing this rule should only result in a couple of changes which are easily checked by hand.

[RobertMe]

For reference, PR created for a rule which only adds return type when only literal values (of single type) are returned: rectorphp/rector-src#5361

As written earlier. If not accepted it's fine for me, because it is a 100% subset of ReturnTypeFromStrictScalarReturnExprRector. Needed the rule for myself / work anyway so might as well try to upstream it 😄.