On Thu, Mar 6, 2025, at 09:04, Tim Düsterhus wrote:
> Hi
>
> Am 2025-03-06 07:23, schrieb Rob Landers:
> > So, technically, they aren’t required to be in the same RFC; but also,
> > they complement each other very well.
>
> They really should be separate RFCs then. Your RFC text acknowledges
> that in the very first sentence: “two significant enhancements to the
> language”. Each individual proposal likely has sufficient bike-shedding
> potential on its own and discussion will likely get messy, because one
> needs to closely follow which of the two proposals an argument relates
> to.
I put a lot of thought into this issue off and on, all day. I've decided to
remove short syntax from the RFC and focus on inner classes. If this passes,
then I will propose it as a separate RFC. Introducing them concurrently makes
little sense in light of the feedback I have gotten so far, and it is turning
out that there is much more to discuss than I initially expected.
Thus, I will skip replying about short classes.
>
> As for the “Inner classes” proposal:
>
> - “abstract is not allowed as an inner class cannot be parent classes.”
> - Why?
This is mostly a technical reason, as I was unable to determine a grammar rule
that didn't result in ambiguity. Another reason is to ensure encapsulation and
prevent usages outside their intended scope. We can always add it later.
> - “type hint” - PHP does not have type hints, types are enforced. You
> mean “Type declaration”.
Thank you for pointing this out! I learned something new today! I've updated
the RFC.
> - “this allows you to redefine an inner class in a subclass, allowing
> rich hierarchies” - The RFC does not specify if and how this interacts
> with the LSP checks.
It doesn't affect LSP. I've updated the RFC accordingly.
On Thu, Mar 6, 2025, at 20:08, Niels Dossche wrote:
> Hi Rob
>
> Without looking too deep (yet) into the details, I'm generally in favor of
> the idea.
> What I'm less in favor of is the implementation choice to expose the inner
> class as a property/const and using a fetch mode to grab it.
> That feels quite weird to me honestly. How did you arrive at this choice?
>
> Kind regards
> Niels
It's a slightly interesting story about how I arrived at this particular
implementation. If you noticed the branch name, this is the second
implementation. The first implementation used a dedicated list on the
class-entry for inner classes. Since I wanted to prevent static property/consts
from being declared with the same name, I had just set it to a string of the
full class name as a placeholder. That implementation also required some pretty
dramatic OPcache changes, which I didn't like. At one point, I went to add the
first test that did `new Outer::Inner()` and the test passed...
You can imagine my surprise to see a test pass that I had expected to fail, and
it was then that I went into the details of what was going on. Any `new
ClassName` essentially results in the following AST:
ZEND_AST_NEW
-- ZEND_AST_ZVAL
-- "ClassName"
-- (... args)
The original grammar, at the time, was to reuse the existing static property
access AST until I could properly understand OPcache/JIT. My change had
resulted in (approximately) this AST:
ZEND_AST_NEW
-- ZEND_AST_ZVAL
-- ZEND_AST_STATIC_PROP
-- "Outer::Inner"
-- (... args)
Which, effectively resulted in emitting opcodes that found the prop + string
value I happened to put there as a placeholder until I figured out a better
solution, handling autoloading properly and everything. This pretty much
negated all efforts up to that point, and I was stunned.
So, I branched off from an earlier point and eventually wrote the version you
see today. It's 1000x simpler and faster than the original implementation
(literally), since it uses all pre-existing (optimized)) infrastructure instead
of creating entirely new infrastructure. It doesn't have to check another
hashmap (which is slow) for static props vs. constants vs. inner classes.
In essence, while the diff can be improved further, it is quite simple; the
core of it is less than 500 lines of code.
I'd recommend leaving any comments about the PR on the PR itself (or via
private email if you'd prefer that). I'm by no means an expert on this code
base, and if it is not what you'd expect, being an expert yourself, I'd love to
hear any suggestions for improvements or other approaches.
On Thu, Mar 6, 2025, at 20:33, Larry Garfield wrote:
> My biggest concern with this is that it makes methods and short-classes
> mutually incompatible. So if you have a class that uses short-syntax, and as
> it evolves you realize it needs one method, sucks to be you, now you have to
> rewrite basically the whole class to a long-form constructor. That sucks
> even more than rewriting a short-lambda arrow function to a long-form
> closure, except without the justification of capture semantics.
I literally fell out of my chair laughing. Thanks for that, and it is true. I
look forward to discussing this further!
> ## Inner classes
>
> I'm on board with the use case. What I'm not sure on is inner classes vs
> file-private visibility, something that Ilija was working on at one point and
> Michał Brzuchalski suggested in his post. Both solve largely the same
> problem with different spelling.
>
> Arguably, inner classes have fewer issues with current autoload conventions.
> I must ponder this further.
Indeed! See: https://externals.io/message/126331#126337
>
> > However, no classes may not inherit from inner classes, but inner classes
> > may inherit from other classes, including the outer class.
>
> I think you have one too many negatives in that sentence.
Thank you, this is fixed!
— Rob
