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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added mixed argument type & return type #2603

Open
wants to merge 2 commits into
base: master
from

Conversation

@Majkl578
Copy link
Contributor

Majkl578 commented Jun 30, 2017

This is a proof of concept for built-in mixed type for parameter types and return types.
The behavior of mixed type matches the behavior when no type is specified (thus being implicitly mixed).
Primary motivation for having explicit mixed type is consistence and easier static analysis. In PHP 7.2, mixed types are unfortunately the only type that could not be type hinted upon (true, resource as well, but its future is unclear).
I believe having mixed type would polish the code interface even more, thus having code with 100% explicit type hint coverage.


Before:

<?php
function foo($arg) {
    return $arg;
}

Now:

<?php
function foo(mixed $arg): mixed {
    return $arg;
}

(Note that I barely know the PHP internals so this may be entirely wrong approach.)


TODOs:

  • disalow ?mixed at compile time?
  • add more tests?

This probably needs an RFC, although mixed is already a reserved type. Since I don't have permission to create RFCs, a sponsor would be welcomed (anyone? 馃槆).
Not sure whether it'd be too late for inclusion in 7.2 in case it'd be accepted (probably not yet since there's not even a beta yet).

What do you think? 馃殺

@krakjoe krakjoe added the RFC label Jul 4, 2017

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jul 6, 2017

@sgolemon @remicollet Hi, do you think this would still be acceptable for 7.2?
Scheduled feature freeze for 7.2 is on July 20th and as RFCs require 14 days discussion period + 7-14 days voting period, it's unfortunately after the deadline. :/
But since this would be a tiny language change with no BC breaks, just an alias to existing behavior, I think it'd be okay to eventually fine to introduce in later beta. Or not?

@Majkl578 Majkl578 force-pushed the Majkl578:mixed-type branch 2 times, most recently from 5c0b355 to e1142bb Jul 6, 2017

@KalleZ

This comment has been minimized.

Copy link
Contributor

KalleZ commented Jul 6, 2017

I was recently hit by this oddity, I actually thought we reserved it and made it possible with the introduction in 7.0, get this in!

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jul 6, 2017

I actually thought we reserved it and made it possible with the introduction in 7.0,

mixed is reserved as of PHP 7.0, but it has never been implemented (pretty much like object). :/ Having object in 7,2 is just another reason why this would be nice to have in 7.2 as well. 馃憤

@Majkl578 Majkl578 force-pushed the Majkl578:mixed-type branch from e1142bb to 60aa68b Jul 11, 2017

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jul 19, 2017

Ping @sgolemon @remicollet - what do you think about this, is it already too late or would it be acceptable as a self-contained addition aliasing current behavior?
Note that I've requested Wiki karma in php.internals to write an RFC over a week ago and got absolutely no reply so far... 馃槥

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Jul 19, 2017

You have missed the official feature freeze (yesterday), while the branch is in pre-release mode, we can't do anything.

The thing to do is let the RFC process run, we can see where we are at the end of that once all discussion and voting is done. It may be acceptable to merge once GA, although this is not my call.

You have permission to write an rfc on wiki.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jul 19, 2017

@b1rdex

This comment has been minimized.

Copy link

b1rdex commented Dec 1, 2017

@Majkl578 have you started RFC discussion?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Dec 1, 2017

Not yet, it's not finished and there is plenty of time for 7.3. :)

@Majkl578 Majkl578 force-pushed the Majkl578:mixed-type branch from 60aa68b to e6921a9 Dec 19, 2017

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Dec 19, 2017

I must admit I have no clue why one build target fails on two of these tests and the other one is green, both Travis and AppVeyor. It was fine before rebase in which I only changed type code in zend_types.h. :/
Can't reproduce any failure localy... Could anyone provide any pointers? Thanks.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Dec 19, 2017

@jordyvandomselaar

This comment has been minimized.

Copy link

jordyvandomselaar commented Dec 19, 2017

Why would you typehint a non type? Then just don't typehint -.-, this undoes everything typehinting was supposed to fix.

@b1rdex

This comment has been minimized.

Copy link

b1rdex commented Dec 20, 2017

@jordyvandomselaar @carusogabriel answer for your question is in first comment 鈥斅爐his feature is supposed to add more clearance to code to show where mixed is really accepted and where just no type hint specified.

@jordyvandomselaar

This comment has been minimized.

Copy link

jordyvandomselaar commented Dec 20, 2017

鈥 And not typehinting wouldn't be clear enough? No type hints === no specified type === throw in whatever you'd like.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Dec 20, 2017

I am not going to argue with people who don't understand difference between mixed type and no type, sorry. Look at code that uses 7.1 and maintains BC for another X years. Example: class with 10 recently added methods with typehints, 10 old methods without anything to maintain BC. Now tell anyone whether mixed is intentional or not.

@jordyvandomselaar

This comment has been minimized.

Copy link

jordyvandomselaar commented Dec 20, 2017

There isn't a difference between no type and a mixed type. Intentional or not, old code will still accept any type, a.k.a. mixed type. If you're upgrading to php 7.2 I suggest you refactor code that ends up using type hints anyways.

@Htarlov

This comment has been minimized.

Copy link

Htarlov commented Dec 22, 2017

I'm not sure it's very good idea.

You always can add comment to hint that type hint is ommited on purpose (with some inforation why it was done).

With mixed type there will be situations when someone asks "please add type hints to your code" or there will be a requirement that code has to be with type hints and then a programmer will add massive amounts of "mixed" type hints - "because deadline".

}
}
class Baz extends bar {

This comment has been minimized.

@carusogabriel

carusogabriel Jan 20, 2018

Member

Minor: bar in lower case

@thekid thekid referenced this pull request Jan 21, 2018

Closed

Support "mixed" #28

@Taluu

This comment has been minimized.

Copy link

Taluu commented May 14, 2018

Bumping, would like to see that one in 7.3 if possible... Because I transformed a few of my interfaces to mixed type, where it makes sense. and not adding any typehint kinda annoys me.

@ostrolucky

This comment has been minimized.

Copy link

ostrolucky commented May 14, 2018

@Htarlov has very good point. Developers will just start adding mixed typehints mindlessly and everybody will think they are mixed by purpose.

@Taluu

This comment has been minimized.

Copy link

Taluu commented May 14, 2018

IMO it's the same really as typing nothing, except that if nothing is typed it really means "meh don't care". with the typehint, it's "tried to care, up to you to care to check". It's semantics, really.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented May 14, 2018

@Htarlov has very good point. Developers will just start adding mixed typehints mindlessly and everybody will think they are mixed by purpose.

If anyone wants to shoot themselves into foot, it's their choice.

Similar with Error, you can throw TypeError on your own although it's probably not a good idea.

@jordyvandomselaar

This comment has been minimized.

Copy link

jordyvandomselaar commented May 15, 2018

I still don't think it adds anything but bloat. It's functionality is none, it doesn't help the developer- nor PHP at all. Mixed is implicit if there is no type hint. It literally has no upsides at all.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented May 15, 2018

It's functionality is none

If nothing else, it forbids mixing mixed and void.

it doesn't help the developer

Quite the contrary: the developer instantly knows it's explicitly expected to accept/return mixed, instead of being forgotten type hint or legacy PHP 5 pre-7.1/7.2 code.

Mixed is implicit if there is no type hint.

Don't forget that no return type means EITHER mixed OR void.

@JanTvrdik

This comment has been minimized.

Copy link

JanTvrdik commented May 15, 2018

Don't forget that no return type means EITHER mixed OR void.

mixed (being the top type) is supertype of void (being a unit type).

@jordyvandomselaar

This comment has been minimized.

Copy link

jordyvandomselaar commented May 16, 2018

Void is also a part of mixed, so it literally does absolutely nothing.

@JacobGrady

This comment has been minimized.

Copy link

JacobGrady commented Jun 1, 2018

Bump! I'd also like to see this

@KacerCZ

This comment has been minimized.

Copy link

KacerCZ commented Jun 26, 2018

@Majkl578 Thanks for this proposal, because it can distinct knowingly returning mixed type vs forgotten type.

Feature freeze for PHP 7.3 is near, do you plan to put this RFC to vote?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 26, 2018

I will probably soon abandon the RFC, given all the hate here and on Twitter. It's not my passion to contribute interesting ideas to ignorant and hostile community.

@zerkms

This comment has been minimized.

Copy link

zerkms commented Jun 26, 2018

I think it's relevant: what do you people think of algebraic data types instead?

If those were implemented - mixed would not be needed. And in general those would be more expressive and powerful.

@KacerCZ

This comment has been minimized.

Copy link

KacerCZ commented Jun 27, 2018

@zerkms Union and intersection types are far more complex problem than adding mixed type.
As stated in original post and related RFC - its already reserved word and is used in documentation, this patch will enable to explicitly use this type in function signatures.

@kunicmarko20

This comment has been minimized.

Copy link

kunicmarko20 commented Jun 27, 2018

because it can distinct knowingly returning mixed type vs forgotten type.

Exactly, would be nice to see this in 7.3

@carusogabriel

This comment has been minimized.

Copy link
Member

carusogabriel commented Jun 28, 2018

@Majkl578 May I continue the work on this RFC and prepare it to open for voting?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 28, 2018

Sure, if you want to give it your time. :)
It needs to be bumped in internals mailing list first, to get attention (ideally also address previous feedback) and then we can open voting on monday.

@carusogabriel

This comment has been minimized.

Copy link
Member

carusogabriel commented Jun 28, 2018

@Majkl578 Perfect. I'll first rebase with master and get a green build and open a PR in your fork. Then, we can proceed with internals

Majkl578 and others added some commits Jun 29, 2018

Added mixed argument type & return type
Co-Authored-By: Gabriel Caruso <carusogabriel34@gmail.com>
Disallow nullable mixed types at compile time
Co-Authored-By: Gabriel Caruso <carusogabriel34@gmail.com>

@Majkl578 Majkl578 force-pushed the Majkl578:mixed-type branch from e6921a9 to eec8dd9 Jun 29, 2018

@Majkl578 Majkl578 referenced this pull request Jun 29, 2018

Closed

Rebased and fixed conflicts #1

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 29, 2018

Rebased the PR to current master (thanks @carusogabriel).
Also slightly updated the RFC with some clarifications: https://wiki.php.net/rfc/mixed-typehint?do=diff&rev2%5B0%5D=1513652589&rev2%5B1%5D=1530279325&difftype=sidebyside

Since I sitll disagree with:

mixed (being the top type) is supertype of void (being a unit type).

I made this explicit in the RFC as mixed is not a top type and thus does not include void.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 29, 2018

@Majkl578 What does "As void is not a real type in PHP, it is not part of mixed type" mean in terms of practical implications? If I'm reading this correctly it would mean that a : mixed method could not be changed to a : void method through inheritance. However, this would be in direct contradiction to the statement in the variance section, which implies that : mixed is equivalent to no type, which can be changed to : void through inheritance.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 29, 2018

t would mean that a : mixed method could not be changed to a : void method through inheritance.

Correct.
Mixed method returns anything while void method returns nothing. (The fact that result of void method is currently actually NULL in PHP is a flaw of the language imho.)

this would be in direct contradiction to the statement in the variance section, which implies that : mixed is equivalent to no type

Good point. I should clarify that section.

Actually, adding : void to a method that returns anything (no return type) is violating LSP, am I right? I am not sure why this was allowed when introducing : void, it doesn't make much sense to me: parent A returns a value, but a child B doesn't return at all

@carusogabriel

This comment has been minimized.

Copy link
Member

carusogabriel commented Jun 29, 2018

@nikic From what I can understand, mixed means we can return any unit type:

function filterValue(mixed $value): mixed
{
    switch (gettype($value)) {
      // a lot of cases with a lot of returns
   }
}

And void means that we do not return anything. Like @Majkl578 said:

Actually, adding : void to a method that returns anything (no return type) is violating LSP, am I right? I am not sure why this was allowed when introducing : void, it doesn't make much sense to me: parent A returns a value, but a child B doesn't return at all

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 30, 2018

@nikic Been thinking about the case you mentioned.

Currently this is possible:

class Foo
{
    function test()
    {}
}
class Bar extends Foo
{
    function test() : void
    {}
}

It would probably be a good idea to ban this when parent is explicitly mixed, so the following would be illegal:

class Foo
{
    function test() : mixed
    {}
}
class Bar extends Foo
{
    function test() : void
    {}
}

But doing so would mean that mixed is no longer equivalent to no return type specified, so the following would no longer be valid:

class Foo
{
    function test() : mixed
    {}
}
class Bar extends Foo
{
    function test()
    {}
}
class Baz extends Bar
{
    function test() : void
    {}
}
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 30, 2018

@Majkl578 I don't understand your last example. Is it missing a : mixed on one of those methods?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 30, 2018

@nikic Yes, sorry, there should be Foo::test() : mixed. Fixed.

@ramsey

This comment has been minimized.

Copy link
Contributor

ramsey commented Jun 30, 2018

If mixed includes void, how would a subclass be contravariant in its return type if it only returns void? That seems, to me, to satisfy the parent return type.

Are you arguing that mixed does/should not include void?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jun 30, 2018

Are you arguing that mixed does/should not include void?

Yes, please read the discussion/RFC.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Jul 3, 2018

FYI: Based on the discussion and clearing up the inheritance issue with void, we're postponing this for 7.4/8.0 (whichever comes first).

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

Majkl578 commented Feb 8, 2019

Published new version: https://wiki.php.net/rfc/mixed-typehint
This PR is not updated yet.

/* Child defines a type, but parent doesn't, violates LSP, unless it's a mixed type */
if (ZEND_TYPE_CODE(fe_arg_info->type) == IS_MIXED) {
return 1;
}
return 0;

This comment has been minimized.

@carusogabriel

carusogabriel Feb 8, 2019

Member

I believe this can be simplified:

Suggested change Beta
return 0;
return ZEND_TYPE_CODE(fe_arg_info->type) == IS_MIXED;
@@ -359,6 +362,9 @@ static zend_bool zend_do_perform_implementation_check(const zend_function *fe, c
if (proto->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
/* Removing a return type is not valid. */
if (!(fe->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE)) {
if (ZEND_TYPE_CODE(proto->common.arg_info[-1].type) == IS_MIXED) {
return 1;
}
return 0;

This comment has been minimized.

@carusogabriel

carusogabriel Feb 8, 2019

Member

I believe this can be simplified:

Suggested change Beta
return 0;
return ZEND_TYPE_CODE(proto->common.arg_info[-1].type) == IS_MIXED;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment