Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:76645
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.218.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLvSm_WP1uX5nyC2VQYMZ4wqRpWrbttKUEL3kXtgvDrZfg@mail.gmail.com>
References: <CAF+90c-JHQ4MNLEDP6pZgw6YY37j2dzUK3k26cMWdpbXa1jRGA@mail.gmail.com>
	<CA+9eiLvSm_WP1uX5nyC2VQYMZ4wqRpWrbttKUEL3kXtgvDrZfg@mail.gmail.com>
Date: Mon, 18 Aug 2014 12:26:39 +0200
Message-ID: <CAF+90c_9+8j0joNyKEp1FS7x=A5P9hg-zMHzFPAWyTFLAziJuQ@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c21b0669ccfa0500e4ced1
Subject: Re: [PHP-DEV] [RFC] Introduce Abstract Syntax Tree
From: nikita.ppv@gmail.com (Nikita Popov)

--001a11c21b0669ccfa0500e4ced1
Content-Type: text/plain; charset=UTF-8

On Mon, Aug 18, 2014 at 9:32 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi Nikita,
>
> I think RFC misses few important notes about behavior changes:
>
> 1) The behavior of $a->$b[$c] is changed. PHP apps that used such syntax
> have to be changed into $a->{$b[$c]}.
>

This is a change from the uniform variable syntax (
https://wiki.php.net/rfc/uniform_variable_syntax), not due to the AST. The
AST patch already includes the uniform variable syntax patch for technical
reasons.


> 2) The evaluation order of left and right parts of assignment is changed.
> $a[$i++] = $a[$i++]; It wasn't guaranteed before, but it may break some
> code anyway.
>

This is no longer true, the AST now implements left-to-right evaluation for
assignments.


> 3) $a=[1,2]; list($a,$b) = $a; won't work in the same way as before
>

Yes, this is true. I think assigning list() elements from left-to-right
makes a lot more sense and I also think that this is the kind of behavior
that it's okay to change in a major version. In any case, I've added this
particular example to the list() section of the RFC as well.


> 4) Usage of undefined constants now leads to fatal errors? (see
> Zend/tests/use_const/no_global_fallback.php)
>

This was a bug in the previous implementation - it uses a namespaced
constant "foo\bar\baz" and usage of undefined namespaced constants results
in a fatal error. This does not change that undefined unqualified constants
(like just "baz") will results in a notice and string fallback. This only
fixes the resolution of "use const" imports.

5) The patch also enables expressions on constant arrays e.g.
> isset(arra(1,2,3)[$x]);
>

This is also a change from the uniform variable syntax, not from the AST.


>  Personally, I would prefer to separate syntax and behavior changes from
> the AST patch itself or at least don't miss such changes, because they must
> be very significant from users point of view.
>

I think the confusion here is mainly that the AST patch already includes
the UVS patch (which was already voted in previously). I didn't list the
changes from the UVS patch in this RFC, because they're already listed in
https://wiki.php.net/rfc/uniform_variable_syntax.


> Also some implementation related notes:
>
> - Changes to Zend/tests/bug43450.phpt are unrelated to AST. It's just a
> bad test that may be fixed separately.
>

The AST makes it fail due to different memory usage patterns, which is why
I changed it to use a test compatible with different peak memory usage.

- I didn't get why you deleted Zend/tests/errmsg_014.php
>

I allowed doing $obj->__clone() calls, because __clone was the only magic
method that had a compile-time checks preventing some calls to it. If we
allow all other magic methods to be called, then there's no reason to
forbid __clone. I've added a note about this to the RFC.


> - ZEND_INIT_FCALL_BY_NAME handler shouldn't check if constant operand is
> a string. Compiler must not provide non-string constant operand. (All the
> changes to ZEND_INIT_FCALL_BY_NAME need to be verified more careful)
>

How should this be implemented instead? The case where an IS_CONST operand
is called needs to be handled somewhere. One possible approach would be to
send all constant-string calls through ZEND_INIT_FCALL. It uses the same
code code as ZEND_INIT_FCALL_BY_NAME with the difference that BY_NAME has
an extra zval literal storing the original (non-lowercased) function name.
We could store this additional function name only in cases of a
non-ct-bound function, because that's the only case where the undefined
function error can be thrown. Does that make sense?


> - The same in ext/opcache/Optimizer/optimize_func_calls.c, but it's not
> critical here.
>
> - I think you don't need IS_CONST operand for ZEND_ISSET_ISEMPTY_PROP_OBJ
> handler.
>
> - Can OPcahce always keep AST in shared memory and don't copy it into
> process memory on each request? (
> ext/opcache/zend_accelerator_util_funcs.c)
>

I'm not particularly familiar with opcache, so I'm not sure whether this is
possible. Right now this doesn't work because the constant expression AST
is heap allocated, so we have efree calls. But if we allocate the
constant-expression AST in CG(arena), maybe this would be possible?

Thanks for your review!

Nikita

--001a11c21b0669ccfa0500e4ced1--