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

Union of enum cases accepts the enum class type with negated cases #2418

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

takaram
Copy link
Contributor

@takaram takaram commented May 27, 2023

This PR changes the result of removing an enum case from its class.

enum TestEnum
{
    case A;
    case B;
    case C;
}

function test(TestEnum $enum): void
{
    if ($enum !== TestEnum::A) {
        \PHPStan\dumpType($enum);
    }
}
  • Before: TestEnum~TestEnum::A
  • After: TestEnum::B|TestEnum::C

Closes phpstan/phpstan#8846

@takaram takaram marked this pull request as draft May 27, 2023 09:20
@ondrejmirtes
Copy link
Member

Instead of changing what TypeCombinator::remove() does in this case, I'd rather be if you fixed so that Foo::THREE|Foo::TWO accepts Foo~Foo::ONE. You should be able to debug that by looking into what UnionType::accepts() does with ObjectType in this case. Some code that does $type->getEnumCases() and compares them (because both sides will report the same enum cases) should be put somewhere in there.

@takaram
Copy link
Contributor Author

takaram commented May 30, 2023

@ondrejmirtes
Thank you for your suggestion. I'll revise this PR.

@takaram takaram changed the title Removing a case from enum class type results into union of rest cases Union of enum cases accepts the enum class type with negated cases Jun 5, 2023
@takaram takaram marked this pull request as ready for review June 5, 2023 11:35
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@@ -1186,6 +1188,72 @@ public function dataAccepts(): array
],

];

if (PHP_VERSION_ID < 80100) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit tricky to mark the rest of the dataProvider as tested only on PHP 8.1. Please reverse the condition and put the enum yields into that condition:

if (PHP_VERSION_ID >= 80100) {
    yield...
}

If PHPCS complains, move one existing yield below the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reversed the condition and moved some of the existing yields.

]),
new ObjectType(
'PHPStan\Fixture\TestEnum',
new EnumCaseObjectType('PHPStan\Fixture\TestEnum', 'TWO'),
Copy link
Member

Choose a reason for hiding this comment

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

One more test please:

		yield [
			new UnionType([
				new EnumCaseObjectType('PHPStan\Fixture\TestEnum', 'ONE'),
				new NullType(),
			]),
			new UnionType([
				new ObjectType(
					'PHPStan\Fixture\TestEnum',
					new EnumCaseObjectType('PHPStan\Fixture\TestEnum', 'TWO'),
				),
				new NullType(),
			]),
			TrinaryLogic::createYes(),
		);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this test case.

@takaram
Copy link
Contributor Author

takaram commented Jun 5, 2023

@ondrejmirtes
Thank you for your comments.
Could you check it again?

@ondrejmirtes
Copy link
Member

Alright, one more thing: you're trying to fix phpstan/phpstan#8846. Which means we need a regression test in the test class for the rule that reports the false positive. Which means you should write a test in Functions\ReturnTypeRuleTest. And make sure it fails without your changes in src/ :)

@ondrejmirtes ondrejmirtes merged commit e7eb6eb into phpstan:1.10.x Jun 8, 2023
376 of 379 checks passed
@ondrejmirtes
Copy link
Member

Perfect, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants