Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Implement final anonymous classes #11126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danog
Copy link
Contributor

@danog danog commented Apr 24, 2023

This should also allow some additional opcache optimizations.

@iluuu1994
Copy link
Member

I would prefer them being made final by default, as I doubt many people are relying on this behavior. Either way, that needs some discussion on the ML. 🙂

@danog
Copy link
Contributor Author

danog commented Apr 24, 2023

I would prefer them being made final by default

100% agree, though this would probably require an RFC (?).

Submitted this in the meantime as it doesn't involve breaking changes, will post something to the internals mailing list regarding this MR and other possible options :)

@heiglandreas
Copy link
Contributor

As it might be a BC break I think an RFC would be a good idea

@KapitanOczywisty
Copy link

I would prefer them being made final by default, as I doubt many people are relying on this behavior.

Unfortunately it is officially supported [bug][fix]

@iluuu1994
Copy link
Member

@KapitanOczywisty Indeed. That's probably more of an oversight of the original RFC given that it doesn't talk about it at all. Both solutions are languages changes and as such need an RFC.

@mvorisek
Copy link
Contributor

I would prefer them being made final by default, as I doubt many people are relying on this behavior.

Unfortunately it is officially supported [bug][fix]

the support is needed for on-the-fly generated proxy classes at least

@iluuu1994
Copy link
Member

iluuu1994 commented Apr 24, 2023

@mvorisek Not really. The class needs to be named via class alias for this to work anyway, so why not just name the original class? (That does require eval though).

@mvorisek
Copy link
Contributor

@iluuu1994 proxy classes are generated like:

class Proxy extends OrigClass {
    public __get($name): mixed { ... }
}

where OrigClass is an original class name or aliased name (as anonymous class name cannot be parsed).

I am pointing out that making anonymous class final implicitly would break ORMs which relies on it.

@KapitanOczywisty
Copy link

@mvorisek Can you link something that uses this in the wild? That snippet doesn't make much sense, since __get works only for undefined properties, unless __get method is "proxied" by itself. Also OrigClass name is one use only, so every proxied class is getting own name and proxy class is eval'ed? I'd like to see working example to understand how is this used.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 24, 2023

Doctrine ORM uses that for example (for properties lazy-load, real properties are unset in construct) and anonymous class is allowed AFAK.

@iluuu1994
Copy link
Member

proxy classes are generated like: ... where OrigClass is an original class name or aliased name (as anonymous class name cannot be parsed).

But how would that work? First you need an instance of your anonymous class so that you can extract the class name, create a unique alias to it and pass the alias to the proxy factory. Furthermore, the anonymous class could not be autoloaded and so neither could the proxy class. That sounds dubious at best.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 24, 2023

here is example of proxied anonymous class: https://3v4l.org/NoPDi/rfc#vgit.master

@iluuu1994
Copy link
Member

The question wasn't really whether it was possible, but whether it was useful. I don't see why anybody would write such code.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 24, 2023

But many ORM libs do so. They accept a class/instance, proxy it for lazy loading, and it should not matter if you pass standard or anonymous class to them, this is working now and as long as there is no substancial perfomance improvement (from better JITed code), it should be kept supported.

@nicolas-grekas
Copy link
Contributor

Before merging this (if we do), I'd suggest forbidding using class_alias on such classes.

@iluuu1994
Copy link
Member

@nicolas-grekas No worries, neither solution should just be merged. Given that this is a language change and there was some controversy an RFC is warranted.

I'd suggest forbidding using class_alias on such classes.

Note that the main reason for this PR is to enable some additional optimizations for anonymous classes that are almost exclusively final, i.e. for final classes we know that no "dynamic dispatch" is possible, and so can rely on method signatures. I don't think having aliases to anonymous classes itself causes any issues.

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Apr 25, 2023

OK thanks. I'm making this proposal to forbid aliasing final anonymous classes so that they can truly be considered anonymous. Currently, you can give an addressable name to an anonymous class via aliasing, and that's a leak IMHO. While I don't think we can close it for existing anonymous classes, we could for final ones.

@iluuu1994
Copy link
Member

Note that, even if we disallow class_alias to anonymous classes, many constructs still accept class names that aren't otherwise allowed syntactically. E.g. https://3v4l.org/SMID8

@danog danog changed the title Implement final anonymous classes [RFC] Implement final anonymous classes Nov 15, 2023
@danog danog marked this pull request as ready for review December 3, 2023 12:31
@danog danog requested a review from iluuu1994 as a code owner December 3, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants