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 updating SSA object type for *_ASSIGN_OP #10458

Merged
merged 1 commit into from Feb 14, 2023

Conversation

nielsdos
Copy link
Member

The code fetched the class entry into ce for objects and static properties. However, when the actual update needs to take place (when result_def exists), the class entry in ce was reset to NULL. So the SSA object type update never happened. Fetch the class entry in the result_def>=0 case instead after the reset of ce to NULL.

The code fetched the class entry into ce for objects and static
properties. However, when the actual update needs to take place (when
result_def exists), the class entry in ce was reset to NULL. So the SSA
object type update never happened. Fetch the class entry in the
result_def>=0 case instead after the reset of ce to NULL.
@nielsdos
Copy link
Member Author

nielsdos commented Feb 2, 2023

Rebased because the upstream code was updated in the meantime.

@nielsdos
Copy link
Member Author

Maybe it's wise to tag @dstogov here as well because he recently changed this code.

@dstogov
Copy link
Member

dstogov commented Feb 13, 2023

I didn't recently change anything zend_fetch_prop_type(ce) related. That logic was introduced by the original typed-property commit e219ec1

I'll try to check this now...

@dstogov
Copy link
Member

dstogov commented Feb 13, 2023

I think the fix is right.
It would be great to get a confirmation from @nikic , because I don't see his original intention.

@nikic
Copy link
Member

nikic commented Feb 14, 2023

Looks fine to me. We don't need ce in the initial calls because binary_op_result_type doesn't use it (though maybe it could to check for operator overloading).

This probably doesn't really matter in practice because you usually can't meaningfull apply operators to objects.

@dstogov dstogov merged commit d94ddbe into php:PHP-8.1 Feb 14, 2023
dstogov added a commit that referenced this pull request Feb 14, 2023
* PHP-8.1:
  Fix updating SSA object type for *_ASSIGN_OP (#10458)
dstogov added a commit that referenced this pull request Feb 14, 2023
* PHP-8.2:
  Fix updating SSA object type for *_ASSIGN_OP (#10458)
@TimWolla TimWolla mentioned this pull request Feb 14, 2023
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.

None yet

4 participants