Fix AST export crash for closures in const-expressions

[TimWolla]

Fixes GH-17096.

[iluuu1994]

Tbh, I would prefer a simple solution, e.g. by printing a placeholder in the ast dumper. Accurate ast dumping is not very important.

[TimWolla]

That would also be fine with me.

Nevertheless I would suggest to keep the first commit (I can send a separate PR for that), as it it definitely fixes a bug in handling of zend_ast_decl nodes that is not (easily) detectable with sanitizers. It will just work on garbage values.

[bwoebi]

@iluuu1994 I actually like the full AST being there. While it's not fundamentally necessary for usability, it's nice to have I think.

[iluuu1994]

One difference imo is that closures may grow large, in comparison to singular expressions. This ast is almost never used, but will still take up shm space with little benefit.

[bwoebi]

@iluuu1994 I doubt the amount of closures within attributes will be significant enough to warrant considerations in that regard.

[iluuu1994]

@bwoebi You're right, probably not. However, it's also a weird exception. E.g. we don't attempt to preserve the AST in other cases. Without opcache, the AST is discarded after the expression has been evaluated, and a lot of time expressions get folded into some concrete value at compile time. So, I'm just wondering why we need to make an exception here.

[bwoebi]

@iluuu1994 But it's also a weird exception to not preserve Closure contents, but everything else in an attribute.

[iluuu1994]

@bwoebi But again, "preserve" is a stretch, compile-time optimizations still apply. I don't believe we preserve the AST solely for the purposes of printing, rather than evaluating. Even assert() collapses the AST at compile-time.

But ok, I don't object strongly to this, so feel free to go with this approach if you prefer.

[TimWolla]

@iluuu1994 I don't have a strong opinion either way. Shall I extract the first commit (which I consider to be a bugfix) into a separate PR to allow for an independent review?

[iluuu1994]

@TimWolla If we decide not to store the AST in this case, it might be better to just add some ZEND_ASSERT(!zend_ast_is_decl(ast)); conditions. WDYT? Otherwise we're just adding unused and untested code.

[TimWolla]

WDYT? Otherwise we're just adding unused and untested code.

That makes sense to me. With the assertions being there in all the locations, (re)doing the implementation is straight forward. The complicated part was finding all the locations.

[arnaud-lb]

I don't know what would be the most useful between displaying the source code or displaying a placeholder with the closure location (filename / lineno).

Currently we don't expose a closure's source code in var_dump() or in Reflection, and it's usually fine.

Otherwise, given that we primarily store the AST for evaluation purposes, and the AST is already evaluated in this case (to an op_array), maybe we can store a string instead? This would take less space, and would be comparable to a doc comment. I believe that this is what we do for assert().

[edorian]

I'd be good with just producing a placeholder and not adding more bits to the runtime memory for this.

But maybe I'm missing some context and don't understand the benefits or potential use cases for this, and I'm overlooking some reason why this is useful.

• If PHP wants to support restoring/creating a PHP script from runtime data (without the source files) via reflection and bits of AST (or stringified AST) then this would obviously make sense, but I'm not aware of this being a goal.
• Projects like php-parser or static analysers also shouldn't benefit much from this?
• Are there error messages that would be worse for this having the code?

So without a concrete use case at hand, I'd argue for dropping it, and otherwise I'm happy to change my mind.

[TimWolla]

Okay, I've opted to show a placeholder there, but left commits that store the AST as a string and the original implementation that stores the full AST in the commit history, should we want to decide differently.

Regarding the “store as string” variant, it should be noted that it naturally cannot take the indent and priority parameter of zend_ast_export_ex into account and thus it might also be subtly wrong. Thus the obvious placeholder value might be preferable after all.

[iluuu1994]

Code looks correct 👍 Not necessarily a fan of more ast types, but not objecting.