Skip to content
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

PHP 8.2: Support for disjunctive normal form types #1198

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

kukulich
Copy link
Collaborator

@kukulich kukulich commented Sep 8, 2022

https://wiki.php.net/rfc/dnf_types

#1197 needs to be merged first.

@Ocramius Ocramius added this to the 6.0.0 milestone Sep 10, 2022
@Ocramius Ocramius changed the base branch from 5.10.x to 6.0.x September 10, 2022 17:54
@kukulich kukulich force-pushed the dnf-types branch 2 times, most recently from 30b321b to 622800c Compare September 13, 2022 09:14
@kukulich kukulich marked this pull request as ready for review September 13, 2022 09:16
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

I'm a bit confused about rolling back union types referencing concrete types, and going back to base ReflectionType usage 🤔

src/Reflection/Adapter/ReflectionType.php Outdated Show resolved Hide resolved
array_map(static fn (BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType $type): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null => ReflectionType::fromTypeOrNull($type), $this->betterReflectionType->getTypes()),
static fn (ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $type): bool => $type instanceof ReflectionNamedType,
);
return array_map(static function (BetterReflectionType $type): ReflectionNamedType {
Copy link
Member

Choose a reason for hiding this comment

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

BetterReflectionType is not final: IMO, the union type should be preserved 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to BetterReflectionNamedType because ReflectionIntersectionType::getTypes() returns list<ReflectionNamedType>

Copy link
Member

Choose a reason for hiding this comment

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

I see 🤔

Need to reason about this a bit, 'bit slow after processing a few hundred mails, sorry.

@kukulich kukulich force-pushed the dnf-types branch 2 times, most recently from 94b739d to cb56306 Compare September 13, 2022 09:55
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Gave it another read - found some types that we can refine a bit 👍

src/Reflection/Adapter/ReflectionIntersectionType.php Outdated Show resolved Hide resolved
use function count;

abstract class ReflectionType extends CoreReflectionType
{
public static function fromTypeOrNull(BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType|null $betterReflectionType): ReflectionUnionType|ReflectionNamedType|ReflectionIntersectionType|null
public static function fromType(BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType|null $betterReflectionType): ReflectionUnionType|ReflectionNamedType|ReflectionIntersectionType
Copy link
Member

Choose a reason for hiding this comment

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

According to infection, this method is not used in a public scope? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's used in ReflectionIntersectionType and ReflectionUnionType adapters so marked as @internal

src/Reflection/Adapter/ReflectionUnionType.php Outdated Show resolved Hide resolved
Comment on lines +445 to +447
if (! $innerType instanceof ReflectionNamedType) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks quite broken: perhaps we should drop this method completely from src/Reflection, and only keep it on adapters? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's good idea, the method is marked as deprecated in PHP. However I prefer to do it in separate PR because the method is used in some unit tests.

src/Reflection/ReflectionUnionType.php Outdated Show resolved Hide resolved
stubs/ReflectionIntersectionType.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Sorry for not merging yet: this is really messy PHP stuff, and I've been through LOADS of meetings today @_@

@kukulich
Copy link
Collaborator Author

Sorry for not merging yet: this is really messy PHP stuff, and I've been through LOADS of meetings today @_@

No problem :)

@kukulich kukulich requested a review from Ocramius September 13, 2022 17:21
use function count;

abstract class ReflectionType extends CoreReflectionType
{
public static function fromTypeOrNull(BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType|null $betterReflectionType): ReflectionUnionType|ReflectionNamedType|ReflectionIntersectionType|null
/** @internal */
public static function fromType(BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType|null $betterReflectionType): ReflectionUnionType|ReflectionNamedType|ReflectionIntersectionType
Copy link
Member

Choose a reason for hiding this comment

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

How do we kill this mutant, before merging? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added tests but the mutant is still there 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Let's roll with the current solution for now 👍

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM! Mutation is sub-optimal, but I don't want to waste your time any further :)

@Ocramius Ocramius self-assigned this Sep 14, 2022
@Ocramius Ocramius merged commit acfee0d into Roave:6.0.x Sep 14, 2022
@kukulich kukulich deleted the dnf-types branch September 30, 2022 15:20
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.

2 participants