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

[WIP] Implement nullsafe ?-> operator #5619

Closed
wants to merge 25 commits into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented May 24, 2020

Zend/zend_compile.c Outdated Show resolved Hide resolved
@HallofFamer
Copy link

HallofFamer commented May 26, 2020

Would be nice to have null safe operator in PHP 8, is there a possibility that it will make the deadline before feature freeze? Though it wont hurt to have it in PHP 8.1 if it wont make the cut.

@iluuu1994
Copy link
Member Author

@HallofFamer

Would be nice to have null safe operator in PHP 8, is there a possibility that it will make the deadline before feature freeze?

I'll try 🙂 There are actually quite a few details to this that I didn't anticipate. We need to make sure to catch all the edge cases.

Zend/zend_vm_def.h Outdated Show resolved Hide resolved
@HallofFamer
Copy link

Its looking great so far, hope the RFC will be up and ready for voting before the feature freeze date of PHP 8. Best regards.

Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/011.phpt Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/001.phpt Outdated Show resolved Hide resolved
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

TBH the short circuiting implementation left me completely confused.

Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
@iluuu1994
Copy link
Member Author

iluuu1994 commented Jun 3, 2020

@nikic I haven't cleaned up all of the code yet. The naming is confusing (e.g. scope -> chain, short_circuiting -> ???). I'll try to improve that. The main idea is: Compiling an expression will check if a new short-circuiting chain needs to be created. This is the case if:

  1. There is no active short circuiting chain AND
  2. The AST-node is configured to be part of short circuiting chains

There are some annoyances like, not all expressions are compiled using zend_compile_expr (like zend_compile_var) and the special functions compile their arguments manually (which requires terminating the chain for the arguments manually in each one).

@iluuu1994 iluuu1994 force-pushed the nullsafe-operator branch 2 times, most recently from f5785fd to f56c6ad Compare June 3, 2020 18:29
@iluuu1994 iluuu1994 force-pushed the nullsafe-operator branch 2 times, most recently from 942da68 to 941785b Compare June 10, 2020 21:56
@iluuu1994
Copy link
Member Author

@nikic Is the implementation (especially Zend/zend_compile.c) less confusing to you now?

@mvorisek
Copy link
Contributor

mvorisek commented Jun 18, 2020

@iluuu1994 What is the advantage/use of nullsafe operator in write context?

@iluuu1994
Copy link
Member Author

@mvorisek From the RFC:

$foo = null;
$foo?->bar = 'bar';
var_dump($foo);
 
// Without short circuiting:
// Fatal error: Can't use nullsafe result value in write context
 
// With short circuiting:
// NULL

Without short circuiting the assignment to a nullsafe property would be illegal because it produces an r-value (a value that cannot be assigned to). With short circuiting if a nullsafe operation on the left hand side of the assignment fails the assignment is simply skipped.

@mvorisek
Copy link
Contributor

@iluuu1994 I see, we check if object is not null, not the property (which will not make any sense)...

@HallofFamer
Copy link

PHP 8 already enters alpha, seems that feature freeze will be soon. Do you think your RFC is ready for voting phase before the feature freeze? This one I mean:

https://wiki.php.net/rfc/nullsafe_operator

Copy link
Member Author

@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.

Only (known) remaining issue: Using ?-> in an argument should emit a SEND_VAL instead of SEND_REF because otherwise the reference check is omitted and no error is printed when a null value is passed.

Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/005.phpt Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/004.phpt Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/004.phpt Outdated Show resolved Hide resolved
Zend/tests/nullsafe_method_call/014.phpt Outdated Show resolved Hide resolved
Zend/zend_language_parser.y Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
ext/opcache/Optimizer/zend_inference.c Outdated Show resolved Hide resolved
ext/opcache/Optimizer/zend_ssa.c Outdated Show resolved Hide resolved
Zend/zend_compile.h Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jul 1, 2020

Only (known) remaining issue: Using ?-> in an argument should emit a SEND_VAL instead of SEND_REF because otherwise the reference check is omitted and no error is printed when a null value is passed.

Not sure I agree here, I think SEND_REF is right. This is similar to the write case, where $null?->foo = 123 is silently ignored, similarly here byRef($null?->foo) would silently ignore the fact that the property cannot be written. Otherwise it wouldn't be possible to use the nullsafe operator for by-ref arguments in any meaningful way (i.e. they would work for non-null values, but always fail for null ones, right?)

@iluuu1994
Copy link
Member Author

Otherwise it wouldn't be possible to use the nullsafe operator for by-ref arguments in any meaningful way (i.e. they would work for non-null values, but always fail for null ones, right?)

Right. I mentioned that but wasn't sure if that was preferable. It's kind of different in that it won't just skip the call but pass null so any method doing type checks will still fail (e.g. array_push($mightBeNull?->list, $value)). Thus I'm not entirely sure how useful that would be.

It would be simpler to implement though (as it doesn't require special handling for ?->).

@nikic
Copy link
Member

nikic commented Jul 1, 2020

Would it be possible to implement the short-circuiting by having a single stack of jmp_null opnums and then remembering (via return value) the current stack depth on entry into an expression, and popping the stack down to the remembered depth on exit?

Zend/zend_language_parser.y Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jul 22, 2020

I think I'm happy with this now. @iluuu1994 Apart from the JIT support (which imho should be added separately), anything more you want to do here?

@iluuu1994
Copy link
Member Author

@nikic A few tests are missing (see unresolved discussions) but apart from that no, I think this is ready 🙂

@php-pulls php-pulls closed this in 9bf1198 Jul 24, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 16, 2020
PHP 8 introduces a new object chaining operator `?->` which short-circuits moving to the next expression if the left-hand side evaluates to `null`.

This operator can not be used in write-context, but that is not the concern of this sniff.

Refs:
* https://wiki.php.net/rfc/nullsafe_operator
* php/php-src#5619
* php/php-src@9bf1198

Includes unit test.
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 18, 2020
PHP 8 introduces a new object chaining operator `?->` which short-circuits moving to the next expression if the left-hand side evaluates to `null`.

This operator can not be used in write-context, but that is not the concern of this sniff.

Refs:
* https://wiki.php.net/rfc/nullsafe_operator
* php/php-src#5619
* php/php-src@9bf1198

Includes unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants