Skip to content

Disallow assigning reference to unset readonly property #8188

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Disallow assigning reference to unset readonly property #8188

wants to merge 1 commit into from

Conversation

iluuu1994
Copy link
Member

Closes GH-7942

@iluuu1994
Copy link
Member Author

The commit message is a bit confusing, I'll rephrase to something like "Can't set unset readonly property to a reference"

@iluuu1994 iluuu1994 changed the title Disallow getting reference of unset readonly property Disallow assigning reference to unset readonly property Mar 11, 2022
@iluuu1994 iluuu1994 requested a review from nikic March 11, 2022 14:15
@iluuu1994
Copy link
Member Author

@nikic Are you ok with this change?

nikic
nikic reviewed Mar 20, 2022
View reviewed changes
staabm
staabm reviewed Mar 23, 2022
View reviewed changes
nikic
nikic reviewed Mar 27, 2022
View reviewed changes
@codecov-commenter
Copy link

Codecov Report

Merging #8188 (2fc1ddc) into PHP-8.1 (62a1c06) will increase coverage by 0.09%.
The diff coverage is 77.68%.

❗ Current head 2fc1ddc differs from pull request most recent head ce95a68. Consider uploading reports for the commit ce95a68 to get more accurate results

@@             Coverage Diff             @@
##           PHP-8.1    #8188      +/-   ##
===========================================
+ Coverage    67.34%   67.43%   +0.09%     
===========================================
  Files          800      806       +6     
  Lines       301568   305901    +4333     
===========================================
+ Hits        203084   206278    +3194     
- Misses       98484    99623    +1139
Impacted Files Coverage Δ
Zend/Optimizer/compact_literals.c 97.43% <ø> (ø)
Zend/Optimizer/dce.c 95.28% <ø> (ø)
Zend/Optimizer/nop_removal.c 0.00% <ø> (ø)
Zend/Optimizer/scdf.c 96.52% <ø> (+0.86%) ⬆️
Zend/Optimizer/scdf.h 92.85% <ø> (ø)
Zend/Optimizer/zend_ssa.h 81.66% <ø> (ø)
Zend/zend_ast.h 88.00% <ø> (ø)
Zend/zend_attributes.h 100.00% <ø> (ø)
Zend/zend_attributes_arginfo.h 100.00% <ø> (ø)
Zend/zend_compile.h 100.00% <ø> (ø)
... and 250 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ramsey ramsey modified the milestone: PHP 8.2 May 25, 2022
@ramsey
Copy link
Member

ramsey commented May 25, 2022

Is this waiting on anything? Can we merge it to PHP-8.1?

@iluuu1994
Copy link
Member Author

@ramsey I still haven't addressed Nikitas comments. Hopefully I can do that in the coming days.

@iluuu1994 iluuu1994 force-pushed the gh-7942 branch 2 times, most recently from 8fbc8e7 to 6e73012 Compare June 17, 2022 13:40
@iluuu1994 iluuu1994 mentioned this pull request Jun 24, 2022
Closed
@iluuu1994
Copy link
Member Author

iluuu1994 commented Jun 25, 2022 •
edited
Loading

Sorry for the long delay. @nikic Does this make sense to you now? There's an existing JIT bug (GH-8863) uncovered by the new tests which is why the build is still failing. I'll wait for Dmitry to fix those.

Girgias
Girgias reviewed Jun 29, 2022
View reviewed changes
Zend/tests/readonly_props/variation.phpt
Comment on lines +1 to +3
--TEST--
Readonly variations
--FILE--
Copy link
Member

Choose a reason for hiding this comment

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

Add a string concatenation variation maybe? And maybe ++, -- as IIRC those are different opcodes

Copy link
Member Author

Choose a reason for hiding this comment

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

+= and .= both result in ASSIGN_OP. ++ might be worth checking. I'll adjust the test.

arnaud-lb
arnaud-lb approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

LGTM

@iluuu1994 iluuu1994 closed this in 1105737 Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm left review comments

@nikic nikic nikic left review comments

@Girgias Girgias Girgias left review comments

@arnaud-lb arnaud-lb arnaud-lb approved these changes

Assignees
No one assigned
Labels
Category: Engine Waiting on Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@iluuu1994 @codecov-commenter @ramsey @staabm @nikic @arnaud-lb @Girgias