Skip to content

Fix GH-19044: Protected properties are not scoped according to their prototype #19046

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

Open
wants to merge 2 commits into
base: PHP-8.4
Choose a base branch
from

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Jul 5, 2025

This should properly cover all cases, especially also around set hooks which get added later on with protected(set) visibility.

Fixes #19044.

@bwoebi bwoebi requested a review from dstogov as a code owner July 5, 2025 23:27
@bwoebi bwoebi changed the base branch from master to PHP-8.4 July 5, 2025 23:28
@bwoebi bwoebi requested review from iluuu1994 and removed request for dstogov July 5, 2025 23:28
@bwoebi bwoebi force-pushed the fix-19044 branch 5 times, most recently from e8099c0 to e1008f8 Compare July 6, 2025 00:15
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Thanks for having a look at this.

static zend_never_inline int is_asymmetric_set_protected_property_compatible_scope(const zend_property_info *info, const zend_class_entry *scope) /* {{{ */
{
zend_class_entry *ce;
if (!(info->prototype->flags & ZEND_ACC_PROTECTED_SET) && info->hooks && info->hooks[ZEND_PROPERTY_HOOK_SET]) {
Copy link
Member

Choose a reason for hiding this comment

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

The implementation does rely on some semantic details that could be spelled out better, as it looks a bit confusing at first-sight. The way I understand it:

  • !(info->prototype->flags & ZEND_ACC_PROTECTED_SET) implies that info->prototype is abstract, and may or may not declare a set operation.
  • We want to find the first parent that does declare a set operation. To do so, we use set_hook->prototype.

Maybe add a comment for this.

I did find this example that looks wrong:

<?php

abstract class GP {
    abstract mixed $foo { get; }
}

class C1 extends GP {
    public mixed $foo = 1;
}

class C2 extends GP {
    public mixed $foo;

    static function foo($c) { return $c->foo += 1; }
}

var_dump(C2::foo(new C1));

// int(2)

Here, we're not using hooks at all. Following the semantics from this PR, this should fail because GP::$foo::set is not declared. This breaks because the fallback branch ce = info->prototype->ce; picks a parent that's too high up in the inheritance chain.

One way around this would be to introduce a separate setPrototype, but this would be ABI-breaking. The alternative is to iterate the inheritance chain. In that case, I'd probably prefer being inaccurate and allowing the set in all cases, even if the parent doesn't declare it.

Copy link
Member

Choose a reason for hiding this comment

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

Another similar case:

<?php

abstract class GP {
    public abstract mixed $foo { get; }
}

class P extends GP {
    public protected(set) mixed $foo { get => $this->foo; }
}

class C1 extends P {
    public protected(set) mixed $foo = 1;
}

class C2 extends P {
    public protected(set) mixed $foo;

    static function foo($c) { return $c->foo += 1; }
}

var_dump(C2::foo(new C1));

// int(2)

C1 does have hooks (inherited), but the set is implicit because it is backed. Hence, we're dodging the branch and selecting the wrong parent.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and yet another.

<?php

class P {
    public mixed $foo { get => 42; }
}

class C1 extends P {
    public protected(set) mixed $foo = 1;
}

class C2 extends P {
    public protected(set) mixed $foo;

    static function foo($c) { return $c->foo += 1; }
}

var_dump(C2::foo(new C1));

// int(43)

The parent may be lacking protected(set) even if it's not abstract, if it's virtual and doesn't have set at all.

Copy link
Member Author

@bwoebi bwoebi Jul 6, 2025

Choose a reason for hiding this comment

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

(First post:) I did find this example that looks wrong:

Why does that look wrong? The setter is public. It being public trumps any protected behaviour.

Second post

Where's the problem? Both inherit from P, which is protected(set). So that's what it shall look for. (The parent guarantees the visbility.)

Third post

Yeah, that one's wrong, C1 introduces the protected(set) in the first place. Here it must break.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the third post, I think the only option is to iteratively go through the parents of the class.

Copy link
Member Author

@bwoebi bwoebi Jul 6, 2025

Choose a reason for hiding this comment

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

Put a fallback for the third case into the code - the happy paths are still fast. And the unhappy path is not that expensive, after all you anyway have an iteration through the hierarchy for the instanceof later.

public protected(set) mixed $foo { get => 1; set {} }
}

class GrandC1 extends C1 {
Copy link
Member

Choose a reason for hiding this comment

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

This naming is quite confusing. It's the child of C1, grand implies grand-child or grand-parent.

Copy link
Member Author

Choose a reason for hiding this comment

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

C1 = Child 1, hence GrandC1 :-D (Just like GrandP in some other test)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how exactly to rename them, but feel free to just push to my branch for that.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 6, 2025

@iluuu1994 May you please re-review now? :-)

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