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

Implement property hooks via virtual properties #166

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

thekid
Copy link
Member

@thekid thekid commented May 13, 2023

This pull request adds support for property hooks to all PHP versions

Example

This example calculates and caches the full name from first and last names

class User {
  private string $full;
  private string $first { set { $field= $value; unset($this->full); } }
  private string $last { set { $field= $value; unset($this->full); } }
  public string $fullName => $this->full??= $this->first.' '.$this->last;

  public function __construct(string $first, string $last) {
    $this->first= $first;
    $this->last= $last;
  }
}

$u= new User('Timm', 'Friebe');
$u->fullName; // "Timm Friebe"

Implementation

The above results in the following being emitted:

class User {
  private string $full;
  private function __set_first($value) { $this->__virtual['first']= $value; unset($this->full); }
  private function __set_last($value) { $this->__virtual['last']= $value; unset($this->full); }
  public function __get_fullName() { return $this->full??= $this->first.' '.$this->last; }

  public function __construct(string $first, string $last) {
    $this->first= $first;
    $this->last= $last;
  }

  private $__virtual= ['first' => null, 'last' => null, 'fullName' =>  null];

  public function __set($name, $value) {
    switch ($name) {
       case 'first': /* Check omitted for brevity */ $this->__set_first($value); break;
       case 'last': /* Check omitted for brevity */ $this->__set_last($value); break; 
       default: throw new \Error('Unknown property '.$name);
    }
  }

  public function __get($name) {
    switch ($name) {
       case 'fullName': return $this->__get_fullName(); break;
       default: throw new \Error('Unknown property '.$name);
    }
  }
}
  • Although the code could be inlined within __set and __get, exceptions raised would print incorrect line numbers in their stack trace!
  • The check for private and protected properties uses debug_backtrace() to verify the calling scope.

Shortcomings

  • Currently, property modifiers are unchecked, the __set and __get functions effectively make them public, we would need to use debug_backtrace() for this! addressed in ed5003f
  • The syntax for invoking parent selectors parent::$x::get() is not implemented yet to be rewritten to parent::__get_x() as the former is ambiguous, see RFC addressed in f21a615, see comment below
  • We always create a backing property as seen in the above case for fullName, where it wouldn't be necessary.
  • Overwriting properties with properties with hooks will not work correctly due to the nature of this implementation with __set / __get, see Implement property hooks via virtual properties #166 (comment)

See also

@thekid
Copy link
Member Author

thekid commented May 18, 2023

On the ambiguity of parent::$propertyName::get()

In current PHP, this means the following:

On the parent class' static property $x, invoke a static method named get

Example

abstract class OS {
  public function exec($command, $arguments= []) { ... } }

class Command {
  public static $OS;

  static fucntion __static() {
    self::$OS= ...
  }
}

class Test extends Command {
  public function run($arguments= []) {
    return parent::$OS::exec('xp', ['test', ...$arguments]);
  }
}

Inside Test, however, self::$OS would suffice, so after carefully thinking about it I agree with the statement in the RFC: As the above code is very rare in the wild and rather contrived, and easily worked around, we feel this edge case is acceptable.

Possible alternative: parent::$propertyName (without ::[set|get]())

Kind This member Parent member
Constant self::CONSTANT parent::CONSTANT
Static method self::method() parent::method()
Static property self::$property parent::$property
Instance method $this->method() parent::method()
Constructor never invoked directly parent::__construct()
Instance property $this->property not possible or useful

The most consistent way would be to use parent::$property here. Determining whether to invoke get or set could be done by looking for assignment expression, and only invoking set in those situations.

Possible alternative: $this->propertyName

Inside hooks, we should use $field to reference the property rather than $this->propertyName, which the RFC states is "supported by discouraged". If we drop this translation, we could use it as follows:

class Base {
  public $test { get => 'Test'; }
}

class Child extends Base {
  public $test { get => $this->test.'!'; }
}

$test= (new Child())->test; // "Test!"

See https://gist.github.com/thekid/d83bac22f30e3d3e41425bb703ab1d0b

Possible alternative: parent->propertyName

Same as above, a bit more specific and in line with #164, but would be ambiguous with accessing the property propertyName of a constant named parent:

define('parent', new class() { public $propertyName= 'Test'; }); 

$name= parent->propertyName; // "Test"

This would be inconsistent with parent::<method>() syntax to invoke parent methods and the parent constructor.

Discussion in the original "Property accessors" RFC

An open question is how parent accessors may be invoked. A possible syntax is to use parent::$prop for the parent getter and parent::$prop = $value for the parent setter. As the “static-ness” of properties cannot change, this reference should be unambiguous.

This syntax can't extend to other accessor types though, so those would either have no way to invoke the parent accessor, or invoke it implicitly. For possible guard accessors, it would make sense to automatically chain the accessors (though the order is not entirely obvious). For lazy accessors this wouldn't be possible though.

Other syntax choices like parent::$prop::get() are somewhat ambiguous, for example this syntax looks like a static property access followed by a static method call.

In any case, adding support for parent accessors will be technically non-trivial, because we need to perform a modified-scope property access through a separate syntax.

Source: https://wiki.php.net/rfc/property_accessors

return new Block($nodes);
}

array_unshift($nodes, new Code($check));
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be written as return new Block([new Code($check), ...$nodes]); as of PHP 7.4.0, see https://wiki.php.net/rfc/spread_operator_for_array and xp-framework/rfc#343

@thekid
Copy link
Member Author

thekid commented Feb 24, 2024

After much on-again/off-again work, Ilija and I are back with a more polished property access hooks/interface properties RFC. It’s 99% unchanged from last summer; the PR is now essentially complete and more robust, and we were able to squish the last remaining edge cases.

Baring any major changes, we plan to bring this to a vote in mid-March.

https://externals.io/message/122445

See https://externals.io/message/122445#122478:
> However, since it seems no one likes $field, we have removed it from the RFC
@thekid
Copy link
Member Author

thekid commented Mar 10, 2024

More changes, see https://externals.io/message/122445#122583, especially:

Shorthands

The $foo => expression shorthand has been removed. The legal shorthands are now:

public string $foo {
  get => /* evaluates to a value; */
  set => /* assigns this value; */
}

The set shorthand (with =>) now means "write this value instead", see https://wiki.php.net/rfc/property-hooks#short-set

Type and name

On a set hook, the user may specify both a type and name, or neither. (That is, set {} or set (Foo $newFoo). If not specified, it defaults to the type of the property and $value, as before.

Parent

Clarified that the parent::$foo::get() syntax works on a parent property regardless of whether it has hooks


The rest of the changes seem to not have any effect on this implementation

@thekid
Copy link
Member Author

thekid commented Mar 16, 2024

Clarified that the parent::$foo::get() syntax works on a parent property regardless of whether it has hooks

This cannot work in this implementation due to us relying on __set and __get, as in the following, the virtual property would never be triggered:

class B {
    public mixed $x;
}
class C extends B {
    public mixed $x {
        set {
            $f = parent::$x::set(...);
            $f($value);
        }
    }
}

$c = new C();
$c->x = 0;

@thekid
Copy link
Member Author

thekid commented Mar 17, 2024

Status: Waiting for PHP RFC to successfully be voted on

@thekid
Copy link
Member Author

thekid commented Apr 17, 2024

The vote for the Property Hooks RFC is now open, see https://externals.io/message/123139

}
}

// TODO move this to lang.ast.emit.PHP once https://github.com/php/php-src/pull/13455 is merged
Copy link
Member Author

Choose a reason for hiding this comment

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

@thekid
Copy link
Member Author

thekid commented May 18, 2024

✅ Now works with both current PHP 8.4 and PHP 8.4 with php/php-src#13455 in place

@thekid thekid removed the question label Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

1 participant