[RFC][VOTE] E_WARNING on invalid container read-adccess View as Markdown

I'd like to go ahead and open up the RFC to voting to the scope
that it is written.

Hi David,

Although I will almost certainly be voting yes, the vote does need 2/3's to pass.

All language changes require that, even if no syntax is changed.

cheers
Dan
Ack

Hi David,

I raised this concept a couple weeks ago to a couple +1's. Discussion was
held mostly upon the PR [1], and I went through and documented within the
RFC [2]. I'd like to go ahead and open up the RFC to voting to the scope
that it is written.

--
Dave

[1] https://github.com/php/php-src/pull/2031
[2] https://wiki.php.net/rfc/notice-for-non-valid-array-container

Could you add performance analysis section?

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Le 16/08/2016 à 17:55, David Walker a écrit :

I'd like to go ahead and open up the RFC to voting to the scope
that it is written.

Hi,

At AFUP, we would be +1 on this RFC.

Basically: it could/will help detect problems, and the behavior change
should not cause too many bc-breaks (actually, when it does cause some,
it'll - probably - mostly be by detecting problems/bugs).

Thanks for your work on this!

--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/

I raised this concept a couple weeks ago to a couple +1's. Discussion was
held mostly upon the PR [1], and I went through and documented within the
RFC [2]. I'd like to go ahead and open up the RFC to voting to the scope
that it is written.

[1] https://github.com/php/php-src/pull/2031
[2] https://wiki.php.net/rfc/notice-for-non-valid-array-container

For the record, the RFC rightly claims:

| The default behavior, of a silent NULL return, is not correctly
| defined in the Array Documentation however is noted in a comment

I have documented this behavior now[3], and removed the note, which was
not quite correct, because it claimed that TRUE would be returned when
array dereferencing NULL, bool or int.

[3] http://svn.php.net/viewvc?view=revision&revision=339937

--
Christoph M. Becker

Hi Dmitry,

I vote NO, because the implementation introduces more problems than
intents to fix.

For example the following code starts to throw exception:

<?php $a = [null]; $c = null; var_dump($a[0][0] + $c[0]); ?>

Isn't that the point of this RFC?
I would expect this code to only ever work by accident.

Dmitry Stogov dmitry@zend.com schrieb am Mi., 31. Aug. 2016, 08:37:

I vote NO, because the implementation introduces more problems than
intents to fix.

For example the following code starts to throw exception:

<?php $a = [null]; $c = null; var_dump($a[0][0] + $c[0]); ?>

Array access on null should always throw IMO.

BTW, I'm not against the feature, I'm against the implementation.

It's just can't be committed in current state.

I think we vote on features, not implementations. Therefore you should
probably have voted yes.

Thanks. Dmitry.

---

From: Christoph M. Becker cmbecker69@gmx.de
Sent: Thursday, August 25, 2016 2:49:51 PM
To: David Walker; PHP internals
Subject: [PHP-DEV] Re: [RFC][VOTE] E_WARNING on invalid container
read-adccess

I raised this concept a couple weeks ago to a couple +1's. Discussion
was
held mostly upon the PR [1], and I went through and documented within the
RFC [2]. I'd like to go ahead and open up the RFC to voting to the scope
that it is written.

[1] https://github.com/php/php-src/pull/2031
[2] https://wiki.php.net/rfc/notice-for-non-valid-array-container

For the record, the RFC rightly claims:

| The default behavior, of a silent NULL return, is not correctly
| defined in the Array Documentation however is noted in a comment

I have documented this behavior now[3], and removed the note, which was
not quite correct, because it claimed that TRUE would be returned when
array dereferencing NULL, bool or int.

[3] http://svn.php.net/viewvc?view=revision&revision=339937

--
Christoph M. Becker

Ah, thanks, I thought it was silently passing :-)

no. if you would try the proposed PR with this code, you would see:

Fatal error: Uncaught Error: Unsupported operand types

---

From: Marco Pivetta ocramius@gmail.com
Sent: Wednesday, August 31, 2016 10:10:23 AM
To: Dmitry Stogov
Cc: Christoph M. Becker; PHP Internals List; Nikita Popov; David Walker
Subject: Re: [PHP-DEV] Re: [RFC][VOTE] E_WARNING on invalid container
read-adccess

Hi Dmitry,

I vote NO, because the implementation introduces more problems than
intents to fix.

For example the following code starts to throw exception:

<?php $a = [null]; $c = null; var_dump($a[0][0] + $c[0]); ?>

Isn't that the point of this RFC?
I would expect this code to only ever work by accident.

no. if you would try the proposed PR with this code, you would see:

Fatal error: Uncaught Error: Unsupported operand types

I get the following behavior against PR #1269:

$ sapi/cli/php -n -d error_reporting=-1 -r "$a = [null]; $c = null;
var_dump($a[0][0] + $c[0]);"

Notice: Trying to get index of a non-array in Command line code on line 1

Notice: Trying to get index of a non-array in Command line code on line 1
int(0)

Cheers!

---

From: Marco Pivetta ocramius@gmail.com
Sent: Wednesday, August 31, 2016 10:10:23 AM
To: Dmitry Stogov
Cc: Christoph M. Becker; PHP Internals List; Nikita Popov; David Walker
Subject: Re: [PHP-DEV] Re: [RFC][VOTE] E_WARNING on invalid container read-adccess

