Preserve the original AST 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().