phpinspectionsea icon indicating copy to clipboard operation
phpinspectionsea copied to clipboard

False positive "Objects are always passed by reference" when reference is actually needed

Open chylex opened this issue 5 years ago • 3 comments

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();

chylex avatar Aug 27 '20 21:08 chylex

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:

  1. a passing-type which exposes the memory location (in PHP: passing by reference)
  2. a passing-type which allows mutation of the value (in PHP: passing of objects)
  3. 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.

informatik-handwerk avatar Feb 16 '22 17:02 informatik-handwerk

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?

neuberkauf avatar Feb 27 '23 15:02 neuberkauf