Parameter type widening RFC

Fleshgrinder

9 years ago

On 5/23/2017 9:25 PM, Rasmus Schultz wrote:

> This parameter type widening RFC, I didn't know about, but I have a remark.
>
> The feature, as implemented, will allow accidental omission of type-hints,
> will it not?
>
> Previously, the implements keyword guaranteed a correctly type-hinted
> implementation - it's now possible to (purposefully, obviously, in certain
> rare cases) omit type-hints by accident, making the implements keyword much
> less (or not any) of a guarantee that the interface is implemented
> correctly.
>
> The addition of an explicit "mixed" or "any" pseudo-type would have made
> this possible, without losing the guarantee that the implements keyword
> used to provide - that is, it would have been possible to have this feature
> for the few cases where it's useful, without affecting safety in the
> majority of other cases where it's not.
>
> I feel like this feature takes a pretty dangerous shortcut by simply
> removing a constraint check that we used to have - in favor of supporting a
> few rare cases, we removed a guarantee that interfaces and the
> implements-keyword has always provided.
>
> Those rare cases could have been supported in a safe manner by introducing
> a "mixed" or "any" type, which would have made the use of this feature
> explicit - which would have removed the risk of accidental omission of
> type-hints in the majority of cases.
>
> The RFC page doesn't link to any discussion, and the Github thread was shut
> down after some negative remarks.
>
> I didn't see a discussion or a vote here on internals - did I miss
> something? Where or how did this get discussed and passed? Are these
> discussions happening somewhere else besides internals now?
>

Hey Rasmus!

- Discussion: https://marc.info/?t=147972136200001&r=1&w=2

I also had a look at the GitHub discussion, and I think that the things
that were written there have nothing to do with your concern. The people
commenting there simply did not understand LSP.

That being said, your remark is actually more than legit and I have to
agree that this is indeed just crying out for static analysis tools to
add more warnings to code.

I would very much love to see "any", but introducing yet another keyword
is imho not a good idea. We have already an extremely mixed up
terminology, hence, "mixed" would do and match the existing grammar of
PHP nicely.

There is time left for PHP 7.2 (20. July I think), maybe this is fixable
before that? I guess this will require an RFC too though.

--
Richard "Fleshgrinder" Fussenegger

Andrey Andreev

9 years ago

Hi,

On Tue, May 23, 2017 at 10:38 PM, Fleshgrinder <php@fleshgrinder.com> wrote:

>
> I also had a look at the GitHub discussion, and I think that the things
> that were written there have nothing to do with your concern. The people
> commenting there simply did not understand LSP.
>

Well, here's one who doesn't claim to fully understand LSP -> Me

I'm trying, but fail to find a source that says replacing stdClass
with mixed/any/etc is ok. Every (re)definition and example talks about
substituting *objects* with their subtypes, and one particular source*
even explicitly defines what is considered a subtype.
The RFC is very much lacking in details and I can only assume, by the
above logic, that it considers "mixed" a subtype of ... everything?
What am I missing?

* http://www.engr.mun.ca/~theo/Courses/sd/5895-downloads/sd-principles-3.ppt.pdf

Cheers,
Andrey.

Marco Pivetta

9 years ago

On Thu, May 25, 2017 at 4:30 PM, Andrey Andreev <narf@devilix.net> wrote:

> I'm trying, but fail to find a source that says replacing stdClass
> with mixed/any/etc is ok.

It is OK for a subtype to handle a wider range of types than its contract,
it is not ok for the subtype to handle a smaller range of types than its
contract.

A contract says what you "must" be able to do to satisfy it, so the
invariants hold.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Andrey Andreev

9 years ago

Hi Marco,

On Thu, May 25, 2017 at 6:58 PM, Marco Pivetta <ocramius@gmail.com> wrote:

> On Thu, May 25, 2017 at 4:30 PM, Andrey Andreev <narf@devilix.net> wrote:
>>
>> I'm trying, but fail to find a source that says replacing stdClass
>> with mixed/any/etc is ok.
>
>
> It is OK for a subtype to handle a wider range of types than its contract,
> it is not ok for the subtype to handle a smaller range of types than its
> contract.
>
> A contract says what you "must" be able to do to satisfy it, so the
> invariants hold.
>

Sorry, but by "source" I didn't mean somebody state it here. :)

When I said I don't claim to fully understand LSP, I didn't mean "tell
me what it is" - I'm familiar with it. What I don't understand is how
do we get from this:

> Wikipedia:
>
> an object of type T may be substituted with any object of a subtype S

To this:

> A single type may be substituted by any other type

Cheers,
Andrey.

Fleshgrinder

9 years ago

On 5/25/2017 6:38 PM, Andrey Andreev wrote:

> Sorry, but by "source" I didn't mean somebody state it here. :)
>
> When I said I don't claim to fully understand LSP, I didn't mean "tell
> me what it is" - I'm familiar with it. What I don't understand is how
> do we get from this:
>
>> Wikipedia:
>>
>> an object of type T may be substituted with any object of a subtype S
>
>
> To this:
>
>> A single type may be substituted by any other type
>
>
> Cheers,
> Andrey.
>

Hey Andrey!

Do not mix the LSP with type variance on arguments or return types. The
LSP is targeting subtyping directly. The quoted sentence refers to the
fact that S is not allowed to behave differently than T. I think you get
a better understanding if you read the "A typical violation" part of the
Wikipedia article.

The problem we are dealing with is variance of method arguments (and
return values). The corresponding Wikipedia article is here:

https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)

Specifically contravariant arguments:

https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)#Contravariant_method_argument_type

Hope this helps to clarify things. :)

--
Richard "Fleshgrinder" Fussenegger

Dan Ackroyd

9 years ago

Andrey Andreev <narf@devilix.net> wrote:

> When I said I don't claim to fully understand LSP

Hi Andrey,

The RFC specifically didn't mention LSP....because that is separate
from co/contravariance. It's unfortunate for other people to be
throwing the two around at you with a lack of precision.

(disclaimer - I helped Niklas draft the RFC, but I still muddle up the
terms occassionaly also.)

> What I don't understand is how do we get from this...To this:

It's not difficult, just subtle. First, some code, to define some classes:

interface A {
function foo(Bar $bar);
}

class B implements A {
function foo($bar) {
// this function is capable of handling either objects
// of type Bar, or strings
}
}

There are two separate things here.

First - method B::foo is contravariant to method A::foo, which just
means that anything that can be successfully passed as the parameter
to A::foo can also be passed to B::foo.

Second - because of the contravariance, and because PHP allows classes
to implement interfaces, the Liskov substition principle kicks in, and
we can substitute an object of type B anywhere where an object of type
A is expected.

e.g. If we have a function that takes A as a parameter:

function doSomething(A $a) {
$a->foo(new Bar());
}


Both of these are guaranteed* to be work equivalently due to LSP:

doSomething(new A());
doSomething(new B());

> by the above logic, that it considers "mixed" a subtype of ... everything?

No, I think you've been confused by people saying LSP where they meant
contravariance. Although class B can accept either an object of type
Bar, or a string that doesn't meant that a string is a sub-type of
Bar.

Instead because B obeys the contravariance rules for the
implementation of the method, an object of type B can be substituted
where A is expected.

Incidentally, this would be a lot nicer if we had passed the union
type RFC, and so were able to write:

class B implements A {
function foo(Bar|string $bar) {
// hurrah for static code inspection.
}
}


Or for avoiding 'clashing' interfaces:

interface C {
function foo(Bar $bar);
}
interface D {
function foo(string $bar);
}
class E implements C, D {
function foo(Bar|string $bar) {
//This method satifies the contracts for both interfaces.
}
}

> Every (re)definition and example talks about
> substituting *objects* with their subtypes

It is a little unfortunate that a lot of information about type
systems is presented in object-oriented languages, as the same thing
applies to things that aren't objects, e.g. arrays, generators =>
iterable and also callback functions e.g. the callable passed to
set_exception_handler() must accept at least \Exception in PHP 5.6 and
\Throwable in PHP 7, but it's fine for it to accept a wider set of
parameters.

cheers
Dan

*guarantees about program behaviour with LSP are kind of...bogus. In
fact it's probably better to concentrate on how it usually makes your
program be easier to write, rather than relying too heavily on it for
an application to actually 'behave' correctly in all scenarios.

Andrey Andreev

9 years ago

Hi,

On Thu, May 25, 2017 at 11:02 PM, Dan Ackroyd <danack@basereality.com> wrote:

>
> The RFC specifically didn't mention LSP....because that is separate
> from co/contravariance. It's unfortunate for other people to be
> throwing the two around at you with a lack of precision.
>

Perhaps this was the issue ... I was under the impression that LSP was
used as (part of) the motivation for the RFC.

The rest is pretty clear, though thanks for the lengthy explanation. :)

Cheers,
Andrey.

Nikita Popov

9 years ago

On Fri, May 26, 2017 at 10:35 AM, Andrey Andreev <narf@devilix.net> wrote:

> Hi,
>
> On Thu, May 25, 2017 at 11:02 PM, Dan Ackroyd <danack@basereality.com>
> wrote:
> >
> > The RFC specifically didn't mention LSP....because that is separate
> > from co/contravariance. It's unfortunate for other people to be
> > throwing the two around at you with a lack of precision.
> >
>
> Perhaps this was the issue ... I was under the impression that LSP was
> used as (part of) the motivation for the RFC.
>

The RFC respects the variance rules, the variance rules are a direct
consequence of LSP. That's why we're talking about LSP in this context. The
implication chain is LSP => variance => this RFC.

Nikita

Niklas Keller

9 years ago

2017-05-26 10:35 GMT+02:00 Andrey Andreev <narf@devilix.net>:

> Hi,
>
> On Thu, May 25, 2017 at 11:02 PM, Dan Ackroyd <danack@basereality.com>
> wrote:
> >
> > The RFC specifically didn't mention LSP....because that is separate
> > from co/contravariance. It's unfortunate for other people to be
> > throwing the two around at you with a lack of precision.
> >
>
> Perhaps this was the issue ... I was under the impression that LSP was
> used as (part of) the motivation for the RFC.
>
> The rest is pretty clear, though thanks for the lengthy explanation. :)

Sorry for the delay and confusion. As Nikita already mentioned, it's a
consequence of LSP, not directly what LSP is about.

Regards, Niklas

Christoph Becker

9 years ago

On 26.05.2017 at 12:16, Niklas Keller wrote:

> 2017-05-26 10:35 GMT+02:00 Andrey Andreev <narf@devilix.net>:
>
>> On Thu, May 25, 2017 at 11:02 PM, Dan Ackroyd <danack@basereality.com>
>> wrote:
>>>
>>> The RFC specifically didn't mention LSP....because that is separate
>>> from co/contravariance. It's unfortunate for other people to be
>>> throwing the two around at you with a lack of precision.
>>
>> Perhaps this was the issue ... I was under the impression that LSP was
>> used as (part of) the motivation for the RFC.
>>
>> The rest is pretty clear, though thanks for the lengthy explanation. :)
>
> Sorry for the delay and confusion. As Nikita already mentioned, it's a
> consequence of LSP, not directly what LSP is about.

Indeed, it is not about LSP, so I suggest to fix the note in UPGRADING[1].

[1]
<https://github.com/php/php-src/blob/e5c273dc0ea4cb0e0583af9b4c5889a8c431264f/UPGRADING#L100-L103>

--
Christoph M. Becker

lists@rhsoft.net

9 years ago

Am 26.05.2017 um 14:16 schrieb Christoph M. Becker:

> On 26.05.2017 at 12:16, Niklas Keller wrote:
>
>> 2017-05-26 10:35 GMT+02:00 Andrey Andreev <narf@devilix.net>:
>>
>>> On Thu, May 25, 2017 at 11:02 PM, Dan Ackroyd <danack@basereality.com>
>>> wrote:
>>>>
>>>> The RFC specifically didn't mention LSP....because that is separate
>>>> from co/contravariance. It's unfortunate for other people to be
>>>> throwing the two around at you with a lack of precision.
>>>
>>> Perhaps this was the issue ... I was under the impression that LSP was
>>> used as (part of) the motivation for the RFC.
>>>
>>> The rest is pretty clear, though thanks for the lengthy explanation. :)
>>
>> Sorry for the delay and confusion. As Nikita already mentioned, it's a
>> consequence of LSP, not directly what LSP is about.
>
> Indeed, it is not about LSP, so I suggest to fix the note in UPGRADING[1]

does that also fix the issue https://bugs.php.net/bug.php?id=74394

Dan Ackroyd

9 years ago

On 26 May 2017 at 13:23, lists@rhsoft.net <lists@rhsoft.net> wrote:

> does that also fix the issue https://bugs.php.net/bug.php?id=74394

Dear Anonymous,

That "issue" is actually 3 issues.

case 1

class A {
public function test($a) {}
}
class B extends A {
public function test(string $a) { }
}

This breaks LSP - because B::test doesn't accept all the things that
A::test can accept and so is unlikely to ever be supported.


case 2

class A {
public function test($a) {}
}

class B extends A {
public function test($a): string {}
}

This works since return types were introduced.

case 3

class A {
public function test($a): string { }
}

class B extends A {
public function test($a) {}
}

This breaks LSP - anything that is calling A::test would only expect a
string to be returned, but B::test can return anything so is unlikely
to ever be supported.

btw I assume that lists@rhsoft.net and spam2 at rhsoft dot net are the
same anonymous person. Quoting some of your words:

spam2 at rhsoft dot net

>
> maybe you guys also don't see the possibility of shared-libraries used by hundrets of
> applications by just use "include_path" when you talk about "deplyoment" and "2017"....
>
> that is not possible with a stupid fatal error

lists@rhsoft.net wrote:

>
> frankly get out of my sight with composer
>
> i maintain the whole webstacke for many years at my own (rpm packages) and so the Fedora > repos have excluded anything relevant to PHP - trying to build composer by just download > the src.rpm ends in a ton of build dependencies
>
> we maintain a large codebase (250000 LOC) over 15 years

Your tone of words is quite rude and dismissive of other people both
on this list and in the bug reports. That's really not a good way to
persuade people, but particularly so when you seem to not understand
subtle computer science principles but instead want PHP to be
customised for your own way of working.

If you're installing Composer through an RPM and it's requiring you to
install lots of dependencies, something has gone horribly wrong.
Composer is a standalone phar executable which just needs a normal PHP
environment.

I'd strongly recommend having another go at using it, and in general
opening yourself up to new ways of doing things rather than being rude
to people who don't share your way of working.

cheers
Dan

lists@rhsoft.net

9 years ago

Am 26.05.2017 um 16:26 schrieb Dan Ackroyd:

> On 26 May 2017 at 13:23, lists@rhsoft.net <lists@rhsoft.net> wrote:
>> does that also fix the issue https://bugs.php.net/bug.php?id=74394
>
>
> Dear Anonymous,
>
> That "issue" is actually 3 issues.
>
> case 1
>
> class A {
>    public function test($a) {}
> }
> class B extends A {
>    public function test(string $a) { }
> }
>
> This breaks LSP - because B::test doesn't accept all the things that
> A::test can accept and so is unlikely to ever be supported.

and how is that a problem?

A accepts anything
B limits it's inut to a *subset* of "anything"

> case 2
>
> class A {
>    public function test($a) {}
> }
>
> class B extends A {
>    public function test($a): string {}
> }
>
> This works since return types were introduced.

but is not helpful when you introduce typehints and have to change *all*
consumers before and at least it should be *consistent* while param
typehints trigger warnings and return-types fatal errors

> case 3
>
> class A {
>   public function test($a): string { }
> }
>
> class B extends A {
>   public function test($a) {}
> }
>
> This breaks LSP - anything that is calling A::test would only expect a
> string to be returned, but B::test can return anything so is unlikely
> to ever be supported.

and how is that a practical problem?

if i add a return-type to A it is *again a subset* of "anything" and can
be no problem for the extending class and as a cosumer of B i do not
need anything to know about the parent method

> Your tone of words is quite rude and dismissive of other people both
> on this list and in the bug reports. That's really not a good way to
> persuade people, but particularly so when you seem to not understand
> subtle computer science principles but instead want PHP to be
> customised for your own way of working.

i understand more than you imagine

> If you're installing Composer through an RPM and it's requiring you to
> install lots of dependencies, something has gone horribly wrong.
> Composer is a standalone phar executable which just needs a normal PHP
> environment.

you don't understand what i talk about - when i maintain the complete
webstack with own packages *no* distribution package ever will work with
php subpackage dependencies

> I'd strongly recommend having another go at using it, and in general
> opening yourself up to new ways of doing things rather than being rude
> to people who don't share your way of working

i have no need for composer and i just pointed out that people thinking
"hey there is a package manager named composer and that's the new way
everyting works" are just plain wrong

PHP is a programming language and the same way as i *never* compromise
my environment with perl CPAN stuff or "pip" i won't for PHP

people these days seem to think having a ton of package managers
parallel and going back to days with static linking by using
flatpak/snap is a long term solution - it is not - but that same people
also don't maintain servers without reinstall over decades or maintain a
whole application stack for more the a decade without throwing it all
alway and start from scratch every few years

Nikita Popov

9 years ago

On Tue, May 23, 2017 at 9:25 PM, Rasmus Schultz <rasmus@mindplay.dk> wrote:

> This parameter type widening RFC, I didn't know about, but I have a remark.
>
> The feature, as implemented, will allow accidental omission of type-hints,
> will it not?
>
> Previously, the implements keyword guaranteed a correctly type-hinted
> implementation - it's now possible to (purposefully, obviously, in certain
> rare cases) omit type-hints by accident, making the implements keyword much
> less (or not any) of a guarantee that the interface is implemented
> correctly.
>
> The addition of an explicit "mixed" or "any" pseudo-type would have made
> this possible, without losing the guarantee that the implements keyword
> used to provide - that is, it would have been possible to have this feature
> for the few cases where it's useful, without affecting safety in the
> majority of other cases where it's not.
>
> I feel like this feature takes a pretty dangerous shortcut by simply
> removing a constraint check that we used to have - in favor of supporting a
> few rare cases, we removed a guarantee that interfaces and the
> implements-keyword has always provided.
>
> Those rare cases could have been supported in a safe manner by introducing
> a "mixed" or "any" type, which would have made the use of this feature
> explicit - which would have removed the risk of accidental omission of
> type-hints in the majority of cases.
>

One of the primary motivations behind this RFC is to allow the addition of
typehints (for example in library code) without introducing a backwards
compatibility break for any code implementing or extending your
interface/class. Requiring an explicit "mixed" or "any" type would not
allow this. I think this is 95% of the reason for this RFC, so without it,
the change itself is probably not worthwhile.

> The RFC page doesn't link to any discussion, and the Github thread was shut
> down after some negative remarks.

> I didn't see a discussion or a vote here on internals - did I miss
> something? Where or how did this get discussed and passed? Are these
> discussions happening somewhere else besides internals now?
>

The RFC was discussed in http://markmail.org/message/2oydyyl45v4korau. You
participated in that discussion.

The vote started in http://markmail.org/message/jgn4hwcgeezzk22w.

Both of those were found by searching for the literal RFC name.

Nikita

Fleshgrinder

9 years ago

On 5/23/2017 9:51 PM, Nikita Popov wrote:

> One of the primary motivations behind this RFC is to allow the addition of
> typehints (for example in library code) without introducing a backwards
> compatibility break for any code implementing or extending your
> interface/class. Requiring an explicit "mixed" or "any" type would not
> allow this. I think this is 95% of the reason for this RFC, so without it,
> the change itself is probably not worthwhile.
>

Yeah, I remember that and it is actually one of the very first sentences
in the RFC:

> Implementing this RFC would allow libraries to be upgraded to use
> type declarations in their method signatures. Currently adding types
> to a method of a class in a library would break any code that extends
> that class.
>
> --- https://wiki.php.net/rfc/parameter-no-type-variance

This was/is a great feature for library developers. We should probably
add Rasmus' concern to the list of things that can be added in PHP 8.
Another possible approach here could be to enforce an explicit "mixed"
if "strict_types" are active for the declaring class.

--
Richard "Fleshgrinder" Fussenegger

Niklas Keller

9 years ago

Hi Rasmus,

> This parameter type widening RFC, I didn't know about, but I have a remark.
>
> The feature, as implemented, will allow accidental omission of type-hints,
> will it not?
>

Yes, correct.

> Previously, the implements keyword guaranteed a correctly type-hinted
> implementation - it's now possible to (purposefully, obviously, in certain
> rare cases) omit type-hints by accident, making the implements keyword much
> less (or not any) of a guarantee that the interface is implemented
> correctly.
>

The interface will just work correctly, what's the actual problem you're
seeing?

> The addition of an explicit "mixed" or "any" pseudo-type would have made
> this possible, without losing the guarantee that the implements keyword
> used to provide - that is, it would have been possible to have this feature
> for the few cases where it's useful, without affecting safety in the
> majority of other cases where it's not.
>
> I feel like this feature takes a pretty dangerous shortcut by simply
> removing a constraint check that we used to have - in favor of supporting a
> few rare cases, we removed a guarantee that interfaces and the
> implements-keyword has always provided.
>
> Those rare cases could have been supported in a safe manner by introducing
> a "mixed" or "any" type, which would have made the use of this feature
> explicit - which would have removed the risk of accidental omission of
> type-hints in the majority of cases.
>

Unfortunately, that doesn't work for the second (maybe even primary) use
case of the RFC: Adding types to a class isn't a BC break anymore with the
RFC, as child classes will just widen the type and need explicit checks as
before. Without the RFC or with a version with "mixed" or "any", method
signatures will be incompatible again and thus be a BC break when adding
types to the base class.

"mixed" has been mentioned in the original discussion, but there was not a
lot of interest and mostly explicit disinterest.

> The RFC page doesn't link to any discussion, and the Github thread was shut
> down after some negative remarks.
>
> I didn't see a discussion or a vote here on internals - did I miss
> something? Where or how did this get discussed and passed? Are these
> discussions happening somewhere else besides internals now?

- Original discussion: https://externals.io/thread/505
- Vote announcement: https://externals.io/thread/613
- Vote resumption after internals having issues:
https://externals.io/thread/619

Regards, Niklas

Rasmus Schultz

9 years ago

> > The feature, as implemented, will allow accidental omission of

type-hints,

> > will it not?
>
> Yes, correct.

So this breaks the expected behavior of interfaces.

This will be harmful to everyone, but especially harmful to new
programmers, who may never even learn how to correctly implement an
interface, since now they don't have to.

Promising to address this in a future version isn't helpful, as that would
be a BC break, which rarely anyone condones, even for major releases -
changing it now isn't a BC break, so it's easy, but fixing it will be a BC
break, which will be very hard to push through.

Addressing this by allowing developers to opt-in to proper interface
validation in strict mode isn't helpful either, and especially not to new
programmers who are likely much farther from learning about strict mode
than they are from learning about basic things like properly implementing
an interface - and much, much further from learning about parameter
type-hint widening.

IMO, this is likely to become another one of those historically bad design
decisions that were made in spite of everything we know about other
programming languages :-(


On Tue, May 23, 2017 at 10:46 PM, Niklas Keller <me@kelunik.com> wrote:

Dan Ackroyd

9 years ago

On 25 May 2017 at 10:21, Rasmus Schultz <rasmus@mindplay.dk> wrote:

> So this breaks the expected behavior of interfaces.

No it doesn't. It allows interfaces to use contravariance within PHP's
type system, which is different from before, but it doesn't break
anything.

Niklas Keller wrote:

> The interface will just work correctly, what's the actual problem you're

seeing?

Rasmus, not answering people's questions and instead saying emotional
stuff suggesting other people have made historically bad decisions
isn't a productive way to conduct a conversation.

cheers
Dan

Rasmus Schultz

9 years ago

> > So this breaks the expected behavior of interfaces.
>
> No it doesn't. It allows interfaces to use contravariance within PHP's
> type system, which is different from before, but it doesn't break
> anything.

The RFC explicitly states that this is *not* contravariance.

And if it's not clear, what I mean by "breaks the expected behavior", is,
as far as I understand, what this RFC actually does is it *removes*
expected behavior.

Correct me if I'm wrong, but really all this RFC does, is remove the
requirement that and implementation of an interface be type-hinted?

To me, that's "broken".

You're favorizing a very, very small number of isolated, valid use-cases -
by removing the normal, natural, expected constraint that an implementation
be type-hinted as specified by the interface. It's part of the reason
interfaces even exist - and this simply ignores that and removes these
checks.

In my opinion, this will very likely lead to a large number of accidental
type-hint omissions - and a very, very small number of actually useful
solutions to real problems.

In my opinion, we can do much better than that, by adding a mixed
type-hint, which will ensure people aren't just accidentally omitting
type-hints and instead explicitly opting out of it to achieve parameter
widening. Solving this from the start also ensures we can have a clean
implementation of real contravariance in the future, without that being a
break change.

> Rasmus, not answering people's questions and instead saying emotional
> stuff suggesting other people have made historically bad decisions
> isn't a productive way to conduct a conversation.

Okay, I'm emotional, sorry about that.

But I think I'm pointing out a real problem and a real possible solution
that doesn't make the trade-offs of the current solution.

That is, I'm fine with introducing this simplified type of contravariance,
but I think it needs to be made explicit - simply omitting a type-hint does
not indicate the desired behavior; it doesn't indicate anything, and
thereby creates a new problem case of accidental type-hint omission.

Is there a very good reason we shouldn't address that, if it's at all
possible?


On Thu, May 25, 2017 at 12:32 PM, Dan Ackroyd <danack@basereality.com>
wrote:

Niklas Keller

9 years ago

2017-05-28 12:30 GMT+02:00 Rasmus Schultz <rasmus@mindplay.dk>:

> > > So this breaks the expected behavior of interfaces.
> >
> > No it doesn't. It allows interfaces to use contravariance within PHP's
> > type system, which is different from before, but it doesn't break
> > anything.
>
> The RFC explicitly states that this is *not* contravariance.
>

Where does it state that? (The "true" in the future scope section should
probably be replaced with "full".)

> And if it's not clear, what I mean by "breaks the expected behavior", is,
> as far as I understand, what this RFC actually does is it *removes*
> expected behavior.
>

How does this *remove* expected behavior? Could you show a concrete example
of code that breaks?

> Correct me if I'm wrong, but really all this RFC does, is remove the
> requirement that and implementation of an interface be type-hinted?
>

Correct, but not just for interfaces.

> To me, that's "broken".
>

As said, please show a concrete example that breaks.

> You're favorizing a very, very small number of isolated, valid use-cases -
> by removing the normal, natural, expected constraint that an implementation
> be type-hinted as specified by the interface. It's part of the reason
> interfaces even exist - and this simply ignores that and removes these
> checks.
>

No, interfaces exist to guarantee the caller can always pass a type XY
there. With type constraints removed in a subclass, this guarantee does not
break.

> In my opinion, this will very likely lead to a large number of accidental
> type-hint omissions - and a very, very small number of actually useful
> solutions to real problems.
>
> In my opinion, we can do much better than that, by adding a mixed
> type-hint, which will ensure people aren't just accidentally omitting
> type-hints and instead explicitly opting out of it to achieve parameter
> widening. Solving this from the start also ensures we can have a clean
> implementation of real contravariance in the future, without that being a
> break change.

You're, again, ignoring one of the major benefits of the RFC: Being able to
move from explicit type checks to type declarations without breaking
subclasses.

> > Rasmus, not answering people's questions and instead saying emotional
> > stuff suggesting other people have made historically bad decisions
> > isn't a productive way to conduct a conversation.
>
> Okay, I'm emotional, sorry about that.
>
> But I think I'm pointing out a real problem and a real possible solution
> that doesn't make the trade-offs of the current solution.
>

You haven't pointed out a real problem, yet.

Regards, Niklas

Rasmus Schultz

9 years ago

Alright, here's an example of code where a type-hint was accidentally left
out.

Before this change:

https://gist.github.com/mindplay-dk/e988902ed2d587dc05a1d8cb164b88c4/4d03babff16883a67accfb51b50e3d1aefb13a8a

And after this change:

https://gist.github.com/mindplay-dk/e988902ed2d587dc05a1d8cb164b88c4/03a4eec05b64185830785c5e684b3f94c1d62f5e

In other words, accidental omission of type-hints now means that this
faulty implementation is valid code.

And hopefully something will error, deep inside a call-stack, maybe in a
database-component that performs a type-check or something, but there is no
guarantee that it will error at all - if you passed an object that does
have a getID() method, because there's actually no type-check for the
Identity interface when calling the implementation, worst case, it actually
executes and quietly saves *something* to the database.

> No, interfaces exist to guarantee the caller can always pass a type XY

there.

> With type constraints removed in a subclass, this guarantee does not

break.

No, but you no longer get a type-check.

So yes, for *correct* code, there is no problem - but if you didn't write
the code correctly the first time, with this change, you may never even
know you've done something wrong, or you may have to drill through a much
larger call-stack and try to derive the source of the problem.

Before this change, a faulty implementation would simply get rejected.

To me, that's a setback that leads to harder-to-debug errors, and increases
learning-curve.

With an explicit mixed type-hint, you can still achieve what you want in
those rare cases where this simplified kind of contravariance is actually
useful, without also creating new problem cases for everyday coding.


On Sun, May 28, 2017 at 2:27 PM, Niklas Keller <me@kelunik.com> wrote:

Rowan Collins

9 years ago

On 28/05/2017 14:25, Rasmus Schultz wrote:

> With an explicit mixed type-hint, you can still achieve what you want

You are making a valid point in general, but you clearly haven't read or
understood the explanations you've been given why an explicit mixed
type-hint will NOT achieve what the authors of this RFC wanted.

If an existing class implements a library's interface, or extends a
library's base class, with no type hint, then the library currently has
to break both backward and forward compatibility to add type hints: the
user of the library will *have to* add the type hints to their
implementation, and once they do so, they will *have to* require that
minimum version of the library.

If the user of the library has to add "mixed" to all their method
signatures to use the now-type-hinted library, the library still has to
declare a breaking change, so that feature would not help this use case,
even if it would be better in others.

Regards,

--
Rowan Collins
[IMSoP]

Aidan Woods

9 years ago

So, if I understand everything here correctly

```
interface Foo
{
public function bar(Boo $Boo);
}
```

and

```
interface Foo
{
/*
* @param $Boo, pretty please accept type Boo here
*/
public function bar($Boo);
}
```

will be equivalent in 7.2? :(

Regards,
Aidan

On 28 May 2017 at 16:49, Rowan Collins <rowan.collins@gmail.com> wrote:

Dan Ackroyd

9 years ago

On 28 May 2017 at 18:06, Aidan Woods <aidantwoods@gmail.com> wrote:

> So, if I understand everything here correctly
> ...
> will be equivalent in 7.2? :(
>

Not equivalent. Adding the Boo param type to an implementation, when
it is only a comment in the parent gives an error.

interface Foo
{
/*
* @param $Boo, pretty please accept type Boo here
*/
public function bar( $Boo);
}

class Zot implements Foo {
public function bar(Boo $Boo) {}
}

Fatal error: Declaration of Zot::bar(Boo $Boo) must be compatible with
Foo::bar($Boo) in %s on line %d


cheers
Dan

Rowan Collins

9 years ago

On 28/05/2017 18:06, Aidan Woods wrote:

> So, if I understand everything here correctly
>
> ```
> interface Foo
> {
>    public function bar(Boo $Boo);
> }
> ```
>
> and
>
> ```
> interface Foo
> {
>    /*
>     * @param $Boo, pretty please accept type Boo here
>     */
>    public function bar($Boo);
> }
> ```
>
> will be equivalent in 7.2? :(

No, what's being proposed is that this will be allowed:

interface Foo
{
public function bar(Boo $Boo);
}

class Bar implements Foo
{
public function bar($Boo) { ... }
}

Whereas currently, the class would have to look like this:

class Bar implements Foo
{
public function bar(Boo $Boo) { ... }
}

An interface's job is to provide guarantees to calling code about where
and how an object can be used. It doesn't place any limits on what
*else* the class can do - class Bar might have extra methods, public
properties, be written in a completely different style, etc. Accepting
extra types *as well as* the one specified in the interface is in the
same category: the class is doing *more than* the interface, and that's
none of the interface's business.

I think some of the examples here are approaching interfaces as a
broader contract, that allows a library author to say "you must
implement the library this way". It's similar to the debate about
whether strict type hints should be the choice of the caller or the
callee. If you treat each module of the program as a black box, then all
that matters is the guarantees of what's acceptable, which type hints
and interfaces provide; but if you want more control over *how* somebody
meets those guarantees, then I can see that this RFC, and the
caller-selected type hint modes, would feel like a step in the wrong
direction.

Regards,

--
Rowan Collins
[IMSoP]

Dan Ackroyd

9 years ago

On 28 May 2017 at 14:25, Rasmus Schultz <rasmus@mindplay.dk> wrote:

>
> To me, that's a setback that leads to harder-to-debug errors, and increases
> learning-curve.

Everything is a trade-off. The type widening RFC makes it easier to
move to parameter types for existing libraries.

While I can see what you mean about how it could lead to more
accidental errors, for me at least, it would be actually quite hard to
not notice those errors in the code.

First, when people are programming against the interface rather than
the implementation, that type of mistake is much easier to spot.

function foo(Repository $repo) {
$repo->save(new stdClass());
// The above line is marked as an error in any decent IDE
// or code analysis tool.
}

Second, accidentally dropping the type from the parameter is also an
easy mistake to spot.

class UserRepository implements Repository {
public function save($item) {
// When inside this function the programmer has no idea
// what type item is. Any call to a method or property will
// be marked as a potential error either by an IDE, static
// analysis tool, or just the programmer realising
// "that parameter has no type".
}
}

So, to me (and possibly others who voted for the RFC) the ability to
add types to interfaces without breaking implementing library is a
good trade-off.

And yes, the type system in PHP could definitely be improved. Perhaps
you could propose an RFC? I suspect it might need to target PHP 8. I'm
personally hoping that some form of the union type RFC could return,
though possibly in a more general form.

cheers
Dan
Ack

Rasmus Schultz

9 years ago

> The type widening RFC makes it easier to move to parameter types for

existing libraries.

I'm curious to see how you'll set the version constraint in your
composer.json file.

After adding a scalar type-hint to an interface, which is a breaking change
in 7.0 and 7.1, but a non-breaking change in 7.2, is the new version of
your package a major or minor release?

If you release it as minor, you'll need a PHP version constraint like
"^5.5, ^7.2", but excluding 7.0-7.1 In that way is itself a breaking
change, so likely your only practical move is to release (and commit to
maintain!) both major and minor versions, which doesn't sound easy at all.


On May 29, 2017 00:53, "Dan Ackroyd" <danack@basereality.com> wrote:

Rowan Collins

9 years ago

On 30/05/2017 07:24, Rasmus Schultz wrote:

>> The type widening RFC makes it easier to move to parameter types for
> existing libraries.
>
> I'm curious to see how you'll set the version constraint in your
> composer.json file.
>
> After adding a scalar type-hint to an interface, which is a breaking change
> in 7.0 and 7.1, but a non-breaking change in 7.2, is the new version of
> your package a major or minor release?
>
> If you release it as minor, you'll need a PHP version constraint like
> "^5.5, ^7.2", but excluding 7.0-7.1 In that way is itself a breaking
> change, so likely your only practical move is to release (and commit to
> maintain!) both major and minor versions, which doesn't sound easy at all.

That's a good point, and reduces the immediate value of the change
slightly, but there is perhaps still value in making upgrading *easier*
on users.

For instance, you might want to use some other feature of PHP 7.2
anyway, so be willing to set that as your minimum version, but still not
want users to have to go through checking all their code for the correct
type hints. If they're already using 7.2, they might be able to migrate
to your new version without changing a single line of code.

Regards,

--
Rowan Collins
[IMSoP]

Christoph Becker

9 years ago

On 28.05.2017 at 12:30, Rasmus Schultz wrote:

> In my opinion, this will very likely lead to a large number of accidental
> type-hint omissions - and a very, very small number of actually useful
> solutions to real problems.
>
> In my opinion, we can do much better than that, by adding a mixed
> type-hint, which will ensure people aren't just accidentally omitting
> type-hints and instead explicitly opting out of it to achieve parameter
> widening. Solving this from the start also ensures we can have a clean
> implementation of real contravariance in the future, without that being a
> break change.

Something like that has already been suggested in the original RFC
discussion[1] but has been rejected, so I don't think it makes much
sense to repeat this discussion again.

[1] <http://news.php.net/php.internals/97106>

--
Christoph M. Becker

Levi Morrison

9 years ago

On Tue, May 23, 2017 at 1:25 PM, Rasmus Schultz <rasmus@mindplay.dk> wrote:

>
> I feel like this feature takes a pretty dangerous shortcut by simply
> removing a constraint check that we used to have - in favor of supporting a
> few rare cases, we removed a guarantee that interfaces and the
> implements-keyword has always provided.
>
> Those rare cases could have been supported in a safe manner by introducing
> a "mixed" or "any" type, which would have made the use of this feature
> explicit - which would have removed the risk of accidental omission of
> type-hints in the majority of cases.
>

This was my opinion at voting time which is why i voted against the RFC. It
passed 24 to 5 so it's just something we'll have to live with for now.

Filippo Tessarotto

9 years ago

> The feature, as implemented, will allow accidental omission of type-hints

Hi, stated the purpose of `declare(strict_types=1)` and that this is
used by developers and libraries that actually crave for strictness,
would it be votable an RFC that reintroduce the Warning only with this
declare active?

Filippo Tessarotto