phpinspectionsea
phpinspectionsea copied to clipboard
False positive "Objects are always passed by reference" when reference is actually needed
| Subject | Details |
|---|---|
| Plugin | Php Inspections (EA Extended), version 4.0.5 |
| Language level | PHP 7.4 |
Bug description
The following minimal example gets B::__construct marked with "Objects are always passed by reference" and suggests replacing A &$a_ref with A $a_ref.
The actual intention is to give B ownership of the reference itself, so that it can reassign it. Applying the inspection removes that reference, causing $a to never get assigned so the code throws an error.
I'm not sure how common this pattern is in PHP code, but it should be enough to detect when a method parameter such as &$a_ref is used inside that method again as &$a_ref rather than $a_ref, and not show the intention in such case.
class A{
public function test(): void{
echo 'hi';
}
}
class B{
private ?A $a_ref;
public function __construct(?A &$a_ref){
$this->a_ref = &$a_ref;
}
public function assign(): void{
$this->a_ref = new A();
}
}
$a = null;
$b = new B($a);
$b->assign();
$a->test();
I am experiencing quite some issues with this inspection as well.
There is a misunderstanding over what a "passing by reference" is
It is about the variable - memory location, yes, having certain content.
But it goes beyond this content, the value inside the variable.
Passing by reference
You can change the content in the context of caller - are writing to the same memory location.
When I "pass object by reference" to a callee, the callee can assign into that variable a string and I as the caller, have suddenly also a string there, instead of that object.
Passing by value
New memory is allocated for the argument. The content of that memory location depends on the type:
- In case of primitives, the content is copied.
- In case of arrays (and possibly strings), afaiui, the new memory location points to a lazy cloner which clones the value upon mutation operation.
- In case of objects (also resources), the new memory location points to the same object.
The classification of variable passing is more broad than by reference/by value
There are many modalities, but the simplest illustrating the point is:
- a passing-type which exposes the memory location (in PHP: passing by reference)
- a passing-type which allows mutation of the value (in PHP: passing of objects)
- one where the value is (lazy-)detached. (in PHP: passing by value of primitives and arrays)
A couple of interesting points:
- You cannot pass an array to have it changed by callee, without giving away the control of getting it overwritten altogether by anything. You jump from 3 directly to 1 when switching from "by value" to "by reference".
- In the development of PHP, people did not attempt to plug in cloning of objects and resources into this passing-type split, which would make things more interesting.
- I am not sure how arrays with references inside them behave, I remember the references being actually kept, because the by value only applies to the carrier array.
i'm seeing a similar problem with reference parameters, but not exactly as described...
(i scrubbed my original example code - realized a problem so it's no longer valid here. However, here's a simpler example):
// PHP 8.1
function update(string $key, $value, Settings|array &$settings) {
if ($settings instanceof Settings) {
$settings->set($key, $value);
} else {
$settings[$key] = $value;
}
}
$foo = [ /* some array data */ ];
update('bar', 'new foo', $foo);
i'm not suggesting this function is good design, but it's valid.
The $settings parameter must be a reference when the argument is an array, but the inspection is ignoring the primitive type (which it normally wouldn't complain about as long as it's written to) because it's using a union type hint with a class.
Maybe this is the same issue, or should i file a new one?