Skip to content

Added default as an expression in argument lists #15437

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 15 commits into
base: master
Choose a base branch
from

Conversation

Bilge
Copy link

@Bilge Bilge commented Aug 16, 2024

This PR adds default as an expression in argument lists; it is not valid in any context other than argument lists. At runtime, default will be substituted with the default value as declared at the call site of the function so called. For full details, see the default expression RFC.

TODO

  • Decide which error/exception types to throw at both compile and runtime.

Limitations

The engine cannot retrieve the default value of a trampoline call.

Special thanks

  • Thanks to @iluuu1994 for hand-holding me through the implementation.
  • Thanks to @bwoebi for the Bison patch.
  • Thanks to all the R11 folk who put up with me.

GNU Bison showing you which rules are in conflict

  • Thanks to @gooh for the above.

@Bilge Bilge force-pushed the default-expr branch 2 times, most recently from 098ef37 to 10cfffe Compare August 16, 2024 08:31
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.

Not commenting on things listed in the todos.

Patched match expression to work with the new grammar.
Changed ZEND_FETCH_DEFAULT_ARG VM handler to use reflection_parameter_ptr.
Exposed get_parameter_default from reflection API.
Added exception handling for passing `default` to required parameters.
Fixed default_value leak in ZEND_FETCH_DEFAULT_ARG handler.
Added compile-time exclusion for default in non-argument contexts.
@Bilge Bilge force-pushed the default-expr branch 4 times, most recently from fd34f8d to 29855cc Compare August 21, 2024 07:39
if (op_array->opcodes[i].opcode == ZEND_FETCH_DEFAULT_ARG) {
return;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned off-list, this should check only for opcodes within the current call (between fcall and opline), not the entire function. It would be nice to also skip nested functions, but that will require listing all init/do opcodes.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the note. I am still trying to figure out how to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Store fcall in a local var, loop, and bump until reaching opline.

Copy link
Member

Choose a reason for hiding this comment

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

There is code for that in PHP already somewhere, and now also in Xdebug: xdebug/xdebug@551b203#diff-846d4897469701b1d805f3574922dc47b632fd189087a2589b2e01e848585a8bR378 — that should probably become a PHP API function.

Copy link
Author

Choose a reason for hiding this comment

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

I'll be honest, I still have no idea what to do here, and no idea how the Xdebug code relates to this. Anyway, I'll deal with this if the feature gets accepted.

Copy link
Member

Choose a reason for hiding this comment

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

for (zend_op *op = fcall + 1; op != opline; op++) {
	// Don't inline calls with explicit default arguments.
	if (op->opcode == ZEND_FETCH_DEFAULT_ARG) {
		return;
	}
}

Copy link
Author

@Bilge Bilge Aug 28, 2024

Choose a reason for hiding this comment

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

Thanks. Clearly I need to understand more about what oparrays, oplines and opcodes really are, and how they relate to each other. I tend to imagine that an oparray contains oplines which contain opcodes, but I still don't really know why oparray is even a thing, and when you would bundle oplines together.

Added tests for anonymous classes, callables and closures.
@Bilge Bilge marked this pull request as ready for review August 27, 2024 08:38
@Bilge Bilge requested a review from dstogov as a code owner August 27, 2024 08:38
Added test for default with variadic arguments.
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.

5 participants