Skip to content

Return types #820

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

Closed
wants to merge 3 commits into from
Closed

Return types #820

wants to merge 3 commits into from

Conversation

morrisonlevi
Copy link
Contributor

This is a pull request to implement the return type RFC: https://wiki.php.net/rfc/returntypehinting

Notes about the implementation:

  • Adds a new opcode ZEND_VERIFY_RETURN_TYPE. When a function has a return type, this opcode will be emitted to check that the return value satisfies the return type.
  • When:
    • a child class inherits from a parent class
    • and the parent class has a method which uses a class or interface as a return type
    • and the method is overwritten
    • then the child class must use delayed binding. In some cases we could do string matching to avoid the delayed binding.
  • Internal classes cannot specify return types, at least not easily.
  • During pass_two if a ZEND_VERIFY_RETURN_TYPE opcode is found in a Generator then it will be made into a no-op. It would be nicer to not emit one for generators, but this is somewhat difficult to do because when you process the return expression you may not know if the function is a generator yet.


case ZEND_FETCH_CLASS_PARENT:
case ZEND_FETCH_CLASS_STATIC:
zend_error_noreturn(E_COMPILE_ERROR, "Cannot use return type of %s", return_type->name->val);
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

yeah.. it could be able to implemented by add a ZEND_FETCH_CLASS before verify return type...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would save you a lot of trouble here. You can just use zend_compile_class_ref and avoid all the manual handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the review; I was not really ready but someone had asked me to open the PR so they could help me with a memory issue I was having.

I tried using zend_compile_class_ref but I need the return_type AST to call it but don't have it in the return AST, which is where this function is actually called from. This function would emit ZEND_FETCH_CLASS if necessary.

Lastly, parent should work (just unimplemented currently) but static doesn't work by design, based on the discussion phase. I haven't gone through and verified if static would break covariance or not; I need to do that soon.

Thanks for the partial review. I still need to implement reflection support and verify generator rules; after that I'll try tackling this in a more efficient way again.

@morrisonlevi morrisonlevi force-pushed the return_types branch 7 times, most recently from d01f821 to 2f5f460 Compare September 29, 2014 22:50
@morrisonlevi morrisonlevi force-pushed the return_types branch 6 times, most recently from 9e8205d to e84237c Compare October 13, 2014 18:46
@morrisonlevi morrisonlevi force-pushed the return_types branch 8 times, most recently from c8d7088 to 8f41c5d Compare November 7, 2014 00:36
@morrisonlevi morrisonlevi force-pushed the return_types branch 3 times, most recently from c2799af to 13f7485 Compare November 17, 2014 15:56
@smalyshev smalyshev added the RFC label Nov 18, 2014
@morrisonlevi morrisonlevi force-pushed the return_types branch 2 times, most recently from ce60d70 to e1a3fd4 Compare November 25, 2014 11:42
Works for methods, functions, and closures (all 'function' types).

There needs to be a few more tests for 'parent' functionality, and
ReflectionType needs to implement get_debug_info.
Clean up and document the covariance check too.
@HallofFamer
Copy link

I like this idea, but the syntax seems a bit strange and cumbersome to me. Is it possible to just replace the function keyword by the return type? For instance, 'public function __toString()' will just become 'public string __toString()' instead. Okay it's not a good example since in PHP string is not a class and may not be valid in return type hinting, but I think you get the idea. The function keyword is redundant, just like var keyword which became obsolete since PHP 5. I dont see why it's still there, the compiler should be able to tell that it's a method rather than a property just by the parenthesis following the method name.

@dstogov
Copy link
Member

dstogov commented Jan 9, 2015

Hi,

Both syntaxes make sense. I, peronally, prefer optioal type after function
arguments.
This syntax consistent with the existent and it's also alrey used in HHVM.

Thanks. Dmitry.
On Jan 9, 2015 7:21 AM, "HallofFamer" notifications@github.com wrote:

I like this idea, but the syntax seems a bit strange and cumbersome to me.
Is it possible to just replace the function keyword by the return type? For
instance, public function __toString() will just become public string
__toString() instead. Okay it's not a good example since in PHP string is
not a class and may not be valid in return type hinting, but I think you
get the idea. The function keyword is redundant, just like var keyword
which became obsolete since PHP 5. I dont see why it's still there, the
compiler should be able to tell that it's a method rather than a property
just by the parenthesis following the method name.


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