Hi Dmitry,

I vote NO, because the implementation introduces more problems than intents to fix.

For example the following code starts to throw exception:

<?php $a = [null]; $c = null; var_dump($a[0][0] + $c[0]); ?>

Isn't that the point of this RFC?
I would expect this code to only ever work by accident.

Hi David,

I would propose to cancel voting and restart it when the good
implementation is found.

Otherwise, people may rise their hands for something that can't be
implemented in good enough way.

There is no need to cancel the vote. However it is the role of the RMs to
veto the patch of its quality is not at the level we want.

If an acceptable patch can be done before RC then it can be applied.

Thoughts?

Cheers
Pierre

Hi David,

I would propose to cancel voting and restart it when the good
implementation is found.

Otherwise, people may rise their hands for something that can't be
implemented in good enough way.

Thanks. Dmitry.

Hi Dmitry (et.al),

I ended up addressing the problem you raised where simply looking at the
next opcode type wasn't verbose enough, and had the thrown exception.

So I'm wondering the reasons why looking ahead wouldn't be considered a
good implementation? The attempt to limit the quantity of raised warnings,
and raising once for a null, does need to be aware if the return of our op,
is being used again in the next op. I do notice that in zend_execute
there's no other examples of looking forward an opline, but that it does
exist within zend_vm. I'm wondering if a more acceptable implementation
would be to move the logic of looking-forward and checking if the next
fetch-dim-r to the opcode-handler.

Or, if your concern is more with the looking-forward in general. It was
brought up a while back (and on the PR) of the problems associated with
being able to handle a null-return from error, as compared to a null that
is honestly set. The idea that array-access on null just be ignored. I'm
not sure if this would be seen as the more acceptable implementation which
would remove the need to look-ahead in oplines, as a NULL (error) result
would safely be ignored.

Thanks

Dave

Dmitry,

Poking around, I'm must not be understanding how TICK opcodes would
negatively impact the lookahead. It may be my newness, but the way I'm
reading the compilation of ZEND_AST_DIM, would not be suspect to random
TICK opcodes from impacting the lookahead. I see that for a single
ZEND_AST_DIM, compilation will recuse emitting all the FETCH_DIM_R opcodes
for it in succession. Whereafter, a subsequent call to zend_compile_stmt()
will emit the TICK if it was declared. It could also be that I'm unsure
what a test example would be that could inject the TICK into the middle, of
the DIM compilation.

The same applies to the BEGIN/END silence, as they will be wrapped around
the entire compilation of the DIM opcodes. [1] has an example using both
ticks and silencing, which seems to work as intended emitting a single
warning (for the un-silenced access)

I'm not exactly sure what you mean by debugging-opcodes here though,
however, unless someone is hooking into the compilation and compiling DIM's
in a different way, I don't see the inherent problems with the lookahead.

I was thinking about the use of an exception, however, I think that'd be a
pretty significant shift. It would also be very odd that this case would
be the exception throwing one, whereas accessing via reference wouldn't, or
any of the undefined indexes wouldn't. It would also impact most user code
out there by needing to wrap variable array access in a try{}catch, which
would be imposing a pretty large change to php 8.0. That's why I didn't
really think shifting to an exception would be appropriate here.

Thanks

Dave

[1] - https://gist.github.com/bp1222/18d80e8ec962eba1ba75c99124191a0a

Looking forward won't always work out of the box.

Take into account possible TICK opcodes, BEGIN/END silence, debugging
opcodes.

It's probably easier to throw exception, instead of recovery, but this is
a BC break, and might be done only in 8.0.

Thanks. Dmitry.

From: David Walker dave@mudsite.com
Sent: Thursday, September 1, 2016 7:40:15 PM
To: Dmitry Stogov; Christoph M. Becker; Marco Pivetta

Cc: PHP Internals List; Nikita Popov
Subject: Re: [PHP-DEV] Re: [RFC][VOTE] E_WARNING on invalid container
read-adccess

Hi David,

I would propose to cancel voting and restart it when the good
implementation is found.

Otherwise, people may rise their hands for something that can't be
implemented in good enough way.

Thanks. Dmitry.

Hi Dmitry (et.al),

I ended up addressing the problem you raised where simply looking at the
next opcode type wasn't verbose enough, and had the thrown exception.

So I'm wondering the reasons why looking ahead wouldn't be considered a
good implementation? The attempt to limit the quantity of raised warnings,
and raising once for a null, does need to be aware if the return of our op,
is being used again in the next op. I do notice that in zend_execute
there's no other examples of looking forward an opline, but that it does
exist within zend_vm. I'm wondering if a more acceptable implementation
would be to move the logic of looking-forward and checking if the next
fetch-dim-r to the opcode-handler.

Or, if your concern is more with the looking-forward in general. It was
brought up a while back (and on the PR) of the problems associated with
being able to handle a null-return from error, as compared to a null that
is honestly set. The idea that array-access on null just be ignored. I'm
not sure if this would be seen as the more acceptable implementation which
would remove the need to look-ahead in oplines, as a NULL (error) result
would safely be ignored.

Thanks

Dave