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

Fix setRawValueWithoutLazyInitialization() / skipLazyInitialization() on initialized proxy #16344

Closed
wants to merge 1 commit into from

Conversation

arnaud-lb
Copy link
Member

skipLazyInitialization() removes the lazy flag of a property and initializes it to its default value. If the property was already initialized, it has no effect.

However, initialized proxies have all their properties marked as lazy, but skipLazyInitialization() should not initialize them.

Similarly, setRawValueWithoutLazyInitialization() should not set the value on initialized proxies.

Here I make sure to produce the effect on the real instance rather than the proxy.

NB: It's possible that the real instance is also a proxy. In this case I fetch its real instance again, and the affected object is the deepest instance.

@arnaud-lb arnaud-lb marked this pull request as ready for review October 10, 2024 16:26
@arnaud-lb arnaud-lb requested a review from iluuu1994 November 4, 2024 17:08
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.

Looks good otherwise. Sorry for the late response. I guess it's too late for 8.4 but this seems like a fringe case.


if (!(Z_PROP_FLAG_P(dst) & IS_PROP_LAZY)) {
if (!prop_was_lazy) {
Copy link
Member

Choose a reason for hiding this comment

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

prop_was_lazy is used below, and always true. Is this correct? In that case, it should be dropped from the condition.

@arnaud-lb
Copy link
Member Author

@iluuu1994 just to be sure, you meant 8.4.0, not 8.4?

@iluuu1994
Copy link
Member

@arnaud-lb Yes. Sorry for the imprecise wording.

@arnaud-lb
Copy link
Member Author

No worries! Thank you for the review

@arnaud-lb arnaud-lb closed this in c310be0 Nov 26, 2024
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