Implement return types #997

morrisonlevi · 2015-01-13T22:36:13Z

RFC is documented here: https://wiki.php.net/rfc/return_types

This differs from Dmitry's pull request #986 in a few ways:

Minor naming differences
Updated to latest master (were some merge conflicts)
Return type mismatches are now fatals instead of E_STRICTs.

HallofFamer · 2015-01-14T08:40:59Z

So I guess, the function keyword is still going to be required for declaring methods even when return type hinting is implemented? If so, it just kills the purpose of this feature. Much more typing and ugly syntax, I wont use it then. I hope, this one will be optional though.

morrisonlevi · 2015-01-14T09:04:02Z

@HallofFamer No, it won't replace the function keyword. At some point in some future RFC someone could theoretically make function optional. It's not happening with this RFC as it would certainly kill it dead in the water. Return types and removing the function keyword are orthogonal if return types are placed after the parameter list.

derickr · 2015-01-14T10:05:02Z

Zend/zend_ast.h

@@ -193,7 +193,7 @@ ZEND_API zend_ast *zend_ast_create(zend_ast_kind kind, ...);

 ZEND_API zend_ast *zend_ast_create_decl(
 	zend_ast_kind kind, uint32_t flags, uint32_t start_lineno, zend_string *doc_comment,
-	zend_string *name, zend_ast *child0, zend_ast *child1, zend_ast *child2
+	zend_string *name, zend_ast *child0, zend_ast *child1, zend_ast *child2, zend_ast *child3


Instead of changing the API/ABI here, wouldn't it make more sense to use a ** for the children? Right now, this ABI can be changed for PHP 7, but not generally in any other case than a major release.

@derickr I thought we broke the ABI on minors?

x.y.z to x.y+1.z

Bugfixes

New features

Extensions support can be ended (moved to pecl)

Backward compatibility must be kept

API compatibility must be kept (userland)

ABI and API can be broken (internals), src compatibility should be kept if possible, while breakages are allowed

https://wiki.php.net/rfc/releaseprocess#releases_cycle

This is for PHP7

On Wed, Jan 28, 2015 at 5:23 AM, Andrea Faulds [email protected]
wrote:

In Zend/zend_ast.h
#997 (comment):

@@ -193,7 +193,7 @@ ZEND_API zend_ast *zend_ast_create(zend_ast_kind kind, ...);

ZEND_API zend_ast *zend_ast_create_decl(
zend_ast_kind kind, uint32_t flags, uint32_t start_lineno, zend_string *doc_comment,

zend_string *name, zend_ast *child0, zend_ast *child1, zend_ast *child2

zend_string *name, zend_ast *child0, zend_ast *child1, zend_ast *child2, zend_ast *child3

x.y.z to x.y+1.z

Bugfixes

New features

Extensions support can be ended (moved to pecl)

Backward compatibility must be kept

API compatibility must be kept (userland)

ABI and API can be broken (internals), src compatibility should be
kept if possible, while breakages are allowed

—
Reply to this email directly or view it on GitHub
https://github.com/php/php-src/pull/997/files#r23661647.

This is also a largely internal API anyway

HallofFamer · 2015-01-15T08:09:02Z

Okay for god's sake I hope by the time PHP 7 comes out, the function keyword will be optional, especially with class methods(not necessary with orphan functions, since these really by definition are 'functions'). The function keyword does not mesh well with the return type syntax, the two combined makes syntax ugly and hard to read.

beberlei · 2015-01-16T22:32:27Z

Zend/tests/return_types/012.phpt

@@ -0,0 +1,28 @@
+--TEST--
+Method returned callable, expected callable


wrong test name, copied from 011.phpt

staabm · 2015-01-24T08:46:46Z

IMO it would be a good idea to run the phpng benchmark before and after merging this into master

//cc @dstogov

dstogov · 2015-01-24T17:58:54Z

the patch was carefully designed to not make any performance degradation.
It was tested about month ago. Nothing serious was changed after that.
Anyway, if you have time and experience - please, test it and report your
results.

On Sat, Jan 24, 2015 at 11:47 AM, Markus Staab [email protected]
wrote:

IMO it would be a good idea to run the phpng benchmark before and after
merging this into master

//cc @dstogov https://github.com/dstogov

—
Reply to this email directly or view it on GitHub
#997 (comment).

staabm · 2015-01-24T18:11:38Z

I am just started playing with docker and try to get a vm up and runiing, so i can do those benchmarks myself in the future. Thanks

staabm · 2015-01-24T22:46:42Z

@dstogov i just ran the benchmark on a freshly configured ubuntu 14 lts. It yielded 7,339,515,001 instructions for the Zend/bench.php. Does the number of instructions depend on the architecture or where might this big diff come frame - comparing to your measures within the phpng rfc?

dstogov · 2015-01-26T06:10:56Z

I suppose you run PHP master branch. may be you run DEBUG build?

On Sun, Jan 25, 2015 at 1:47 AM, Markus Staab [email protected]
wrote:

@dstogov https://github.com/dstogov i just ran the benchmark on a
freshly configured ubuntu 14 lts. It yielded ~9,267,xxx,yyy instructions
for the Zend/bench.php. Does the number of instructions depend on the
architecture or where might this big diff come frame - comparing to your
measures within the phpng rfc?

—
Reply to this email directly or view it on GitHub
#997 (comment).

RFC is documented here: https://wiki.php.net/rfc/return_types

rdlowrey · 2015-01-28T01:43:24Z

For anyone who's concerned about the failing Travis CI build that shows lots of return type test case failures ...

I just compiled release and debug builds in linux, windows and mac environments and didn't receive any unexpected test failures. Not sure why Travis borked, but everything seems fine so far as I can tell.

hikari-no-yume · 2015-01-28T01:45:31Z

@rdlowrey Travis is segfaulting, that is incredibly serious and must be looked into.

rdlowrey · 2015-01-28T01:52:11Z

Serious is serious, I'm just saying I've tried to reproduce and have been unable to do so with this patch applied to current master.

marcioAlmada · 2015-01-28T02:01:41Z

Could not reproduce too. Linux and OSx here.

hikari-no-yume · 2015-01-28T02:20:37Z

I also can't reproduce, but I'm curious now. May look over the source.

dstogov · 2015-01-28T02:22:51Z

some problems are reproducible running: make test TESTS="-m
Zend/tests/return_types"
I'm working on them

Dmitry.

On Wed, Jan 28, 2015 at 5:21 AM, Andrea Faulds [email protected]
wrote:

I also can't reproduce, but I'm curious now. May look over the source.

—
Reply to this email directly or view it on GitHub
#997 (comment).

hikari-no-yume · 2015-01-28T02:30:51Z

Zend/zend_opcode.c


 		if (op_array->fn_flags & ZEND_ACC_VARIADIC) {
 			num_args++;
 		}
-		for (i = 0; i < num_args; i++) {
+		for ( ; i < num_args; i++) {


That for loop looks like an error to me - I know it's not, but the lack of an initialisation expression is weird. while (i++ < num_args) { would seem better to me. But what do I know.

right :)
I'm in the process of fixing.

Dmitry.

On Wed, Jan 28, 2015 at 5:31 AM, Andrea Faulds [email protected]
wrote:

In Zend/zend_opcode.c
#997 (comment):

if (op_array->fn_flags & ZEND_ACC_VARIADIC) { num_args++; }

for (i = 0; i < num_args; i++) {

for ( ; i < num_args; i++) {

This looks to my eyes like an error... while (i++ < num_args) { would
seem better to me. But what do I know.

—
Reply to this email directly or view it on GitHub
https://github.com/php/php-src/pull/997/files#r23661891.

It may just be the space that throws me off, for (; i < num_args; i++) { would be fine and probably better than using a while loop

Ooh, this may be causing a segfault actually:

http://chat.stackoverflow.com/transcript/message/21204876#21204876

morrisonlevi force-pushed the typed_returns branch from ef8e348 to c322955 Compare January 13, 2015 22:40

morrisonlevi force-pushed the typed_returns branch from c322955 to f047cf7 Compare January 14, 2015 09:08

morrisonlevi changed the title ~~Implement return type hinting~~ Implement return types Jan 14, 2015

morrisonlevi force-pushed the typed_returns branch from f047cf7 to a0ce77a Compare January 14, 2015 09:08

derickr reviewed Jan 14, 2015
View reviewed changes

jpauli added PHP7 RFC labels Jan 14, 2015

beberlei reviewed Jan 16, 2015
View reviewed changes

morrisonlevi force-pushed the typed_returns branch 2 times, most recently from cfd7dcf to 0009013 Compare January 27, 2015 18:49

Implement return types

c8576c5

RFC is documented here: https://wiki.php.net/rfc/return_types

morrisonlevi force-pushed the typed_returns branch from 0009013 to c8576c5 Compare January 27, 2015 18:50

hikari-no-yume reviewed Jan 28, 2015
View reviewed changes

php-pulls merged commit c8576c5 into php:master Jan 28, 2015

		@@ -0,0 +1,28 @@
		--TEST--
		Method returned callable, expected callable

Implement return types #997

Implement return types #997

Uh oh!

Conversation

morrisonlevi commented Jan 13, 2015

Uh oh!

HallofFamer commented Jan 14, 2015

Uh oh!

morrisonlevi commented Jan 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HallofFamer commented Jan 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm commented Jan 24, 2015

Uh oh!

dstogov commented Jan 24, 2015

Uh oh!

staabm commented Jan 24, 2015

Uh oh!

staabm commented Jan 24, 2015

Uh oh!

dstogov commented Jan 26, 2015

Uh oh!

rdlowrey commented Jan 28, 2015

Uh oh!

hikari-no-yume commented Jan 28, 2015

Uh oh!

rdlowrey commented Jan 28, 2015

Uh oh!

marcioAlmada commented Jan 28, 2015

Uh oh!

hikari-no-yume commented Jan 28, 2015

Uh oh!

dstogov commented Jan 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!