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
Implement Parameter Type Widening RFC #2265
Conversation
a9ed7c8
to
a7a682a
Compare
Comment on behalf of krakjoe at php.net: labelling |
16d2400
to
6dce990
Compare
430f972
to
2e85f21
Compare
Rebase onto current master with the type changes, the test failure is unrelated. |
Of course, send SOLID to hell. Who is Barbara Liskov??? Some mad woman? Of course! Let's allow to break this principles and ideas! |
} | ||
|
||
if (!ZEND_TYPE_IS_SET(proto_arg_info->type)) { | ||
/* Child defines a type, but parent doesn't, violates LSP */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more intuitive to move this these checks into the argument checking code, rather than have it in the code that is shared between argument and return types. It works either way because return type checking only invokes the function if both types are set, but it's a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno, it's the place where it was before, so I left it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires decoupling into argument check and return type check, otherwise we'll be allowing widening at return types (basically adding contra-variance support), which is not desired.
Return types only support co-variance.
Argument types only support contra-variance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add test cases to check that it doesn't allow contravariant return types and modify as needed.
@skie If you're wondering why everyone is confused about your comment: The Liskov substitution principle explicitly allows contravariance on method argument types, which is what is being implemented here. The previous implementation was too restrictive, forbidding some cases that were perfectly valid under LSP. |
@nikic sure, contravariance must be allowed. But how did complete removing of type hint became possible and accepted? |
@kelunik your arguments in rfc are totally mad. Enforcing type hint on method signature is breaking change and library must define new major version in this case. Depended libraries must upgrade extending/usages code and then define new self major version. That's how it must work in semver with bc-compability in mind. |
@b1rdex No, it's not a breaking change if your code documented the accepted types before and did these checks manually. |
Changing signature is always breaking change.
…On Jan 16, 2017 17:09, "Niklas Keller" ***@***.***> wrote:
@b1rdex <https://github.com/b1rdex> No, it's not a breaking change if
your code documented the accepted types before and did these checks
manually.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2265 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AATGF500dHYhFD6SB0IzsnR583CUoJ_pks5rSxe1gaJpZM4LYtl->
.
|
Please explain where it breaks. |
@b1rdex It may help you to think of removing a type as specifying a (non-existing) |
@nikic good catch. For anyone interested: following snippet is valid in Java public class Foo {
public Foo testParentClass(int $foo) {return new Foo();}
}
public class Bar extends Foo {
public int testParentClass(String $foo) {return 2;}
} |
in php changing type hints on public api will break extenders before this got merged :) |
@b1rdex The Java case is somewhat different, because it's not overriding the method, but adding an overload with a different signature. In languages with method overloading variance considerations usually only come in for generic parameters and return types. |
@nikic shame on me, I totally forgot about polymorphism… Hope php will see it someday. |
@b1rdex I don't think method overloading is something we'll see in PHP, at least not in the near future. It's rather complicated with weak types and comes with significant overhead IIRC.
|
2e85f21
to
5874be7
Compare
`abstract class Person
} class Foo class Human extends Person $foo = new Foo(); What about this? |
@artemzakholodilo This PR won't affect your example;
|
5874be7
to
c419e8f
Compare
@guilhermeblanco @nikic I've improved the implementation to use a separate function for args that does the additional checks. I've added an assertion to the current function, so it's only called if both types are set. |
Zend/zend_inheritance.c
Outdated
if (!ZEND_TYPE_IS_SET(proto_arg_info->type)) { | ||
/* Child defines a type, but parent doesn't, violates LSP */ | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Some space (instead of tab) indents in the above blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I put them into a separate commit? The rest of the file already has trimmed whitespace on empty lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is newly introduced code, it should be in the same commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, misunderstanding. I was referring to use of 4-space indentation instead of tabs in the lines above, not the the removal of trailing whitespace, that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, fixed. I didn't fix the other places using spaces that are not new code.
c419e8f
to
2da1079
Compare
Zend/zend_inheritance.c
Outdated
/* Only one has a type declaration and the other one doesn't */ | ||
return 0; | ||
} | ||
ZEND_ASSERT(ZEND_TYPE_IS_SET(fe_arg_info->type) && ZEND_TYPE_IS_SET(fe_arg_info->type)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be proto_arg_info->type
once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
2da1079
to
6a95578
Compare
Merged via 2edc3cf with an UPGRADING note. Thanks! |
Are you mad ? Why do you want to violate SOLID and Liskus Substitution like that ? Do you really think that kind of thing will improve the reputation of PHP ? Shame. |
@JuJuDropThor This does not violate the Liskov substitution principle (LSP). |
So could you tell me, please, how that could be possible if you change declaration of your method between a type and a subtype ? |
Sure, it's called contravariance for method argument types: https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)#Contravariant_method_argument_type |
Oh array covariance, great... You have an array parameter: In the subclass, you could have... everything you want. Your subclass EverythingClass can't be replaced by an object of type ArrayClass without violating LSP. First line of the wiki you linked: Well, do as you wish, that RFC is validated anyway. That kind of thing doesn't honor PHP repuration. |
In this case the contravariance is not the example you mentioned but exactly the other way around. Assuming your EverythngClass, ArrayClass example, and that ArrayClass is the parent and EverythingClass is the child, EverythingClass can replace ArrayClass without breaking LSP. In the example you are giving, you are trying to use the parent to replace the child which is not valid. |
Right, an |
See https://wiki.php.net/rfc/parameter-no-type-variance.