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

Allow asserting array offset certainty #3503

Merged

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Sep 27, 2024

This makes it possible to do this:

/**
 * @param array{firstName: string, lastName?: string} $context
 */
public function arrayOffset(array $context) : void
{
    assertVariableCertainty(TrinaryLogic::createYes(), $context['firstName']);
    assertVariableCertainty(TrinaryLogic::createMaybe(), $context['lastName']);
    assertVariableCertainty(TrinaryLogic::createNo(), $context['email']);
}

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 2.0.x. PHPStan 2.0 is not going to be released for months. If your code is relevant on 1.12.x and you want it to be released sooner, please rebase your pull request and change its target to 1.12.x.

@ruudk ruudk changed the base branch from 2.0.x to 1.12.x September 27, 2024 10:24
@ruudk ruudk force-pushed the assertVariableCertaintyOnArrayDimFetch branch 3 times, most recently from 76f24ad to d92c523 Compare September 27, 2024 10:36
@ruudk ruudk force-pushed the assertVariableCertaintyOnArrayDimFetch branch from d92c523 to 47fbf55 Compare September 27, 2024 11:09
@ruudk ruudk requested a review from ondrejmirtes September 27, 2024 13:32
@staabm
Copy link
Contributor

staabm commented Sep 27, 2024

nice one!

@ruudk ruudk force-pushed the assertVariableCertaintyOnArrayDimFetch branch 4 times, most recently from f8ac7b6 to af44d78 Compare September 30, 2024 11:59
@ruudk
Copy link
Contributor Author

ruudk commented Sep 30, 2024

@ondrejmirtes I pushed another commit to add assertions on the errors. Is this what you had in mind?

@ruudk
Copy link
Contributor Author

ruudk commented Oct 1, 2024

@ondrejmirtes Would be great to have this part of the next release 🙏. Does this need more testing?

* @dataProvider dataFile
*/
public function testFile(string $file): void
public function testFile(string $file, array $expectedFailures = []): void
Copy link
Member

@ondrejmirtes ondrejmirtes Oct 1, 2024

Choose a reason for hiding this comment

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

Don't change this signature. Don't change the many yield lines. It doesn't make sense to expect failures outside of testing the actual implementation.

There's already TypeInferenceTestCaseTest. Test it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the last part of your comment was not stored...

Copy link
Member

Choose a reason for hiding this comment

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

I meant to delete that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ondrejmirtes Done, I hope this is what you had in mind.

@ruudk ruudk force-pushed the assertVariableCertaintyOnArrayDimFetch branch 2 times, most recently from 5a8c907 to 351ee4a Compare October 1, 2024 16:55
@ruudk ruudk requested a review from ondrejmirtes October 1, 2024 16:58
Comment on lines +100 to +101
$this->assertSame('variable $context', $variableAssert[4]);
$this->assertSame("offset 'email'", $offsetAssert[4]);
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 did it this way, because the rest in this test did not actually ran the assertions, it only waited for thrown acceptors. See line 91 above.

So if you think we should actually call the asserts, please tell me how, and I'll make the change.

Then we can assert on the full error message.

@ruudk
Copy link
Contributor Author

ruudk commented Oct 2, 2024

@ondrejmirtes I think this one is now ready to go. The failing tests seem not related?

@ruudk ruudk force-pushed the assertVariableCertaintyOnArrayDimFetch branch from 351ee4a to a547291 Compare October 3, 2024 12:22
@ondrejmirtes ondrejmirtes merged commit dbe8b74 into phpstan:1.12.x Oct 4, 2024
473 of 499 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@ruudk ruudk deleted the assertVariableCertaintyOnArrayDimFetch branch October 4, 2024 08:51
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.

None yet

4 participants