@HallofFamer
Copy link

I see, if so maybe they can make both syntax valid so developers can choose one over the other, and each company can set its own standard for this? I personally do not think the choice of HHVM should matter in the case of PHP in any way though, after HHVM derives from PHP, not the other way around. I just dont think the function keyword should ever exist for class/object anymore, no matter whether return type is declared before or after the method name, just remove the function keyword requirement. For orphan functions though, maybe the function keyword has its reason to exist, but in a class/object I dont see a point.

@morrisonlevi
Copy link
Contributor Author

@HallofFamer, you do not seem to understand that removing the function keyword will not fix anything. What about anonymous functions and closures? They can have return types too; should we just remove function? Parsing rules aside, I think this looks bad:

$var = array() {
     return [];
};

Additionally, you can no longer search for function foo to find the declarations of methods/functions.

No, the syntax was not chosen because it's what Hack has; rather the Hack team made a good decision and for the same and similar reasons we've chosen the same syntax.

@HallofFamer
Copy link

@morrisonlevi: Okay, I personally do not really think anonymous functions and closures look bad without function keyword. Instead, it should've been this way from the very beginning. We can use lambda expression like this:

($a, $b) use ($c, $d) => {return ($a == $c ) and ($b != $d);}

In fact, I am surprised such lambda expression syntax hasnt been implemented, and the fact that anonymous function requires function keyword actually is a drawback. I will in fact use anonymous function more if function keyword is not required, than I do now.

I also dont think the ability to search for function foo in IDE to find declaration of methods/functions is anything that actually matters, so a keyword exists primarily for searching/indexing purposes? Is there something I am missing?

So my point stands as that function keyword is redundant in method declaration, I think its only usage should be to declare orphan functions that are not used as class member functions/methods. No matter where return type is specified, it's better to remove the function keyword. It will fix something, it will save some typing, and it will make the code look more elegant.

@morrisonlevi
Copy link
Contributor Author

so a keyword exists primarily for searching/indexing purposes? Is there something I am missing?

Yes, there is. Consider this snippet: $f = vector($a, $b) is it a function call, or an anonymous function that returns vector? It depends on what comes next, but that's quite a ways into the expression to detect what is being done. It's possible but not trivial.

It will fix something, it will save some typing, and it will make the code look more elegant.

It fixes nothing but will break tools and workflows that use function foo to find foo's definition. Without the function keyword, can you give me an easy to type and remember way to find foo's definition?

I appreciate your feedback, but none of this is new information. All of these things and more have been considered by Internals. In the past people have proposed replacing the function keyword and their RFC's were dropped.

@HallofFamer
Copy link

Well I think you should ask those who designed Java and C# that question, they do not use the function keyword and no one has a problem with it. I just know that function keyword is redundant, its less elegant and just adds to more typing. I cant speak for PHP internals, but from a developer's point of view the requirement to use function keyword is a drawback, not a plus. Especially if you add return type hinting to it, a method declaration will become very messy and hard to read.

Yeah maybe it helps with searching, but again I dont think it's that important. In fact, if you follow good object oriented design, there should be no need to explicitly search for a method declaration. Because when you have to actually go as far as to find foo's definition, it means your class is too long and your file is too big to navigate properly. It will be about time to refactor your code, dont you think so?

@morrisonlevi
Copy link
Contributor Author

Well I think you should ask those who designed Java and C# that question, they do not use the function keyword and no one has a problem with it

These are also compiled languages, unlike PHP which is interpreted. We have to pay that cost at runtime, but they pay it at compile time.

In fact, if you follow good object oriented design, there should be no need to explicitly search for a method declaration.

This is untrue. Pretty much every project uses methods on classes that were defined in other files, and you don't always know which file contains the declaration.

@morrisonlevi
Copy link
Contributor Author

Closing; refer to pull request #986 instead.

@morrisonlevi morrisonlevi deleted the return_types branch January 13, 2015 21:20
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 170d129 on morrisonlevi:return_types into * on php:master*.

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.

8 participants