Skip to content

PFA: Fix magic method resolution - #23251

Open
arnaud-lb wants to merge 1 commit into
php:masterfrom
arnaud-lb:pfa-bug-2
Open

PFA: Fix magic method resolution#23251
arnaud-lb wants to merge 1 commit into
php:masterfrom
arnaud-lb:pfa-bug-2

Conversation

@arnaud-lb

@arnaud-lb arnaud-lb commented Aug 13, 2026

Copy link
Copy Markdown
Member

We conveniently set the scope of generate PFA closures to the function's scope as this allows const exprs referencing self:: or parent:: to behave normally:

class C {
    const VAL = 1;
    function f($a, $b = self::VAL) {}
}

$f = (new C)->f(1, ...);

// Generates the following closure with scope=C:
// self::VAL resolves to C::VAL due to scope.
$f = function ($b = self::VAL) {
    $this->f(1, $b);
};

However this affects method resolution for magic methods:

class C {
    private static function priv($a) {}
    public static function __callStatic($name, $args) {}
}

// Sees only C::__callStatic() 
$f = C::priv(?);

// Generates the following closure with scope=C: 

$f = function ($arguments0) {
    // Sees C::priv()
    static::priv($arguments0);
};

Fix by using the actual scope for PFAs of magic methods.

This should be enough as long as the generated closure doesn't inherit any self:: or parent:: expression from the magic method. Currently that's the case. If this changes we may need to rewrite these expressions, which I would like to avoid as this increases complexity and maintenance overhead a bit.

The test magic_scope.phpt demonstrates the issue. default_arg_scope.phpt just checks relative-class resolution in default argument values.

Bug found by Ryan @ Calif.io

We conveniently use the function's scope for the scope of the generated closure
as this allows const exprs referencing self:: or parent:: to behave normally.

However this affects method resolution for magic methods. Fix by using the
actual scope for PFAs of magic methods.
@arnaud-lb
arnaud-lb marked this pull request as ready for review August 13, 2026 16:33
@arnaud-lb
arnaud-lb requested a review from dstogov as a code owner August 13, 2026 16:33

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any problems with this.

One question: would resolving self/parent at compile time for traits remove the possible concern about needing to rewrite?

As I cannot see this really being a problem with an unbound closure being partially applied. But maybe I'm missing something.

<?php

if (getenv('A')) {
/* Relative class references are never resolved at compile time on traits */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I was trying to resolve them at one point, but was hitting issues with the JIT. So I guess this might be revisited at one point.

?>
--EXPECTF--
Closure [ <user> static public method {closure:%s:%d} ] {
Closure [ <user> static function {closure:%s:%d} ] {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches more what:

<?php

class C {
    private static function priv($a) { echo "private priv\n"; }
    public static function __callStatic($name, $args) { echo "trampoline $name\n"; }
    public static function foo() {}
}

// Sees only C::__callStatic() 
$f = C::priv(...);
echo (string) new ReflectionFunction($f);
$g = C::foo(...);
echo (string) new ReflectionFunction($g);

produces (although that output is also a bit confusing):

Closure [ <internal> static public method priv ] {

  - Parameters [1] {
    Parameter #0 [ <optional> mixed ...$arguments ]
  }
}
Closure [ <user> static public method foo ] {
  @@ /in/AItpa 6 - 6
}

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.

2 participants