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

Returntypehinting RFC #653

Closed
wants to merge 81 commits into from
Closed

Returntypehinting RFC #653

wants to merge 81 commits into from

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Apr 26, 2014

--FILE--
<?php
class Foo {
public static function getInstance() : self {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about parent & static? Are they supported? If yes, they should be in the tests too.

Copy link
Member Author

Choose a reason for hiding this comment

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

They aren't supported, it's being discussed, I'm really just following direction, if it's decided they'll be supported tests will appear ;)

@krakjoe
Copy link
Member Author

krakjoe commented May 10, 2014

ATT @dstogov @smalyshev @ircmaxell @nikic @bwoebi @felipensp @derickr

Opcache changes what you can do at compilation time, because it resets class (etc) tables for every file it compiles, you can't lookup internal classes or classes compiled in another file at compile time.

I'm not actually sure what the proper solution is, I don't like the idea of these checks being performed at runtime at all, it complicates everything, but the choices are fix opcache, or change the implementation, and since opcache is "core" I don't know what way to go from here ...

The oddities in the implementation (failing tests) are because I'm sort of up against a brick wall with the opcache thing ... hit me with some knowledge/wisdom/direction to move forward, moving forward is either finalizing this implementation and hoping dmitry will make opcache not change what is possible, or re-implmenting everything at runtime as vm opcodes, and then having to change opcache's optimizer (don't know if I can do that) because ZEND_RETURN will no longer mark the end of a code path.

I rather hope, I'm wrong, and there is some way that I do not see to keep everything as it is now (simple) ...

parent/static not supported, so not tested, we could add tests to show they are not supported I suppose ...

@dstogov
Copy link
Member

dstogov commented May 12, 2014

Hi Joe,

I would definitely help you with this proposal and its integration with
opcache, but I would postpone it a bit.
At first, I'm currently overloaded with phpng related tasks.
At second, I think we should implement typehinting at the moment when we
may really benefit from it (using JIT).
Otherwise, we may make us more troubles in the future.

Thanks. Dmitry.

On Sat, May 10, 2014 at 10:45 AM, Joe Watkins notifications@github.comwrote:

ATT @dstogov https://github.com/dstogov @smalyshevhttps://github.com/smalyshev
@ircmaxell https://github.com/ircmaxell @NikiChttps://github.com/NikiC
@bwoebi https://github.com/bwoebi @felipensphttps://github.com/felipensp
@derickr https://github.com/derickr

Opcache changes what you can do at compilation time, because it resets
class (etc) tables for every file it compiles, you can't lookup internal
classes or classes compiled in another file at compile time.

I'm not actually sure what the proper solution is, I don't like the idea
of these checks being performed at runtime at all, but the choices are fix
opcache, or change the implementation, and since opcache is "core" I don't
know what way to go from here ...

The oddities in the implementation (failing tests) are because I'm sort of
up against a brick wall with the opcache thing ... hit me with some
knowledge/wisdom/direction to move forward, moving forward is either
finalizing this implementation and hoping dmitry will make opcache not
change what is possible, or re-implmenting everything at runtime as vm
opcodes, and then having to change opcache's optimizer (don't know if I can
do that) because ZEND_RETURN will no longer mark the end of a code path.

parent/static not supported, so not tested, we could add tests to show
they are not supported I suppose ...


Reply to this email directly or view it on GitHubhttps://github.com//pull/653#issuecomment-42732939
.

@laruence
Copy link
Member

Agree with dmitry , thanks



class qux implements bar {
public function foo() : bar {
Copy link

Choose a reason for hiding this comment

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

Sorry for commenting here instead of the list, but in looking through the example code snippets in all these tests, a single question occurred to me all over again:

Why is there a colon between the function signature and the return type hint?

The colon is one of the few operators in PHP that is consistent with other languages; specifically in ternary operators. The suggested new usage makes code visually appear as

function this OR that

Is there a particular reason for why we need it? What's wrong with the following?

-  public function foo() : bar {
+  public function foo() bar {

The RFC only states the following in its References:

Suggestion 5 is nearly compatible with this RFC; however, it requires the addition of a new token T_RETURNS. This RFC opted for a syntax that does not require additional tokens so returns was replaced by a colon.

At the same time, the Position chapter does not show a colon in its comparison to other languages:

  • After the parameter list: function_name() return_type;

To be clear: Only questioning why there has to be a delimiter. I'm NOT questioning the placement/position of the return type hint. (After thinking through all possible options, the placement makes sense to me.)

Apologies in case this was discussed before already. I'm new to internals. I read the RFC. The mailing list based infrastructure makes it a bit hard to retroactively follow discussions. In case this was actively decided already, feel free to just point me to the thread that I may have missed.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Aug 20, 2014 at 6:10 PM, sun notifications@github.com wrote:

In Zend/tests/return_hint/008.phpt:

@@ -0,0 +1,24 @@
+--TEST--
+Basic return hints covariance interfaces
+--FILE--
+<?php
+interface foo {}
+interface bar extends foo {

  • public function foo() : foo;
    +}

+class qux implements bar {

  • public function foo() : bar {

Sorry for commenting here instead of the list, but in looking through the
example code snippets in all these tests, a single question occurred to me
all over again:

Why is there a colon between the function signature and the return type
hint?

The colon is one of the few operators in PHP that is consistent with other
languages; specifically in ternary operators. The suggested new usage makes
code visually appear as function this OR that.

Is there a particular reason for why we need it? What's wrong with the
following?

  • public function foo() : bar {+ public function foo() bar {

The RFC only states the following in its References
https://wiki.php.net/rfc/returntypehinting#references:

Suggestion 5 is nearly compatible with this RFC; however, it requires the
addition of a new token T_RETURNS. This RFC opted for a syntax that does
not require additional tokens so returns was replaced by a colon.

At the same time, the Position
https://wiki.php.net/rfc/returntypehinting#position_of_type_declaration
chapter does not show a colon in its comparison to other languages:

  • After the parameter list: function_name() return_type;

    To be clear: Only questioning why there has to be a delimiter. I'm NOT
    questioning the placement/position of the return type hint. (After thinking
    through all possible options, the placement makes sense to me.)

Apologies in case this was discussed before already. I'm new to
internals. I read the RFC. The mailing list based infrastructure makes it a
bit hard to retroactively follow discussions. In case this was actively
decided already, feel free to just point me to the thread that I may have
missed.

It's late for me; I'll respond properly tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose the syntax with column came from Virt family languages (Pascal,
Modula, Ada, etc).
It looks natural for me and it shouldn't make any grammar conflicts with
ternary operator.

Thanks. Dmitry.

On Thu, Aug 21, 2014 at 4:10 AM, sun notifications@github.com wrote:

In Zend/tests/return_hint/008.phpt:

@@ -0,0 +1,24 @@
+--TEST--
+Basic return hints covariance interfaces
+--FILE--
+<?php
+interface foo {}
+interface bar extends foo {

  • public function foo() : foo;
    +}

+class qux implements bar {

  • public function foo() : bar {

Sorry for commenting here instead of the list, but in looking through the
example code snippets in all these tests, a single question occurred to me
all over again:

Why is there a colon between the function signature and the return type
hint?

The colon is one of the few operators in PHP that is consistent with other
languages; specifically in ternary operators. The suggested new usage makes
code visually appear as function this OR that.

Is there a particular reason for why we need it? What's wrong with the
following?

  • public function foo() : bar {+ public function foo() bar {

The RFC only states the following in its References
https://wiki.php.net/rfc/returntypehinting#references:

Suggestion 5 is nearly compatible with this RFC; however, it requires the
addition of a new token T_RETURNS. This RFC opted for a syntax that does
not require additional tokens so returns was replaced by a colon.

At the same time, the Position
https://wiki.php.net/rfc/returntypehinting#position_of_type_declaration
chapter does not show a colon in its comparison to other languages:

  • After the parameter list: function_name() return_type;

    To be clear: Only questioning why there has to be a delimiter. I'm NOT
    questioning the placement/position of the return type hint. (After thinking
    through all possible options, the placement makes sense to me.)

Apologies in case this was discussed before already. I'm new to
internals. I read the RFC. The mailing list based infrastructure makes it a
bit hard to retroactively follow discussions. In case this was actively
decided already, feel free to just point me to the thread that I may have
missed.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Aug 20, 2014 at 11:42 PM, Dmitry Stogov
notifications@github.com wrote:

I suppose the syntax with column came from Virt family languages (Pascal, Modula, Ada, etc). It looks natural for me and it shouldn't make any grammar conflicts with ternary operator.

Yes, the syntax is precedented in these languages and others. It's a
common way to denote types of things in general. One language of
interest is Hack which also uses a colon here.

In regards to the position section: I didn't put symbols in there
because many languages put return types there but use different
symbols for it. I was just demonstrating common positions, not syntax
itself.

Copy link

Choose a reason for hiding this comment

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

One language of interest is Hack which also uses a colon here.

That argument is most likely the most sensible and most compelling one, thanks.

I was mainly concerned about (1) needless visual clutter, and also (2) requiring user-space Tokenizer-based code parsers to additionally account for the (optional) T_WHITESPACE, ":", (optional) T_WHITESPACE sequence.

It's a minor issue. However, since it is reasonable to expect that we'll see an adoption of this grammar in >80% of all PHP code in the future (probably 100% after scalar type hints are in), I thought it would be worth to raise the question.

Given that this can be considered a deliberate choice, clarifying it in the RFC would be welcome.

Thanks! - Really looking forward to use this! :-)

@smalyshev smalyshev added the RFC label Nov 18, 2014
@HallofFamer
Copy link

For class methods, is it possible to remove the requirement to use 'function' keyword? Instead, declare the return type in similar way as C++/Java/C#, like the example given below?

php 5: public function getIntValue(){}
php 7: public int getIntValue(){}

Also by 'remove the requirement' I mean the keyword 'function' just becomes optional, so you can either leave it out or keep it. This way it ensures backward compatibility, while allows other developers to move forward.

@hikari-no-yume
Copy link
Contributor

Request is now unneeded because the other implementation got merged... so this should be closed.

@php-pulls
Copy link

Comment on behalf of laruence at php.net:

closed

@php-pulls php-pulls closed this Feb 7, 2015
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