# [RFC][VOTE] E_WARNING on invalid container read-adccess - Thread: https://externals.io/message/95236 - Original archive: https://news-web.php.net/php.internals/95236 - Started by David Walker on 2016-08-16T15:55:49+00:00 - Messages: 28 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/95236.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #95236 — David Walker — 2016-08-16T15:55:49+00:00 URL: https://externals.io/message/95236#95236 Hi all, 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 --- ## #95237 — Dan Ackroyd — 2016-08-16T17:09:12+00:00 URL: https://externals.io/message/95236#95237 · In reply to: #95236 > > > 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 --- ## #95238 — Levi Morrison — 2016-08-16T17:13:30+00:00 URL: https://externals.io/message/95236#95238 · In reply to: #95237 On Tue, Aug 16, 2016 at 11:09 AM, Dan Ackroyd wrote: > > > […] > > 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 > -- > > > While "language change" is not well-defined I agree that the vote needs to be 2/3 here. --- ## #95240 — David Walker — 2016-08-16T17:16:20+00:00 URL: https://externals.io/message/95236#95240 · In reply to: #95238 Will up the ante: I wasn't sure the RFC page on voting didn't really define this, I have no probs with 2/3. > On Tue, Aug 16, 2016 at 11:09 AM, Dan Ackroyd > wrote: > > > […] > -- > > […] > > While "language change" is not well-defined I agree that the vote needs to > be 2/3 here. --- ## #95251 — Yasuo Ohgaki — 2016-08-17T03:08:47+00:00 URL: https://externals.io/message/95236#95251 · In reply to: #95236 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 --- ## #95422 — Pascal MARTIN — 2016-08-23T18:50:04+00:00 URL: https://externals.io/message/95236#95422 · In reply to: #95236 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/ --- ## #95426 — David Walker — 2016-08-24T14:58:34+00:00 URL: https://externals.io/message/95236#95426 · In reply to: #95422 Hi Internals, My original time for vote ending was yesterday, however Yasuo requested I provide some benchmarking stats to see performance impact. I apologize I wasn't able to provide the information sooner, I was off getting married. I'd like to extend the vote through Sunday evening in the event anyone had reservations based on potential performance impact. The RFC is currently sitting 15-0 in favor. (added to RFC) Execution Time (DualCore 3ghz; 2g ram) * Current Master : ~0.09s (~489k operations) * Current Master w/RFC Displaying Warnings: ~33.25s * Current Master w/RFC Hiding Warnings: ~0.82s (~4.091m operations) We can see there is a significant increase in operations on huge loads. However, I wouldn't suspect 1m of these errors per request almost ever. So, yes there is; but I'd call it useful information overhead. -- Dave On Tue, Aug 23, 2016 at 12:50 PM Pascal MARTIN, AFUP < mailing@pascal-martin.fr> wrote: > Le 16/08/2016 à 17:55, David Walker a écrit : > > […] > > 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/ --- ## #95428 — Bishop Bettini — 2016-08-24T15:36:15+00:00 URL: https://externals.io/message/95236#95428 · In reply to: #95426 > I'd like to extend the vote through Sunday evening in the event anyone had > reservations based on potential performance impact. The RFC is currently > sitting 15-0 in favor. > I'd recommend extending the original end date by a week, since internals tends to vote in weekly intervals (an informal minimal reasonable response time, I suppose). > (added to RFC) > Execution Time (DualCore 3ghz; 2g ram) > * Current Master : ~0.09s (~489k operations) > * Current Master w/RFC Displaying Warnings: ~33.25s > * Current Master w/RFC Hiding Warnings: ~0.82s (~4.091m operations) > > We can see there is a significant increase in operations on huge loads. > However, I wouldn't suspect 1m of these errors per request almost ever. > So, yes there is; but I'd call it useful information overhead. > I've not looked at the code. How much, if at all, does the RFC change the happy path operation, on a test like this? $a = [ 'foo' ]; for ($i = 0; $i < 1000000; $i++) { $a[0]; } Notwithstanding, I feel the ops increase 10x is worth it: this is a subtle code problem. --- ## #95431 — David Walker — 2016-08-24T15:51:13+00:00 URL: https://externals.io/message/95236#95431 · In reply to: #95428 > On Wed, Aug 24, 2016 at 10:58 AM, David Walker > wrote: > > > […] > > I'd recommend extending the original end date by a week, since internals > tends to vote in weekly intervals (an informal minimal reasonable response > time, I suppose). > Good to know the general convention here. I'll extend a week until Wednesday the 31st. > > > > […] > > I've not looked at the code. How much, if at all, does the RFC change the > happy path operation, on a test like this? > > $a = [ 'foo' ]; > for ($i = 0; $i < 1000000; $i++) { $a[0]; } > > Notwithstanding, I feel the ops increase 10x is worth it: this is a subtle > code problem. > That is my thoughts too. When testing this change against my product I uncovered 10 cases of this warning being raised (which was actually helpful), but only ever 2-3 per request. So I'd imagine it's never going to come to the 1m+ happenings per request ever. And for your given test case, since $a[0] is defined (as 'foo') the change in this RFC is not triggered, so there is no difference between any of the testings. -- Dave --- ## #95444 — Christoph M. Becker — 2016-08-25T11:49:51+00:00 URL: https://externals.io/message/95236#95444 · In reply to: #95236 > 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] -- Christoph M. Becker --- ## #95511 — Dmitry Stogov — 2016-08-31T06:37:21+00:00 URL: https://externals.io/message/95236#95511 I vote NO, because the implementation introduces more problems than intents to fix. For example the following code starts to throw exception: BTW, I'm not against the feature, I'm against the implementation. It's just can't be committed in current state. Thanks. Dmitry. --- ## #95512 — Marco Pivetta — 2016-08-31T07:10:23+00:00 URL: https://externals.io/message/95236#95512 · In reply to: #95511 Hi Dmitry, > > I vote NO, because the implementation introduces more problems than intents to fix. > > For example the following code starts to throw exception: > > > Isn't that the point of this RFC? I would expect this code to only ever work by accident. --- ## #95513 — Niklas Keller — 2016-08-31T07:58:16+00:00 URL: https://externals.io/message/95236#95513 · In reply to: #95511 Dmitry Stogov 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: > > > > 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 > 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 > > > > > […] > was > > […] > > 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] > > -- > Christoph M. Becker --- ## #95514 — Dmitry Stogov — 2016-08-31T10:05:48+00:00 URL: https://externals.io/message/95236#95514 RFC proposes to emit warning, but the provided code throws unrelated "Unsupported operand types" exception. I see, that after few weeks of work, the implementation (just few lines of C code) is still hackish and incomplete. This makes me think that, the feature can't be cleanly implemented, and I vote against it. Thanks. Dmitry. --- ## #95515 — Dmitry Stogov — 2016-08-31T10:10:05+00:00 URL: https://externals.io/message/95236#95515 no. if you would try the proposed PR with this code, you would see: Fatal error: Uncaught Error: Unsupported operand types --- ## #95516 — Marco Pivetta — 2016-08-31T10:34:53+00:00 URL: https://externals.io/message/95236#95516 · In reply to: #95515 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 > *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, > > > > […] > intents to fix. > > […] > > Isn't that the point of this RFC? > I would expect this code to only ever work by accident. --- ## #95517 — Christoph M. Becker — 2016-08-31T10:36:57+00:00 URL: https://externals.io/message/95236#95517 · In reply to: #95515 > 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 > 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, > > > > […] > > Isn't that the point of this RFC? > I would expect this code to only ever work by accident. --- ## #95518 — David Walker — 2016-08-31T13:48:10+00:00 URL: https://externals.io/message/95236#95518 · In reply to: #95517 I too get Dmitry's result. As the left hand side is an error-zval, and the right hand is null, it explodes. Yes, the hackish way I try and resolve things was to check opcodes around the current one, which I should have thought in the case of addition wouldn't have been valid. The entire goal of the RFC was to prevent warning for obvious results (as it specifies on long string of accesses). One, thought about and ignored by me, aspect to the RFC was the concept of a null entity. Wherein any array access on null would result with null. I believe this could result in resolving many of the headaches I'm attempting to resolve by managing a null-return from array access as a literal null, or unknown cast to null. On Wed, Aug 31, 2016 at 4:36 AM Christoph M. Becker wrote: > > > > […] > > 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! > > > […] > read-adccess > > […] > dmitry@zend.com>> wrote: > > […] > intents to fix. > > […] --- ## #95519 — David Walker — 2016-08-31T13:49:59+00:00 URL: https://externals.io/message/95236#95519 · In reply to: #95518 > I too get Dmitry's result. As the left hand side is an error-zval, and > the right hand is null, it explodes. Yes, the hackish way I try and > resolve things was to check opcodes around the current one, which I should > have thought in the case of addition wouldn't have been valid. The entire > goal of the RFC was to prevent warning for obvious results (as it specifies > on long string of accesses). One, thought about and ignored by me, aspect > to the RFC was the concept of a null entity. Wherein any array access on > null would result with null. I believe this could result in resolving many > of the headaches I'm attempting to resolve by managing a null-return from > array access as a literal null, or unknown cast to null. > > On Wed, Aug 31, 2016 at 4:36 AM Christoph M. Becker > wrote: > > > […] > Apologies in advance for the toppost. Gmail is the bane of my existence in regards to mailing lists. --- ## #95525 — David Walker — 2016-09-01T00:05:44+00:00 URL: https://externals.io/message/95236#95525 · In reply to: #95519 > > > > […] > > > > Apologies in advance for the toppost. Gmail is the bane of my existence > in regards to mailing lists. > Hi all, As of now, the scheduled time the vote is to close, the RFC stands 19-4 (accepted). Given this is my first solo RFC, I'm not really sure protocol to go from here. Dmitry does raise that the implementation I put forward maybe a bit hackish compared to what someone much more knowledgeable could implement. So I'm not certain if the RFC should be deemed accepted, or, fall into the howto section 7(3) and go back to discussion (if it is considered a serious issue). Direction appreciated. -- Dave --- ## #95527 — Bishop Bettini — 2016-09-01T00:18:07+00:00 URL: https://externals.io/message/95236#95527 · In reply to: #95525 > > Hi all, > > As of now, the scheduled time the vote is to close, the RFC stands 19-4 > (accepted). Given this is my first solo RFC, I'm not really sure protocol > to go from here. Dmitry does raise that the implementation I put forward > maybe a bit hackish compared to what someone much more knowledgeable could > implement. So I'm not certain if the RFC should be deemed accepted, or, > fall into the howto section 7(3) and go back to discussion (if it is > considered a serious issue). > > Direction appreciated. The idea is approved. It still has to be an accepted PR. So, take the dissenting feedback as a guide to improve the test coverage and implementation. Possibly try the null coalesce approach as an alternative implementation: that might make it all simpler! If you get stuck, bounce the list a question. We're here to help. --- ## #95529 — Dmitry Stogov — 2016-09-01T07:03:42+00:00 URL: https://externals.io/message/95236#95529 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. --- ## #95537 — Pierre Joye — 2016-09-01T13:36:20+00:00 URL: https://externals.io/message/95236#95537 · In reply to: #95529 > > 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 --- ## #95541 — David Walker — 2016-09-01T16:40:15+00:00 URL: https://externals.io/message/95236#95541 · In reply to: #95529 > 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 --- ## #95551 — Dmitry Stogov — 2016-09-02T06:19:22+00:00 URL: https://externals.io/message/95236#95551 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. --- Next page: https://externals.io/message/95236.md?page=2