`NewInInitializerRector` gives non-equivalent code

[ChrisDBrown]

Bug Report

Subject	Details
Rector version	v0.13.8

Changes made by this rule are changing constructor signatures in some circumstances. This might be intentional based on the RFC but I'm unsure.

Take this code before transformation:

class ArrayCollection {}

class DemoFile
{
    private ArrayCollection $foo;
    
    public function __construct(?ArrayCollection $foo) {
        $this->foo = $foo ?? new ArrayCollection();
    }
}

and after transformation:

class ArrayCollection {}

class DemoFile
{
    private ArrayCollection $foo;
    
    public function __construct(private ArrayCollection $foo = new ArrayCollection())
	{
	}
}

There's two valid call signatures before transformation:

new DemoFile(null);
new DemoFile($iAmAnArrayCollection);

After the transformation the first of these is no longer valid. To get the same behaviour we'd call it like this:

new DemoFile();

Meaning we need to change everywhere we've used this from explicit to implicit defaults.

Minimal PHP Code Causing Issue

Demo: https://getrector.org/demo/e9b02db5-c274-489b-abb3-3274aed46eba

Expected Behaviour

Honestly, I'm kind of unsure. The most obvious would be not to transform unless the null default is already in the constructor, ie do transform this:

public function __construct(?ArrayCollection $foo = null) {
    $this->foo = $foo ?? new ArrayCollection();
}

This would introduce a similar but opposite issue though. We'd go from 3 valid call signatures:

new DemoFile();
new DemoFile(null);
new DemoFile($iAmAnArrayCollection);

to two:

new DemoFile();
new DemoFile($iAmAnArrayCollection);

Perhaps this rule should be configurable to the users preference of the two? Or just be skipped entirely?

[samsonasik]

I think we need separate rule to remove null arg on new DemoFile(null) when the __construct() passed new in initializer feature, eg: RemoveNullArgOnNewClassInitializerRector

[ChrisDBrown]

@samsonasik this won't solve the problem that this rule transforms valid code into invalid code. We'll also have cases where the argument being passed is of type ArrayCollection|null rather than just null, requiring more than just removing the null arg to fix.

Overall I can't think of a safe way to do the transformation that this rule is applying - the type signatures before and after are just not compatible. Anyone correctly passing null will need to either remove this rule or modify their code to suit it. Is this acceptable with it being part of the 8.1 ruleset?

[samsonasik]

Could you provide sample code/repository/failing test case for it? There are some use case I can think of:

-new DemoFile(null); 
+new DemoFile();

$iAmAnArrayCollection = rand(0,1)
    ? null
    : new ArrayCollection();
-new DemoFile($iAmAnArrayCollection);
+new DemoFile($iAmAnArrayCollection ?? new ArrayCollection()); 

[TomasVotruba]

@ChrisDBrown I wonder, where did this happened to you? Do you use null for defaults in your projects? Could you share 3 most common snippets of that?

[TomasVotruba]

Closing as accepted to avoid keep issue tracker focus on unclear issues that needs our attention.

Feel free to send PR with failing test fixture to cover your case in NewInInitializerRectorTest :+1: It would be easier to solve then :slightly_smiling_face: