fix: report private members of the bound test case in pest closures - #20
Open
MrPunyapal wants to merge 1 commit into
Open
MrPunyapal wants to merge 1 commit into
MrPunyapal wants to merge 1 commit into
Conversation
Pest binds a test closure to the generated test case, which only extends the class given to uses(). PHP private scope rules therefore put the private members of that class out of reach, and the plugin reports none of them. PHPStan registers the closure bind scope from getObjectClassNames() of the $this type, so the bound class counts as the closure's own scope and canAccessClassMember() lets its private members through. The correct scope would be a strict subclass, which does not exist during analysis, so this adds a rule that reports exactly the set PHPStan misses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
uses(SomeTestCase::class)binds that class to the generated test case, so Pest generates a class that extends it. A closure passed toit()is bound to the generated class, not toSomeTestCase, so PHP private scope rules put the private members ofSomeTestCaseout of reach:pest-plugin-phpstan reports nothing for either. pestphp/pest#1945 fixed the opposite case, where members of a trait bound through
uses()were reported even though they run fine.Why
PHPStan registers the closure bind scope from
getObjectClassNames()of the$thistype, inenterAnonymousFunction:TestClosureThisTypeExtensionreturnsObjectType(MyTestCase::class), so the bound class is registered as the closure's own scope.MutatingScope::canAccessClassMember()then grants a private member when its declaring class matches a bind scope class, so PHPStan stays silent.The generated class only extends
MyTestCase, so the correct bind scope would be a strict subclass. That class only exists once Pest has generated it at run time. Returning nothing fromgetObjectClassNames()does report the private members, but it reports protected ones too, and those do run fine.The change
BoundTestCasePrivateMemberRulereports the private members PHPStan misses. It fires when the declaring class is in$thisType->getObjectClassNames(), which is exactly the set PHPStan treats as the closure's own scope, so nothing is reported twice.PHPUnit\Framework\TestCase::runTest()uses()Identifiers are PHPStan's own,
method.private,staticMethod.privateandclassConstant.private, because these are the diagnostics PHPStan should have produced here, and a@phpstan-ignore method.privatea user already has keeps working.rector.phpgains a skip fortests/Type/Fixtures/PrivateMembers, alongside the existing fixture skips. Dead code detection cannot see the private members being called from analysed fixtures, andLocallyCalledStaticMethodToNonStaticRectorfights the static case, so the directory is skipped rather than working around either.Tests
tests/Rules/BoundTestCasePrivateMemberRuleTest.php, four tests. Every expression this rule reports was also run against Pest and fails at run time with the matching message, four on the bound test case and one on the default test case.Each part of the change is covered by a test that fails without it:
isInClass()guard removed$thisreceiver check removedgetObjectClassNames()check removedisPrivate()check removedIdentifiername check removedisInAnonymousFunction()removedTestCasesupertype check removed$thisas a non test case in a file level closure todayChecks
pest→ 524 passed, 640 assertionsphpstan analyse→ 0 errorsrector --dry-run→ 0 changed filespint --test→ passed for every file in this PR. Pint also reports 22 pre-existing files on5.xunchanged, allline_endingfrom a CRLF checkout, not touched here.Not covered
Private properties of the bound test case.
universalObjectCratesClassescoversPHPUnit\Framework\TestCaseincluding subclasses, so any property access on$thisis allowed and the type degrades tomixed. Reporting there is correct at run time, but it is a wider behaviour change than this PR takes on.A private member of a trait used by the bound test case is also still silent. Its declaring class is the trait, so this rule does not reach it, and PHPStan does not report trait privates at all. Run time fails when that trait sits on the bound class rather than the generated one.