DEVSENSE / Phalanger

PHP 5.4 compiler for .NET/Mono frameworks. Predecessor to the opensource PeachPie project (www.peachpie.io).
http://v4.php-compiler.net/
Apache License 2.0
381 stars 92 forks source link

Bug in clone() #28

Closed kripper closed 6 years ago

kripper commented 10 years ago

This test-case shows 'A is wrong' instead of 'A is ok':

$a = new stdClass();
$a->test = 'ok';
$ptr = & $a->test; // <--- This reference corrupts $a's clonation
$b = clone($a);
$b->test = "wrong";
die('A is ' . $a->test);

Note that even doing unset($ptr) doesn't workarround the bug:

$ptr = & $a->test; // This reference corrupts $a's clonation
unset($ptr);
robertleeplummerjr commented 10 years ago

I've reproduced this bug on https://github.com/wikiLingo/wikiLingo/blob/master/WYSIWYGWikiLingo/Parser.php parsing some very simple html. It stores the html tag's in an array, and then later pops them. When I go to a debugger, I can see pushing to the array, and then when it goes to pop, the object has been clearly mutilated. Nothing is being passed by ref, and this works fine in php.

Thanks again for all your hard work guys!

kripper commented 10 years ago

Hi Rob:

Is it related with issue #28 or another issue?

robertleeplummerjr commented 10 years ago

I believe it is directly related.

robertleeplummerjr commented 10 years ago

It is like everything is being passed by ref rather than as it should be.

robertleeplummerjr commented 10 years ago

I should explain that wikiLingo.net uses Phalanger heavily, and we are working on getting all the unit tests working with it through Phalanger. Thus far, it has been fantastic, but here we are at a bottleneck.

kripper commented 10 years ago

Please consider providing an isolated minimal unit test (not relaying on other sources), as the one I pasted above to, to make it easier to test, debug and fix.

robertleeplummerjr commented 10 years ago

You are right. I have actually been working close with Jakub to resolve a few issues to get wikiLingo up, and thought it would be helpful as it pushes Phalanger to the very edge of what it is capable of doing. I will try and keep the complexity to a minimum. Thanks for your suggestion.

On Wed, Jun 4, 2014 at 1:35 PM, J. Christopher Pereira < notifications@github.com> wrote:

Please consider providing an isolated minimal unit test (not relaying on other sources), as the one I pasted above to, to make it easier to test, debug and fix.

— Reply to this email directly or view it on GitHub https://github.com/DEVSENSE/Phalanger/issues/28#issuecomment-45123821.

Robert Plummer

robertleeplummerjr commented 10 years ago

I noticed after looking into the object we use ICloneable. Wouldn't it be easier to use to do:

var copy = new DObject(/*of type DObject*/ RealObject);

And then implement a new type based off the direct attributes that RealObject has for shallow copying? Of course, I could be wrong. Some reasons to not use ICloneable - http://blogs.msdn.com/b/brada/archive/2003/04/09/49935.aspx

kripper commented 7 years ago

Has this been fixed in PeachPie?

jakubmisek commented 7 years ago

@kripper running the sample on PHP 7.1 gives me: A is wrong

kripper commented 7 years ago

Hi @jakubmisek, you are right. PHP 7.1's has the same bug:

$orig = new stdClass();
$orig->test = 'original';

$ptr = & $orig->test; // <--- Setting a ptr changes the behaviour 

$copy = clone $orig;
$copy->test = "modified";

die("Orig is: {$orig->test}"); // Gives 'modified'

But, if you unset $ptr, Phalanger's behaviour is different than PHP 7.1's:

$orig = new stdClass();
$orig->test = 'original';

$ptr = & $orig->test;
unset($ptr); // <--- This fixes the problem

$copy = clone $orig;
$copy->test = "modified";

die("Orig is: {$orig->test}"); // PHP 7.1 gives 'original', but Phalanger gives 'modified'
jakubmisek commented 7 years ago

@kripper I wouldn't call it bug - clone to a reference behaves correctly - the test is wrong

edit: until it behaves like in PHP, it is correct .. since it is not defined well in PHP, we can't specify the correct behavior either

jakubmisek commented 7 years ago

@kripper unset clears the reference (decreases refs count) which is not possible with .NET GC.

kripper commented 7 years ago

I consider it a bug because setting an unused reference shouldn't change the behavior of "clone". Do you know any workaround for using references to object properties and still be able to clone objects?

jakubmisek commented 7 years ago

by contraries; making a variable reference changes behavior. That's the purpose of refs. So it seams in PHP 7 the behavior was fixed: clone ref => ref clone val => copy of val

kripper commented 7 years ago

Sorry, I don't understand your comment. In the example, $ptr is a reference to $orig->test.

$orig->test is not a reference to $ptr and we are not cloning a reference, but a object that is being referenced by $ptr.

Anyway, after doing $ptr = & $orig->test, PHP seems to treat $orig->test as a reference, and like you said (clone ref => ref), the result is that $ptr, $orig->test and $cloned->test are all references to the same value.

If it's not a bug, It's a pity that PHP changed how this works since v5.6, because now I see no way how to use cloned objects together with references, and it's impossible to migrate working code.

I believe this is a design problem that has and will be discussed until fixed, probably related to the fact that PHP probably only manages variable aliases tables instead of real references in only one direction (where $a references $b and $b does not reference $a).

kripper commented 6 years ago

Closing. This is a language design problem in PHP, not a problem with Phalanger. See more info here: https://bugs.php.net/bug.php?id=75632