standard: Take zend.assertions into account for dynamic calls to assert()

[TimWolla]

Fixes #18509.

[nielsdos]

While the patch is right, I wonder if we should even allow dynamic calls to assert.
There's this expectation that assert won't evaluate the arguments if assertions are disabled. Granted, using arguments that induce side-effects is iffy, but it happens.
Users, without engine knowledge, may falsily expect that dynamic calls to assert also won't evaluate the arguments. This is not really possible to do.
We already have a few functions that are forbidden to be called dynamically (search for zend_forbid_dynamic_call). I wonder if we should do the same for assert (but then only on master of course).

[TimWolla]

Granted, using arguments that induce side-effects is iffy, but it happens.

I realize that the documentation is not particularly explicit about this either. It just says “optimize to zero cost”. An example with a side-effect would probably be useful + a caution note.

We already have a few functions that are forbidden to be called dynamically (search for zend_forbid_dynamic_call).

I was not aware of that, nice. That makes sense to me. I'll send a quick email to the list, pointing towards this PR to make a decision. I guess this PR makes sense to have independent of what we decide for master, since it is consistent with assert.active at least?

[kallesommernielsen]

+1 for disabling dynamic calls in master (tbh I kinda expected it to already be the case)

[TimWolla]

I'll send a quick email to the list,

For reference: https://externals.io/message/127327#127327

[Crell]

I struggle to come up with a use case for dynamic calls to assert(). I don't know why you'd ever do that, and it's probably a bug if you do. So I'd be fine with blocking dynamic calls to make the problem go away.

[bwoebi]

This feels arbitrary to me. I see no good reason to forbid this.
Current occurrences of zend_forbid_dynamic_call() are for forbidding calling functions which manipulate the local scope - which has strong technical reasons.

For assert I see no motivations - not evaluating assert args is just an unsafe optimization, explicitly allowed by the ini. And like with all optimizations there should not be a guarantee.

[nielsdos]

I guess this PR makes sense to have independent of what we decide for master, since it is consistent with assert.active at least?

Yes I think this is already an improvement

[nielsdos]

This feels arbitrary to me. I see no good reason to forbid this. Current occurrences of zend_forbid_dynamic_call() are for forbidding calling functions which manipulate the local scope - which has strong technical reasons.

For assert I see no motivations - not evaluating assert args is just an unsafe optimization, explicitly allowed by the ini. And like with all optimizations there should not be a guarantee.

This depends on your perspective. From my perspective, there is a technical reason: we can't optimize this case when assert is called dynamically. We have some true unsafe optimization options for opcache but they are disabled by default.

[bwoebi]

we can't

Well, yes, we could add e.g. check it directly in INIT_DYNAMIC_CALL and do a jump to past the fcall. (Making it fully zero-cost would be harder, but still).
But you would not want that, because it would be a spooky action at a distance - suddenly the args would not be evaluated.

[nielsdos]

suddenly the args would not be evaluated.

Which is what I would expect

[bwoebi]

I see, here we disagree :-) I'd expect this only to happen on explicit calls, where it is clearly visible that something is an assert() call.

[TimWolla]

Given the disagreement, changing whether or not dynamic calls to assert() are allowed definitely requires an RFC (which I am not interested in doing).

I'll merge this PR as an obvious bugfix tomorrow, though.