Skip to content

Implement Parameter Type Widening RFC #2265

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.

Sign up for GitHub

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
kelunik wants to merge 1 commit into from
Closed

Implement Parameter Type Widening RFC #2265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Zend/tests/objects_009.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,5 @@ class test3 extends test {

echo "Done\n";
?>
--EXPECTF--
Warning: Declaration of test3::foo($arg) should be compatible with test::foo(Test $arg) in %s on line %d
--EXPECT--
Done
34 changes: 34 additions & 0 deletions Zend/tests/type_declarations/parameter_type_variance.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
--TEST--
Parameter variance with no type
--FILE--
<?php

class Foo {
function testParentClass(Foo $foo) {}
function testBothClass(Foo $foo) {}
function testChildClass($foo) {}
function testNoneClass($foo) {}

function testParentBuiltin(int $foo) {}
function testBothBuiltin(int $foo) {}
function testChildBuiltin($foo) {}
function testNoneBuiltin($foo) {}
}

class Bar extends Foo {
function testParentClass($foo) {}
function testBothClass(Foo $foo) {}
function testChildClass(Foo $foo) {}
function testNoneClass($foo) {}

function testParentBuiltin($foo) {}
function testBothBuiltin(int $foo) {}
function testChildBuiltin(int $foo) {}
function testNoneBuiltin($foo) {}
}

?>
--EXPECTF--
Warning: Declaration of Bar::testChildClass(Foo $foo) should be compatible with Foo::testChildClass($foo) in %s on line %d

Warning: Declaration of Bar::testChildBuiltin(int $foo) should be compatible with Foo::testChildBuiltin($foo) in %s on line %d
39 changes: 26 additions & 13 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,23 +172,20 @@ static zend_always_inline zend_bool zend_iterable_compatibility_check(zend_arg_i
if (ZEND_TYPE_CODE(arg_info->type) == IS_ARRAY) {
return 1;
}

if (ZEND_TYPE_IS_CLASS(arg_info->type) && zend_string_equals_literal_ci(ZEND_TYPE_NAME(arg_info->type), "Traversable")) {
return 1;
}

return 0;
}
/* }}} */

static int zend_do_perform_type_hint_check(const zend_function *fe, zend_arg_info *fe_arg_info, const zend_function *proto, zend_arg_info *proto_arg_info) /* {{{ */
{
if (ZEND_LOG_XOR(ZEND_TYPE_IS_CLASS(fe_arg_info->type), ZEND_TYPE_IS_CLASS(proto_arg_info->type))) {
/* 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(proto_arg_info->type));

if (ZEND_TYPE_IS_CLASS(fe_arg_info->type)) {
if (ZEND_TYPE_IS_CLASS(fe_arg_info->type) && ZEND_TYPE_IS_CLASS(proto_arg_info->type)) {
zend_string *fe_class_name, *proto_class_name;
const char *class_name;

Expand Down Expand Up @@ -237,14 +234,30 @@ static int zend_do_perform_type_hint_check(const zend_function *fe, zend_arg_inf
zend_string_release(proto_class_name);
zend_string_release(fe_class_name);
} else if (ZEND_TYPE_CODE(fe_arg_info->type) != ZEND_TYPE_CODE(proto_arg_info->type)) {
/* Incompatible type */
/* Incompatible built-in types */
return 0;
}

return 1;
}
/* }}} */

static int zend_do_perform_arg_type_hint_check(const zend_function *fe, zend_arg_info *fe_arg_info, const zend_function *proto, zend_arg_info *proto_arg_info) /* {{{ */
{
if (!ZEND_TYPE_IS_SET(fe_arg_info->type)) {
/* Child with no type is always compatible */
return 1;
}

if (!ZEND_TYPE_IS_SET(proto_arg_info->type)) {
/* Child defines a type, but parent doesn't, violates LSP */
Copy link
Copy Markdown
Member

@nikic nikic Jan 15, 2017

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.

Copy link
Copy Markdown
Member Author

@kelunik kelunik Jan 15, 2017

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.

Copy link
Copy Markdown

@guilhermeblanco guilhermeblanco Jan 16, 2017 •
edited
Loading

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.

Copy link
Copy Markdown
Member Author

@kelunik kelunik Jan 16, 2017

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.

return 0;
}

return zend_do_perform_type_hint_check(fe, fe_arg_info, proto, proto_arg_info);
}
/* }}} */

static zend_bool zend_do_perform_implementation_check(const zend_function *fe, const zend_function *proto) /* {{{ */
{
uint32_t i, num_args;
Expand Down Expand Up @@ -312,15 +325,15 @@ static zend_bool zend_do_perform_implementation_check(const zend_function *fe, c
} else {
proto_arg_info = &proto->common.arg_info[proto->common.num_args];
}
if (!zend_do_perform_type_hint_check(fe, fe_arg_info, proto, proto_arg_info)) {

if (!zend_do_perform_arg_type_hint_check(fe, fe_arg_info, proto, proto_arg_info)) {
switch (ZEND_TYPE_CODE(fe_arg_info->type)) {
case IS_ITERABLE:
if (!zend_iterable_compatibility_check(proto_arg_info)) {
return 0;
}
break;

default:
return 0;
}
Expand All @@ -345,15 +358,15 @@ static zend_bool zend_do_perform_implementation_check(const zend_function *fe, c
if (!(fe->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE)) {
return 0;
}

if (!zend_do_perform_type_hint_check(fe, fe->common.arg_info - 1, proto, proto->common.arg_info - 1)) {
switch (ZEND_TYPE_CODE(proto->common.arg_info[-1].type)) {
case IS_ITERABLE:
if (!zend_iterable_compatibility_check(fe->common.arg_info - 1)) {
return 0;
}
break;

default:
return 0;
}
Expand Down
14 changes: 0 additions & 14 deletions tests/classes/type_hinting_005b.phpt
View file
Open in desktop

This file was deleted.