Skip to content

Fix property hook asymmetric set visibility (#6898) #6980

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.

Sign up for GitHub

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

Merged
PurHur merged 1 commit into from
Jun 6, 2026
Merged

Fix property hook asymmetric set visibility (#6898) #6980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 108 additions & 25 deletions lib/SourcePreprocessor/PropertyHooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,16 @@ private function processClassBody(
if (!str_ends_with($propDecl, ';')) {
$propDecl .= ';';
}
[$methods, $usesBacking, $trailing, $asymmetricSetVis] = $this->lowerHooks($hookSource, $prop, $lcClass, $isStatic);
if (null !== $asymmetricSetVis) {
$marker = '/*phpc-asymmetric-set:'.$asymmetricSetVis.'*/ ';
if (preg_match('/^(\s*)/', $declPrefix, $indentM)) {
$declPrefix = $indentM[1].$marker.ltrim($declPrefix);
} else {
$declPrefix = $marker.$declPrefix;
}
}
$out .= $declPrefix.$propDecl;
[$methods, $usesBacking, $trailing] = $this->lowerHooks($hookSource, $prop, $lcClass, $isStatic);
$trailing = trim($trailing);
if ('' !== $trailing) {
$out .= "\n ".$trailing;
Expand Down Expand Up @@ -228,12 +236,13 @@ private function processClassBody(
}

/**
* @return array{0: list<string>, 1: bool, 2: string} method source chunks, whether any hook touches backing storage, trailing hook-body declarations
* @return array{0: list<string>, 1: bool, 2: string, 3: ?string} method source chunks, backing use, trailing decls, asymmetric set visibility
*/
private function lowerHooks(string $hookSource, string $prop, string $lcClass, bool $isStatic = false): array
{
$methods = [];
$usesBacking = false;
$asymmetricSetVisibility = null;
$rest = trim($hookSource);
while ('' !== $rest) {
$rest = ltrim($rest);
Expand All @@ -247,6 +256,12 @@ private function lowerHooks(string $hookSource, string $prop, string $lcClass, b
$rest = preg_replace('/^set\s*;/', '', $rest, 1) ?? $rest;
continue;
}
if (preg_match('/^set\s*\(\s*(public|protected|private)\s*\)\s*;/s', $rest, $asymM)) {
$asymmetricSetVisibility = strtolower($asymM[1]);
$this->registerRequiredHook($lcClass, $prop, 'requiresSet');
$rest = preg_replace('/^set\s*\(\s*(public|protected|private)\s*\)\s*;/i', '', $rest, 1) ?? $rest;
continue;
}
if (preg_match('/^unset\s*;/s', $rest)) {
$this->registerRequiredHook($lcClass, $prop, 'requiresUnset');
$rest = preg_replace('/^unset\s*;/', '', $rest, 1) ?? $rest;
Expand Down Expand Up @@ -275,19 +290,45 @@ private function lowerHooks(string $hookSource, string $prop, string $lcClass, b
if (preg_match('/^set\s*=>\s*/s', $rest)) {
$rest = preg_replace('/^set\s*=>\s*/', '', $rest, 1) ?? $rest;
[$expr, $rest] = $this->takeUntilSemicolon($rest);
$expr = rtrim($expr);
if ($this->setArrowExprUsesStatementForm($expr, $isStatic)) {
$usesBacking = $usesBacking || $this->hookTouchesBacking($expr, $prop, $isStatic);
$this->registerHookBacking($lcClass, $prop, 'set', $expr, $isStatic);
$body = '{ '.$expr.'; }';
} else {
$backing = $isStatic ? 'self::$'.$prop : '$this->'.$prop;
$usesBacking = true;
$body = '{ '.$backing.' = ('.$expr.'); }';
$methods = array_merge(
$methods,
$this->lowerSetArrowHook($lcClass, $prop, $isStatic, rtrim($expr), $usesBacking)
);
continue;
}
if (preg_match('/^set\s*\(\s*(public|protected|private)\s*\)\s*=>\s*/s', $rest, $asymM)) {
$asymmetricSetVisibility = strtolower($asymM[1]);
$rest = preg_replace('/^set\s*\(\s*(public|protected|private)\s*\)\s*=>\s*/i', '', $rest, 1) ?? $rest;
[$expr, $rest] = $this->takeUntilSemicolon($rest);
$methods = array_merge(
$methods,
$this->lowerSetArrowHook($lcClass, $prop, $isStatic, rtrim($expr), $usesBacking)
);
continue;
}
if (preg_match('/^set\s*\(\s*(public|protected|private)\s*\)\s*\(/s', $rest, $asymM)) {
$asymmetricSetVisibility = strtolower($asymM[1]);
$rest = preg_replace('/^set\s*\(\s*(public|protected|private)\s*\)\s*/i', '', $rest, 1) ?? $rest;
if (!preg_match('/^\(([^)]*)\)\s*\{/s', $rest, $pm)) {
break;
}
$method = self::SET_METHOD_PREFIX.$prop;
$methods[] = $this->hookMethodDecl($isStatic, $method, '$value', $body);
$this->registerHook($lcClass, $prop, 'set', $method, $isStatic);
$params = trim($pm[1]);
$rest = substr($rest, strlen($pm[0]) - 1);
[$body, $rest] = $this->takeBraceBody($rest);
$methods = array_merge(
$methods,
$this->lowerSetBlockHook($lcClass, $prop, $isStatic, $params, $body, $usesBacking)
);
continue;
}
if (preg_match('/^set\s*\(\s*(public|protected|private)\s*\)\s*\{/s', $rest, $asymM)) {
$asymmetricSetVisibility = strtolower($asymM[1]);
$rest = preg_replace('/^set\s*\(\s*(public|protected|private)\s*\)\s*/i', '', $rest, 1) ?? $rest;
[$body, $rest] = $this->takeBraceBody($rest);
$methods = array_merge(
$methods,
$this->lowerSetBlockHook($lcClass, $prop, $isStatic, '$value', $body, $usesBacking)
);
continue;
}
if (preg_match('/^set\s*\(/s', $rest)) {
Expand All @@ -298,21 +339,19 @@ private function lowerHooks(string $hookSource, string $prop, string $lcClass, b
$params = trim($pm[1]);
$rest = substr($rest, strlen($pm[0]) - 1);
[$body, $rest] = $this->takeBraceBody($rest);
$usesBacking = $usesBacking || $this->hookTouchesBacking($body, $prop, $isStatic);
$this->registerHookBackingFromBody($lcClass, $prop, 'set', $body, $isStatic);
$method = self::SET_METHOD_PREFIX.$prop;
$methods[] = $this->hookMethodDecl($isStatic, $method, $params, $body);
$this->registerHook($lcClass, $prop, 'set', $method, $isStatic);
$methods = array_merge(
$methods,
$this->lowerSetBlockHook($lcClass, $prop, $isStatic, $params, $body, $usesBacking)
);
continue;
}
if (preg_match('/^set\s*\{/s', $rest)) {
$rest = preg_replace('/^set\s*/', '', $rest, 1) ?? $rest;
[$body, $rest] = $this->takeBraceBody($rest);
$usesBacking = $usesBacking || $this->hookTouchesBacking($body, $prop, $isStatic);
$this->registerHookBackingFromBody($lcClass, $prop, 'set', $body, $isStatic);
$method = self::SET_METHOD_PREFIX.$prop;
$methods[] = $this->hookMethodDecl($isStatic, $method, '$value', $body);
$this->registerHook($lcClass, $prop, 'set', $method, $isStatic);
$methods = array_merge(
$methods,
$this->lowerSetBlockHook($lcClass, $prop, $isStatic, '$value', $body, $usesBacking)
);
continue;
}
if (preg_match('/^unset\s*=>\s*/s', $rest)) {
Expand All @@ -338,7 +377,51 @@ private function lowerHooks(string $hookSource, string $prop, string $lcClass, b
break;
}

return [$methods, $usesBacking, trim($rest)];
return [$methods, $usesBacking, trim($rest), $asymmetricSetVisibility];
}

/**
* @return list<string>
*/
private function lowerSetArrowHook(
string $lcClass,
string $prop,
bool $isStatic,
string $expr,
bool &$usesBacking
): array {
if ($this->setArrowExprUsesStatementForm($expr, $isStatic)) {
$usesBacking = $usesBacking || $this->hookTouchesBacking($expr, $prop, $isStatic);
$this->registerHookBacking($lcClass, $prop, 'set', $expr, $isStatic);
$body = '{ '.$expr.'; }';
} else {
$backing = $isStatic ? 'self::$'.$prop : '$this->'.$prop;
$usesBacking = true;
$body = '{ '.$backing.' = ('.$expr.'); }';
}
$method = self::SET_METHOD_PREFIX.$prop;
$this->registerHook($lcClass, $prop, 'set', $method, $isStatic);

return [$this->hookMethodDecl($isStatic, $method, '$value', $body)];
}

/**
* @return list<string>
*/
private function lowerSetBlockHook(
string $lcClass,
string $prop,
bool $isStatic,
string $params,
string $body,
bool &$usesBacking
): array {
$usesBacking = $usesBacking || $this->hookTouchesBacking($body, $prop, $isStatic);
$this->registerHookBackingFromBody($lcClass, $prop, 'set', $body, $isStatic);
$method = self::SET_METHOD_PREFIX.$prop;
$this->registerHook($lcClass, $prop, 'set', $method, $isStatic);

return [$this->hookMethodDecl($isStatic, $method, $params, $body)];
}

private function hookTouchesBacking(string $source, string $prop, bool $isStatic): bool
Expand Down
32 changes: 32 additions & 0 deletions lib/VM.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,6 +1587,11 @@ private function runFramesInner(): int
$frame = $catchFrame;
goto restart;
}
$catchFrame = $this->enforceAsymmetricPropertyWrite($arg2, $frame);
if (null !== $catchFrame) {
$frame = $catchFrame;
goto restart;
}
if ($this->dispatchPropertySetHookAssign($arg2, $arg3, $frame)) {
$arg1->copyFrom($arg3);
break;
Expand Down Expand Up @@ -5227,6 +5232,10 @@ private function executeHookedPropertyInPlaceCompound(Frame $frame, OpCode $op,
} catch (\Error $e) {
return $this->dispatchVmError($e->getMessage(), $frame);
}
$catchFrame = $this->enforceAsymmetricPropertyWrite($write, $frame);
if (null !== $catchFrame) {
return $catchFrame;
}
if (!$this->dispatchPropertySetHookAssign($write, $working, $frame)) {
$catchFrame = $this->enforceVirtualPropertyHookWrite($write, $frame);
if (null !== $catchFrame) {
Expand Down Expand Up @@ -5255,6 +5264,10 @@ private function executeHookedPropertyIncDec(
} else {
$working->applyDecrement();
}
$catchFrame = $this->enforceAsymmetricPropertyWrite($write, $frame);
if (null !== $catchFrame) {
return $catchFrame;
}
if (!$this->dispatchPropertySetHookAssign($write, $working, $frame)) {
$catchFrame = $this->enforceVirtualPropertyHookWrite($write, $frame);
if (null !== $catchFrame) {
Expand All @@ -5273,6 +5286,10 @@ private function executeHookedPropertyIncDec(
} else {
$working->applyDecrement();
}
$catchFrame = $this->enforceAsymmetricPropertyWrite($write, $frame);
if (null !== $catchFrame) {
return $catchFrame;
}
if (!$this->dispatchPropertySetHookAssign($write, $working, $frame)) {
$catchFrame = $this->enforceVirtualPropertyHookWrite($write, $frame);
if (null !== $catchFrame) {
Expand Down Expand Up @@ -6550,6 +6567,10 @@ public function readPropertyHookRef(Variable $writeLvalue): Variable
public function writePropertyHookRef(Variable $writeLvalue, Variable $value): void
{
$frame = $this->requireExecutingFrame();
$catchFrame = $this->enforceAsymmetricPropertyWrite($writeLvalue, $frame);
if (null !== $catchFrame) {
throw new VM\PropertyHookRefWriteSignal($catchFrame);
}
if ($this->dispatchPropertySetHookAssign($writeLvalue, $value, $frame)) {
return;
}
Expand Down Expand Up @@ -7537,6 +7558,17 @@ private function readonlyPropertyDeclaringClass(ObjectEntry $object, string $pro
return $meta->declaringClassLc !== '' ? $meta->declaringClassLc : $object->class->name;
}

/** Reject asymmetric set visibility violations (#3165, #6898); returns catch frame or null. */
private function enforceAsymmetricPropertyWrite(Variable $lvalue, Frame $frame): ?Frame
{
$msg = $this->asymmetricPropertyWriteMessage($lvalue, $frame);
if (null === $msg) {
return null;
}

return $this->dispatchVmError($msg, $frame);
}

/** Reject asymmetric set visibility violations (#3165); returns message or null. */
private function asymmetricPropertyWriteMessage(Variable $lvalue, Frame $frame): ?string
{
Expand Down
22 changes: 22 additions & 0 deletions test/compliance/cases/language/property_hook_asymmetric_set.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
Language: property hooks with asymmetric set visibility (#6898, zend_property_hooks.c)
--FILE--
<?php
class C {
public string $x {
get => 'g';
set (protected) => $value;
}
}
$c = new C();
echo $c->x, "\n";
try {
$c->x = 'n';
} catch (Error $e) {
echo get_class($e), ': ', $e->getMessage(), "\n";
}
echo $c->x, "\n";
--EXPECT--
g
Error: Cannot modify protected(set) property C::$x from global scope
g
39 changes: 39 additions & 0 deletions test/unit/SourcePreprocessor/PropertyHooksTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,43 @@ class C {
self::assertStringNotContainsString('$x {', $out);
self::assertStringContainsString('function __phpc_property_get_x', $out);
}

/** @covers issue #6898 — asymmetric set visibility on property hooks */
public function testLowersAsymmetricSetArrowHook(): void
{
$src = <<<'PHP'
<?php
class C {
public string $x {
get => 'g';
set (protected) => $value;
}
}
PHP;
[$out, $registry] = (new PropertyHooks())->process($src);
self::assertStringNotContainsString('$x {', $out);
self::assertStringContainsString('/*phpc-asymmetric-set:protected*/ public string $x;', $out);
self::assertStringContainsString('function __phpc_property_get_x', $out);
self::assertStringContainsString('function __phpc_property_set_x', $out);
self::assertStringContainsString('$this->x = ($value);', $out);
self::assertSame('__phpc_property_set_x', $registry['c']['x']['set'] ?? null);
}

public function testLowersAsymmetricSetBlockHook(): void
{
$src = <<<'PHP'
<?php
class C {
public string $x {
get => 'g';
set (private) {
$this->x = $value;
}
}
}
PHP;
[$out] = (new PropertyHooks())->process($src);
self::assertStringContainsString('/*phpc-asymmetric-set:private*/ public string $x;', $out);
self::assertStringContainsString('function __phpc_property_set_x($value)', $out);
}
}