Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119906
MIME-Version: 1.0
References: <CAFPFaMLghRYP4ioCkfa7WKhTAz+1gWgcZ5mWYAunkb9f3g=fdA@mail.gmail.com>
 <6FA5AA43-9738-4400-8D83-38FC2D464B82@gmail.com>
In-Reply-To: <6FA5AA43-9738-4400-8D83-38FC2D464B82@gmail.com>
Date: Tue, 11 Apr 2023 12:16:18 +0100
Message-ID: <CAFPFaMLLLKDbs7xtykgibrD_FyjLv98f_sjDw650bHyVWKyg2g@mail.gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary="00000000000085196d05f90d9fc0"
Subject: Re: [PHP-DEV] [RFC] New core autoloading mechanism with support for
 function autoloading
From: george.banyard@gmail.com ("G. P. B.")

--00000000000085196d05f90d9fc0
Content-Type: text/plain; charset="UTF-8"

On Tue, 11 Apr 2023 at 09:48, Rowan Tommins <rowan.collins@gmail.com> wrote:

> > As a result of the pinning, function_exists() will return true for
> functions in namespaces that have been pinned to a global function.
>
> The lookup pinning seems like the right way to optimise the
> implementation, but I don't think it should be visible to user code like
> this - a lookup for "strlen" in namespace "Foo" is not the same as defining
> the function "Foo\strlen". Consider this code:
>
> namespace Bar {
>     if ( function_exists('Foo\strlen') ) {
>         \Foo\strlen('hello');
>     }
> }
> namespace Foo {
>      strlen('hello'); // triggers name pinning
> }
> namespace Bar {
>     if ( function_exists('Foo\strlen') ) {
>         \Foo\strlen('hello');
>     }
> }
>
> If I'm reading the RFC correctly, the second function_exists will return
> true. I'm less clear if the call to \Foo\strlen will actually succeed - if
> it gives "undefined function", then function_exists is clearly broken; if
> it calls the global strlen(), that's a very surprising side effect.
>

It *should* indeed call the global strlen() but I hadn't actually created
such a test as ... I hadn't thought of doing that.
However, we *already* do function pinning which can result in this
behaviour via the function cache, see the following bug which defines a new
function via eval():
https://bugs.php.net/bug.php?id=64346

I am not sure that it calling the global strlen() is that surprising, as it
is basically aliasing the function \Foo\strlen() to \strlen().

For bonus points, the call to strlen that triggers pinning could be inside
> an autoloader, making even the first function_exists call return true.
>

If it is in the same namespace as the autoloader, then yes. However, if the
autoloader is in Bar then only Bar\strlen() is being aliased to \strlen().
Ilija mentioned this off-list, and I hadn't considered this, but this could
lead to a large increase of symbols being defined in the function symbol
table, as every nonqualified call (either by using the "use" statement, or
writing the full FQN) will get aliased to a global function and take an
entry in the symbol table.


> Similarly, I think it should be possible to "unpin" a function lookup with
> a later definition, even if no autoloading would be triggered. That is,
> this should not be a duplicate definition error:
>
> namespace Foo;
> if ( strlen('magic') != 42 ) {
>     function strlen($string) { /* ... */ }
> }
>

There are some larger technical issues at play, as mentioned in the
previous bug.
The function cache will pin the call and there is no way of unpinning it. I
tried looking into fixing this, but it turns out to be too complicated
(/impossible?).
More so, disabling the function cache is a massive performance penalty.
As such, the RFC follows the current de facto behaviour.



> > The use of the word class in the API is currently accurate
>
> This isn't actually true: classes, interfaces, traits, and enums all share
> a symbol table, and thus an autoloader. I don't know of a good name for
> this symbol table, though.
>

They do share a symbol table indeed but using class is probably the least
confusing one.


> Regarding the API, would it be possible to take advantage of nearly all
> autoloaders only being interested in particular namespace prefixes?
>
> Currently, every registered autoloader is run for every lookup, and most
> immediately check the input for one or two prefixes, and return early if
> not matched. I suspect this design is largely because autoloading came
> before namespaces, so the definition of "prefix" wasn't well-defined, but
> going in and out of userland callbacks like this is presumably rather
> inefficient.
>
> Perhaps the "register" functions should take an optional list of namespace
> prefixes, so that the core implementation can do the string comparison, and
> only despatch to the userland code if the requested class/function name
> matches.
>

That is actually interesting, hadn't thought about taking an array of
prefixes.
And yes, every callback call requires a VM re-entry, which is expensive.

Should the prefix be with or without the trailing backlash?

Best regards,

George P. Banyard

--00000000000085196d05f90d9fc0--