Skip to content

Better return types for getBackingType #8687

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

Merged
merged 2 commits into from
Jun 2, 2022
Merged

Conversation

SamMousa
Copy link
Contributor

@SamMousa SamMousa commented Jun 2, 2022

The only backing types for Enums are int and string. The proper return type for ReflectionEnum::getBackingType() is thus null|ReflectionNamedType.

See also php/doc-en#1608

The only backing types for Enums are int and string. The proper return type for ReflectionEnum::getBackingType() is thus null|ReflectionNamedType.

See also php/doc-en#1608
@SamMousa
Copy link
Contributor Author

SamMousa commented Jun 2, 2022

Note that some changes to the C source might also be required to more formally guarantee this stricter contract. Unfortunately my C and my php source knowledge is not fluent enough to propose those changes.

I think we need to define a new return type somewhere:

ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_class_ReflectionEnum_getBackingType, 0, 0, ReflectionNamedType, 1)
ZEND_END_ARG_INFO()

@SamMousa SamMousa changed the base branch from master to PHP-8.1.6 June 2, 2022 11:05
@SamMousa SamMousa changed the base branch from PHP-8.1.6 to master June 2, 2022 11:06
@cmb69
Copy link
Member

cmb69 commented Jun 2, 2022

It seems to me that restricting the return type might not work well with tagged unions or other extensions to the current enumerations.

@iluuu1994, @Crell, thoughts?

@iluuu1994
Copy link
Member

Tagged unions can't be backed so that's not a concern. The type would change if we supported more types as backing types but I'm not sure if that's likely to happen. I'm ok with this change, an assert in ZEND_METHOD(ReflectionEnum, getBackingType) might make sense.

@SamMousa
Copy link
Contributor Author

SamMousa commented Jun 2, 2022

I'm ok with this change, an assert in ZEND_METHOD(ReflectionEnum, getBackingType) might make sense.

While I think the assert would go between lines 6869 and 6870 I'm not actually sure what it would look like. (C is not a language I use)

ZEND_ASSERT(ce->enum_backing_type != ...)

Would you mind proposing the code change?

Edit: as far as I can tell the pipeline failing is a CI problem, not a code problem.

@iluuu1994
Copy link
Member

@SamMousa Something like ZEND_ASSERT(Z_OBJCE_P(return_value) == reflection_named_type_ptr); right after the reflection_type_factory to ensure the value is actually of that type. But it's actually fine, we do return type checks for internal functions in debug builds.

@iluuu1994 iluuu1994 merged commit 2ce2aff into php:master Jun 2, 2022
@SamMousa SamMousa deleted the patch-2 branch June 2, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants