# RFC: Expectations - Thread: https://externals.io/message/82797 - Original archive: https://news-web.php.net/php.internals/82797 - Started by Joe Watkins on 2015-02-16T09:37:07+00:00 - Messages: 19 - 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 --- ## #82797 — Joe Watkins — 2015-02-16T09:37:07+00:00 URL: https://externals.io/message/82797#82797 Morning internals, I've had mostly negative feedback on the design-by-contract idea, maybe it's a step too far. The expectations RFC in its current form proposes that we introduce zero-cost assertions, which are compatible with the current assertion API. Since this has been in discussion for quite some time, Dmitry and I propose that the patch is rewritten for PHP7 and we get to voting as quickly as we are able. Please review the RFC, and provide feedback, if there are no strong objections we will go ahead with a vote when the patch is ready, in the coming days. https://wiki.php.net/rfc/expectations Cheers Joe --- ## #82798 — Yasuo Ohgaki — 2015-02-16T09:45:17+00:00 URL: https://externals.io/message/82797#82798 · In reply to: #82797 Hi Joe, > I've had mostly negative feedback on the design-by-contract idea, maybe > it's a step too far. > I think introducing strict type safety for DbC like static/compiled languages is too far. I agree. The expectations RFC in its current form proposes that we introduce > zero-cost assertions, which are compatible with the current assertion API. > > Since this has been in discussion for quite some time, Dmitry and I > propose that the patch is rewritten for PHP7 and we get to voting as > quickly as we are able. > > Please review the RFC, and provide feedback, if there are no strong > objections we will go ahead with a vote when the patch is ready, in the > coming days. > Big +1 Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #82799 — Zeev Suraski — 2015-02-16T09:47:12+00:00 URL: https://externals.io/message/82797#82799 · In reply to: #82798 > -----Original Message----- > From: yohgaki@gmail.com [mailto:yohgaki@gmail.com] On Behalf Of Yasuo > Ohgaki > Sent: Monday, February 16, 2015 11:45 AM > To: Joe Watkins > Cc: PHP internals > Subject: Re: [PHP-DEV] RFC: Expectations > > Hi Joe, > > On Mon, Feb 16, 2015 at 6:37 PM, Joe Watkins > wrote: > > > […] > > I think introducing strict type safety for DbC like static/compiled > languages is > too far. I agree. > > The expectations RFC in its current form proposes that we introduce > > […] > > Big +1 > > Regards, From me as well. Zeev --- ## #82801 — Martin Jansen — 2015-02-16T09:59:25+00:00 URL: https://externals.io/message/82797#82801 · In reply to: #82797 > Please review the RFC, and provide feedback, if there are no strong > objections we will go ahead with a vote when the patch is ready, in the > coming days. > > https://wiki.php.net/rfc/expectations I like it. But why do those two INI settings use different "namespaces"? Why not call zend.assertions something like assert.enable? Also I'm wondering if the default value for zend.assertions should better be 0? - Martin --- ## #82802 — Yasuo Ohgaki — 2015-02-16T10:03:12+00:00 URL: https://externals.io/message/82797#82802 · In reply to: #82801 Hi Martin, > > > […] > > I like it. But why do those two INI settings use different "namespaces"? > Why not call zend.assertions something like assert.enable? Also I'm > wondering if the default value for zend.assertions should better be 0? It requires compiler switch. If we don't have INI, then there should declare(), but it requires script modification to enable/disable. It just does not work. That's the reason why. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #82804 — Crypto Compress — 2015-02-16T10:11:23+00:00 URL: https://externals.io/message/82797#82804 · In reply to: #82797 Hi Joe, is there any penalty of catch-AssertionException-blocks? Are those eliminated in production code? Thanks --- ## #82805 — Benjamin Eberlei — 2015-02-16T10:13:46+00:00 URL: https://externals.io/message/82797#82805 · In reply to: #82797 > Morning internals, > > I've had mostly negative feedback on the design-by-contract idea, maybe > it's a step too far. > > The expectations RFC in its current form proposes that we introduce > zero-cost assertions, which are compatible with the current assertion API. > > Since this has been in discussion for quite some time, Dmitry and I > propose that the patch is rewritten for PHP7 and we get to voting as > quickly as we are able. > > Please review the RFC, and provide feedback, if there are no strong > objections we will go ahead with a vote when the patch is ready, in the > coming days. > Could you improve the PHP.INI section and explain in detail what the two new settings do? i think its somewhere implicitly described in the RFC but there is never an explicit description. This section should probably be it. If this ini setting affects opcode generation, have you thought about invalidation when changing the setting? Do I understand correctly that this is a rebuilt of the already existing `assert()` functionality? What about the old behavior? You mention its compatible, but `assert()` api had a switch to decide over Exceptions vs warnings vs quietly skip. i think the RFC should focus more on this being an improvement of assert. It sounds like a completly new feature. For example: 1. you are changing assert to be an opcode instead of a normal function 2. adding the new Exception mode and that it formalizes what you could do before by using the callback mode: assert_options(ASSERT_CALLBACK, function () { throw new AssertionException(); }); Otherwise i am +1 on this. It fixes some annoying features of the assertion API. > > https://wiki.php.net/rfc/expectations > > Cheers > Joe --- ## #82808 — Joe Watkins — 2015-02-16T10:32:30+00:00 URL: https://externals.io/message/82797#82808 · In reply to: #82804 > is there any penalty of catch-AssertionException-blocks? Are those eliminated in production code? Assertions should never be enabled in production code, the ability to catch (and enable by configuration) the exception serves the developer of the code during development only. The example code is bad, I'll clear this up. Cheers Joe On Mon, Feb 16, 2015 at 10:11 AM, Crypto Compress < cryptocompress@googlemail.com> wrote: > Hi Joe, > > is there any penalty of catch-AssertionException-blocks? Are those > eliminated in production code? > > Thanks --- ## #82856 — Dmitry Stogov — 2015-02-16T15:47:34+00:00 URL: https://externals.io/message/82797#82856 · In reply to: #82808 Hi Nikita, it looks like a part of old implementation is not trivial with new AST compiler. previously we translated assert(condition) into assert(condition, "assert(condition)"). actually we just captured a part of input buffer and added missing string argument. Is there a simple way to do the same now? It's possible to convert AST into string using recursive pretty-printer. It's not a simple task itself, but it may be reused for other things. Joe, the rest (including zero-cost assert) is implemented at https://github.com/php/php-src/pull/1088/files 9 related tests are failed for now. Thanks. Dmitry. > > […] > eliminated in production code? > > Assertions should never be enabled in production code, the ability to catch > (and enable by configuration) the exception serves the developer of the > code during development only. > > The example code is bad, I'll clear this up. > > Cheers > Joe > > On Mon, Feb 16, 2015 at 10:11 AM, Crypto Compress < > cryptocompress@googlemail.com> wrote: > > > […] --- ## #82961 — Nikita Popov — 2015-02-17T14:11:14+00:00 URL: https://externals.io/message/82797#82961 · In reply to: #82856 > Hi Nikita, > > it looks like a part of old implementation is not trivial with new AST > compiler. > > previously we translated assert(condition) into assert(condition, > "assert(condition)"). > actually we just captured a part of input buffer and added missing string > argument. > > Is there a simple way to do the same now? > Not very simple, but we could store pointers/offsets to the start and end of the fcall in the ast node, similar to what is done with lex_pos for declarations: http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_ast.h#177 It would be nice if we could have precise offset information for all nodes (this is particularly valuable if the ast is exposed to userland), but that would increase memory usage during compilation, not sure if it's worthwhile. > It's possible to convert AST into string using recursive pretty-printer. > It's not a simple task itself, but it may be reused for other things. > > Joe, the rest (including zero-cost assert) is implemented at > https://github.com/php/php-src/pull/1088/files > 9 related tests are failed for now. > As this is implemented right now, it would require writing \assert() in order to be zero-cost. Maybe we should disallow redefinition of assert as a namespaced function, so we can always optimize this? Nikita --- ## #82976 — Dmitry Stogov — 2015-02-17T15:51:49+00:00 URL: https://externals.io/message/82797#82976 · In reply to: #82961 > > > > […] > > Not very simple, but we could store pointers/offsets to the start and end > of the fcall in the ast node, similar to what is done with lex_pos for > declarations: http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_ast.h#177 > > It would be nice if we could have precise offset information for all nodes > (this is particularly valuable if the ast is exposed to userland), but that > would increase memory usage during compilation, not sure if it's worthwhile. > > > > […] > I went by this way... :) It must be ready soo. > > > […] > > As this is implemented right now, it would require writing \assert() in > order to be zero-cost. Maybe we should disallow redefinition of assert as a > namespaced function, so we can always optimize this? > good catch. Thanks. > > Nikita --- ## #83009 — Dmitry Stogov — 2015-02-17T21:50:09+00:00 URL: https://externals.io/message/82797#83009 · In reply to: #82976 Hi Joe The patch is ready https://github.com/php/php-src/pull/1088/files 1) I implemented AST pretty-printer to reconstruct the source. It may be reused in Reflection and other places through ZEND_API zend_string *zend_ast_export(const char *prefix, zend_ast *ast, const char *suffix); 2) zend.assertions=-1 - makes zero-cost asserts 3) `assert()` in a namespace leads to call a function defined in this namespace (if it's defined), but zend.assertions is still may disable this call or even prevent code generation for it. it's possible to use \assert() to call the system function. Please, make update RFC, add notes about (2) and (3). Then, it should be ready for voting. Nikita, please take a quick look over the patch. I hope, you don't have objections. Thanks. Dmitry. > > > On Tue, Feb 17, 2015 at 5:11 PM, Nikita Popov > wrote: > > > […] > I went by this way... :) > It must be ready soo. > > > > […] > > good catch. > > Thanks. > > > > […] --- ## #83023 — Joe Watkins — 2015-02-18T06:23:45+00:00 URL: https://externals.io/message/82797#83023 · In reply to: #83009 Will update RFC today, thanks for working on it Dmitry :) Cheers Joe > Hi Joe > > The patch is ready https://github.com/php/php-src/pull/1088/files > > 1) I implemented AST pretty-printer to reconstruct the source. It may be > reused in Reflection and other places through > > ZEND_API zend_string *zend_ast_export(const char *prefix, zend_ast *ast, > const char *suffix); > > 2) zend.assertions=-1 - makes zero-cost asserts > > 3) `assert()` in a namespace leads to call a function defined in this > namespace (if it's defined), but zend.assertions is still may disable this > call or even prevent code generation for it. it's possible to use \assert() > to call the system function. > > Please, make update RFC, add notes about (2) and (3). > Then, it should be ready for voting. > > Nikita, please take a quick look over the patch. I hope, you don't have > objections. > > Thanks. Dmitry. > > > > > > > > […] --- ## #83028 — Joe Watkins — 2015-02-18T07:16:40+00:00 URL: https://externals.io/message/82797#83028 · In reply to: #83023 Updated RFC as requested. I'm just going to wait to hear what Nikita says, especially about optimizing away unqualified calls to assert. I guess this could be viewed as a BC break, it seems like quite a nice break but not sure. Will open voting when we hear from Nikita. Cheers Joe > Will update RFC today, thanks for working on it Dmitry :) > > Cheers > Joe > > > > > […] --- ## #83029 — Dmitry Stogov — 2015-02-18T07:22:00+00:00 URL: https://externals.io/message/82797#83029 · In reply to: #83028 OK thanks. I'm switching to another task :) Thanks. Dmitry. > Updated RFC as requested. > > I'm just going to wait to hear what Nikita says, especially about > optimizing away unqualified calls to assert. > I guess this could be viewed as a BC break, it seems like quite a nice > break but not sure. > > Will open voting when we hear from Nikita. > > Cheers > Joe > > On Wed, Feb 18, 2015 at 6:23 AM, Joe Watkins > wrote: > > > […] --- ## #83071 — Nikita Popov — 2015-02-18T14:44:11+00:00 URL: https://externals.io/message/82797#83071 · In reply to: #83009 > Hi Joe > > The patch is ready https://github.com/php/php-src/pull/1088/files > > 1) I implemented AST pretty-printer to reconstruct the source. It may be > reused in Reflection and other places through > > ZEND_API zend_string *zend_ast_export(const char *prefix, zend_ast *ast, > const char *suffix); > > 2) zend.assertions=-1 - makes zero-cost asserts > > 3) `assert()` in a namespace leads to call a function defined in this > namespace (if it's defined), but zend.assertions is still may disable this > call or even prevent code generation for it. it's possible to use \assert() > to call the system function. > > Please, make update RFC, add notes about (2) and (3). > Then, it should be ready for voting. > > Nikita, please take a quick look over the patch. I hope, you don't have > objections. > I've added a few comments on the PR. Two general notes on the RFC: 1. I don't like the ability to specify a different exception as the second param. Assertions are supposed to be used as sanity checks during development, not to throw meaningful and specialized exception types. Having this possibility will probably only encourage bad usage of `assert()`. It's not a big problem to me, but I'd rather not have this "feature". 2. Similar to the EngineExceptions RFC I'm wondering if AssertionException should extend Exception or be in a separate hierarchy. The same argument as with engine exceptions applies: It's pretty unlikely that you want to catch an AssertionException anywhere apart from top-level error handling code and that people using catch(Exception) blocks would accidentally catch assertions. I'm not sure I agree with this, but I wanted to mentioned the concern. Nikita --- ## #83093 — Dmitry Stogov — 2015-02-18T17:01:27+00:00 URL: https://externals.io/message/82797#83093 · In reply to: #83071 > > > > […] > > I've added a few comments on the PR. > Thank you very much. You found about 25 bugs :) All of them except for "elseif" should be fixed now. I also think printing "else if" instead of "elseif" is not a big problem. Pretty-printer may also add or remove brackets in some situations. > > Two general notes on the RFC: > > 1. I don't like the ability to specify a different exception as the second > param. Assertions are supposed to be used as sanity checks during > development, not to throw meaningful and specialized exception types. > Having this possibility will probably only encourage bad usage of `assert()`. > It's not a big problem to me, but I'd rather not have this "feature". > Joe, this is part of your old patch. I really don't care about it. > 2. Similar to the EngineExceptions RFC I'm wondering if AssertionException > should extend Exception or be in a separate hierarchy. The same argument as > with engine exceptions applies: It's pretty unlikely that you want to catch > an AssertionException anywhere apart from top-level error handling code and > that people using catch(Exception) blocks would accidentally catch > assertions. I'm not sure I agree with this, but I wanted to mentioned the > concern. > This may be changed together with EngineException patch. I started working on it, and I hope I'll show you results tomorrow. Thanks. Dmitry. > > Nikita --- ## #83155 — Joe Watkins — 2015-02-19T07:58:40+00:00 URL: https://externals.io/message/82797#83155 · In reply to: #83093 Morning, I hear the concern regarding custom exceptions. Things will be used badly whatever. It's easy to imagine that in a simple application you just don't need to specify custom exceptions. But in a large codebase, being able to structure exceptions is invaluable, it gives documentation reference points and makes stack traces easier to read at a glance, because they are more meaningful. So I think we should keep the ability to throw custom exceptions. I agree about making the exception part of a new tree, so that they are not caught, so I'll just wait for that from dmitry and open the vote. Cheers Joe > > > On Wed, Feb 18, 2015 at 5:44 PM, Nikita Popov > wrote: > > > […] > > Thank you very much. You found about 25 bugs :) > All of them except for "elseif" should be fixed now. > I also think printing "else if" instead of "elseif" is not a big problem. > Pretty-printer may also add or remove brackets in some situations. > > > > […] > > Joe, this is part of your old patch. I really don't care about it. > > > > […] > > This may be changed together with EngineException patch. > I started working on it, and I hope I'll show you results tomorrow. > > Thanks. Dmitry. > > > > […] --- ## #83157 — Dmitry Stogov — 2015-02-19T08:28:35+00:00 URL: https://externals.io/message/82797#83157 · In reply to: #83155 Hi Joe, I think, we may just add additional vote question. - If we like to custom exceptons as second argument? but I don't care about it, so do as you like. According to AssertException: add a note into Open Issues section. e.g. we may introduce new abstract class BaseException then make Excepton to be a child of BaseException then introduce and use for assert AssertException (direct child of BaseException) All this changes are going to be implemented as part of Engine Exception RFC. And open the vote. Thanks. Dmitry. > Morning, > > I hear the concern regarding custom exceptions. Things will be used badly > whatever. It's easy to imagine that in a simple application you just don't > need to specify custom exceptions. But in a large codebase, being able to > structure exceptions is invaluable, it gives documentation reference points > and makes stack traces easier to read at a glance, because they are more > meaningful. > > So I think we should keep the ability to throw custom exceptions. > > I agree about making the exception part of a new tree, so that they are > not caught, so I'll just wait for that from dmitry and open the vote. > > Cheers > Joe > > > > > […]