Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 11, 2016

This extracts most of the logic for the formatting of type errors into a single function, instead of repeating part of it all over the place.

/cc @TazeTSchnitzel

@nikic
Copy link
Member Author

nikic commented Aug 11, 2016

I've extended this a bit and deduplicated the type checking code for non-internal argument/return types.

return 1;
}

if (Z_TYPE_P(arg) == IS_NULL && (arg_info->allow_null || (default_value && is_null_constant(zf->common.scope, default_value)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check if the default value is null here? Don't we always set allow_null in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, my "or null" code needs changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

This handles cases like the following:

function test(Foo $bar = ABC) {}
define('ABC', null);
test(null);

Here we cannot determine at compile-time that the default value is actually null.

So yeah, at least theoretically the "or null" handling would have to be adjusted for this. However, I feel like we should instead drop this special case altogether. IIRC it was introduced as a bug "fix" sometime in 5.6 without anyone properly evaluating the consequences. For example, this means that allowsNull() in reflection lies. It also means that our LSP checks do not work correctly. Now that PHP 7.1 has an explicit way to mark that argument as nullable using ?Foo $bar = ABC, this should be dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, that makes sense. I agree, dropping it would make sense.

@nikic nikic force-pushed the typeErrorCleanup branch from cc92148 to cbb8244 Compare August 16, 2016 12:00
@nikic
Copy link
Member Author

nikic commented Aug 16, 2016

Merged via ba09a52.

@nikic nikic closed this Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants