ArrayAccess::offsetGet not returning a ref

16 years ago by Etienne Kneuss — view source — reply

unread

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

Personally, I believe that a fatal on such prototypes error is not
warranted, so I'd prefer (d).

What do you think would be the best option? Can you think of another?

Best,

--
Etienne Kneuss

16 years ago by Peter Cowburn — view source — reply

unread

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC
-1,000,000

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

Of the options presented, I think d) would be the best of the bunch.
Folks seem to expect the ability to get references so any solution
that gives them that would be better IMO than keeping the interface
as-is.

Personally, I believe that a fatal on such prototypes error is not
warranted, so I'd prefer (d).

What do you think would be the best option? Can you think of another?

Best,

--
Etienne Kneuss

16 years ago by Daniel Egeberg — view source — reply

unread

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC
-1,000,000

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

Of the options presented, I think d) would be the best of the bunch.
Folks seem to expect the ability to get references so any solution
that gives them that would be better IMO than keeping the interface
as-is.

Without having the technical insight in how it works internally, I
would also say that from an end-user perspective, option (d) makes
most sense.

Personally, I believe that a fatal on such prototypes error is not
warranted, so I'd prefer (d).

What do you think would be the best option? Can you think of another?

--
Daniel Egeberg

16 years ago by johannes@schlueters.de — view source — reply

unread

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

The main use case is some nested structure like

$o = new ArrayObject();
/.../
$o[23][42] = "foobar";

right?

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

If the above case is correct and due to me not liking references I
wonder whether there is a way to for an option e) which adds support for
this in some way to the engine.

johannes

16 years ago by Ralph Schindler — view source — reply

unread

Hey all,

Was there any more discussion on this? I've been bit by this as well,
several times over the past 3 years.

I'd opt for option (d) for all prototype/signature checking. Here's why:

I don't think the return "type" should be considered part of the
signature when enforcing LSP. PHP (currently), does not care what the
type of the return value is, much less if there is even one to be
returned. Whether or not it is a reference or a value should not impact
the conditions presented to the caller, in other words "the
preconditions are not strengthened in a subtype." Since PHP does no
enforcement of return values, postconditions don't seem to apply.

Also, relaxing the checks would be backwards compatible. Since no code
should currently exist that does this (its an E_FATAL currently if the
signature originates in an interface, E_STRICT notice when the signature
originates in an abstract/base class overridden in a subclass).

Thoughts?
Ralph Schindler

Johannes Schlüter wrote:

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

The main use case is some nested structure like

$o = new ArrayObject();
/.../
$o[23][42] = "foobar";

right?

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

If the above case is correct and due to me not liking references I
wonder whether there is a way to for an option e) which adds support for
this in some way to the engine.

johannes

16 years ago by Stas Malyshev — view source — reply

unread

Hi!

I'd opt for option (d) for all prototype/signature checking. Here's why:

I think relaxing the check may make sense. Do you have some code example
that doesn't work and you want it to work?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

16 years ago by Ralph Schindler — view source — reply

unread

I can give 2 examples, one that triggers the problem, the other that is
a real world issue:

Simple:

<?php

interface I {
public function foo($name);
}

class C implements I {
public function & foo($name) {}
}

$c = new Bar();

running this produces:

$ php -d error_reporting=32767 test-reference-in-signature.php
PHP Fatal error: Declaration of C::foo() must be compatible with that
of I::foo() in path/to/test-reference-in-signature.php on line 8

Real world issue with ArrayAccess:

<?php

class SomeContainer implements ArrayAccess {
protected $_data = array('foo' => array(1,2,3));
public function & offsetGet($name) {
$r = & $this->_data['foo'];
return $r;
}
public function offsetSet($name, $value) {}
public function offsetExists($name) {}
public function offsetUnset($name) {}
}

$b = new Bar();
$b['foo'][3] = 4; // implies Bar::offsetGet() happens before assign

running this produces:

$ php -d error_reporting=32767 test-reference-in-arrayaccess.php
PHP Fatal error: Declaration of SomeContainer::offsetGet() must be
compatible with that of ArrayAccess::offsetGet() in
path/to/test-reference-in-arrayaccess.php on line 3

-ralph

Stas Malyshev wrote:

Hi!

I'd opt for option (d) for all prototype/signature checking. Here's why:

I think relaxing the check may make sense. Do you have some code example
that doesn't work and you want it to work?

16 years ago by Ralph Schindler — view source — reply

unread

The attached patch is the suggested fix. I made this against master on
github.

-ralph

Ralph Schindler wrote:

I can give 2 examples, one that triggers the problem, the other that is
a real world issue:

Simple:

<?php

interface I {
public function foo($name);
}

class C implements I {
public function & foo($name) {}
}

$c = new Bar();

running this produces:

$ php -d error_reporting=32767 test-reference-in-signature.php
PHP Fatal error: Declaration of C::foo() must be compatible with that
of I::foo() in path/to/test-reference-in-signature.php on line 8

Real world issue with ArrayAccess:

<?php

class SomeContainer implements ArrayAccess {
protected $_data = array('foo' => array(1,2,3));
public function & offsetGet($name) {
$r = & $this->_data['foo'];
return $r;
}
public function offsetSet($name, $value) {}
public function offsetExists($name) {}
public function offsetUnset($name) {}
}

$b = new Bar();
$b['foo'][3] = 4; // implies Bar::offsetGet() happens before assign

running this produces:

$ php -d error_reporting=32767 test-reference-in-arrayaccess.php
PHP Fatal error: Declaration of SomeContainer::offsetGet() must be
compatible with that of ArrayAccess::offsetGet() in
path/to/test-reference-in-arrayaccess.php on line 3

-ralph

Stas Malyshev wrote:

Hi!

I'd opt for option (d) for all prototype/signature checking. Here's
why:

I think relaxing the check may make sense. Do you have some code
example that doesn't work and you want it to work?

16 years ago by Gustavo Lopes — view source — reply

unread

On Fri, 06 Aug 2010 15:50:33 +0100, Ralph Schindler ralph@smashlabs.com
wrote:

The attached patch is the suggested fix. I made this against master on
github.

In my opinion, it would make more sense, as was already suggested before,
to require the return to be passed by reference only if the prototype
specifies it should be passed by reference. This could be argued to be a
form of return contravariance.

As a bonus, it would be consistent with the "pass_rest_by_reference" flag
check:

if (proto->common.pass_rest_by_reference
&& !fe->common.pass_rest_by_reference) {
return 0;
}

//if (fe->common.return_reference != proto->common.return_reference) {
if (proto->common.return_reference && !fe->common.return_reference) {
return 0;
}

--
Gustavo Lopes

16 years ago by Stas Malyshev — view source — reply

unread

Hi!

In my opinion, it would make more sense, as was already suggested before,
to require the return to be passed by reference only if the prototype
specifies it should be passed by reference. This could be argued to be a
form of return contravariance.

Yes, this makes sense.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

16 years ago by Derick Rethans — view source — reply

unread

Hi,

it looks like the fact that ArrayAccess::offsetGet is not returning a
reference is a recurrent problem, I see basically 4 options:

a) Ignore the issue, change nothing

b) Rewrite offsetGet to return a ref, breaking BC

c) Create a new ArrayAccess interface where it does return a ref

d) Relax prototype checks so that both are allowed.

Personally, I believe that a fatal on such prototypes error is not
warranted, so I'd prefer (d).

Can you give an example of this one?

Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug

16 years ago by Gustavo Lopes — view source — reply

unread

On Sat, 07 Aug 2010 03:55:25 +0100, Etienne Kneuss etienne@immomigsa.ch
wrote:

Returning a ref is an additional constraint, meaning that it would
actually be covariance, and not contravariance. But yes, it makes
sense.

Yes, you are right. To put it another way: a subclass should be able to

Relax the preconditions
Strengthen the post-conditions

I actually noticed this shortly after sending the e-mail, but hoped no one
else would :p

Actually, after rereading what you said and the proposed diff, I'm not
sure it makes sense. It should not be consistent with arguments.

A method subtype may relax the parent prototype by not requiring a
ref for an argument. This is fine and it's indeed contra-variant.

Again, my mistake. The if before was indeed about arguments...

However, the return value of a method subtype should be able to return a
ref even if the parent prototype doesnt. However, if the parent requires
a ref, the child should too. Which is covariant.

I'm confused, this was what exactly what I suggested.

The condition I suggested is this (proto is the parent, fe the child):

if (proto->common.return_reference && !fe->common.return_reference) {
return 0;
}

So it returns an error condition if the parent returns a reference and the
child doesn't.

It's the check on the arguments that's wrong in this sense. However, I
think invariance should be required here. The thing is, passing an
argument by reference is actually also a post-condition -- if we pass an
argument by reference, we're going to do something with it.

In fact, invariance is required for the non-"rest" part of the arguments:

if (fe->common.arg_info[i].pass_by_reference !=
proto->common.arg_info[i].pass_by_reference) {

Again, sorry for the confusion.

--
Gustavo Lopes

ArrayAccess::offsetGet not returning a ref View as Markdown

I think relaxing the check may make sense. Do you have some code example that doesn't work and you want it to work?

I think relaxing the check may make sense. Do you have some code example
that doesn't work and you want it to work?