Report new/function/static calls with empty-bodied constructors and callables even when their source is not analysed#5796
Closed
phpstan-bot wants to merge 1 commit into
Conversation
… callables even when their source is not analysed - The `*WithoutImpurePointsCollector` collectors only run for code that is part of the analysed file set (they react to `MethodReturnStatementsNode`/`FunctionReturnStatementsNode`). A standalone `new \PHPStan\Type\StringType()` whose source comes from a dependency was therefore never reported by `CallToConstructorStatementWithoutImpurePointsRule`, even though its constructor is empty and has no side effects. - Added `EmptyBodyCallableDetector` which parses the declaring file and reports whether a constructor/function/method has an empty body (no statements, no promoted properties, no by-ref params). An empty body provably has no impure points and no throw points. It parses with `currentPhpVersionRichParser` because the default analysis parser strips bodies of non-analysed files, which would make every non-analysed callable look empty. - `PossiblyPureNewCollector`, `PossiblyPureFuncCallCollector` and `PossiblyPureStaticCallCollector` now flag such empty-bodied callables, and the corresponding rules report them even when the analysis-based collector never saw them. - Swept the parallel concrete-call constructs: `new` (the reported case), plain function calls, and static method calls. Excluded: late-static-bound `static::` calls (polymorphic), `parent::__construct()`/constructors in the static path (matching `MethodWithoutImpurePointsCollector`), and built-in classes/functions (reflected from stubs with empty bodies that do not reflect reality, e.g. `new DateTime()`), plus callables carrying `@phpstan-assert` tags. - Instance method calls (`$foo->bar()`) are intentionally left untouched: the runtime object may be a subclass overriding the method, so an empty body in the declared class does not prove the call has no effect.
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.
Summary
Doing
new \PHPStan\Type\StringType();on a separate line was not reported as having no effect, even thoughStringType's constructor is empty. The same was true for plain function calls and static method calls to empty-bodied callables — whenever the callable's source was not part of the analysed file set (e.g. it comes from a third-party dependency or, as in the linked playground, from PHPStan's own classes that the playground does not analyse).The reason is that the
*WithoutImpurePointsCollectorcollectors that feed these rules only run for code that is actually analysed (they react toMethodReturnStatementsNode/FunctionReturnStatementsNode). For a class/function coming from a dependency those nodes are never emitted, so the inferred-purity ("maybe pure") path had no data and the call was silently accepted.This PR closes that gap for concrete calls by determining empty-bodied-ness directly from the callable's source file.
Changes
src/Rules/DeadCode/EmptyBodyCallableDetector.phpservice: parses the declaring file and answers whether a function or method has an empty body — no statements, no promoted-property parameters, and no by-reference parameters. An empty body provably has no impure points and no throw points. It usescurrentPhpVersionRichParser(not the default analysis parser, which strips bodies of non-analysed files and would make every non-analysed callable look empty) and caches parsed files in memory.src/Rules/DeadCode/PossiblyPureNewCollector.php: emits a "has empty constructor body" flag (skipping built-in classes and constructors carrying@phpstan-asserttags).src/Rules/DeadCode/PossiblyPureFuncCallCollector.php: same flag for functions (skipping built-in functions / asserting functions).src/Rules/DeadCode/PossiblyPureStaticCallCollector.php: same flag for static calls, additionally excluding late-static-boundstatic::calls (polymorphic),__construct(i.e.parent::__construct(), mirroringMethodWithoutImpurePointsCollector's constructor exclusion), built-in classes and asserting methods. Now also carries the declaring class display name for the message.CallToConstructorStatementWithoutImpurePointsRule,CallToFunctionStatementWithoutImpurePointsRule,CallToStaticMethodStatementWithoutImpurePointsRule: report the call when the new flag is set, in addition to the existing analysis-based path.Root cause
The dead-code "no effect" detection for inferred (
maybe) purity is built entirely on collectors that only fire for analysed code. Constructors are the only callables whose empty body coincides withmaybepurity (a non-constructor with an empty body has avoid-ish return and is assumed to have side effects, so it never reaches this path — which is exactly why the reporter saw the problem withnew, and why the function/static analogues only trigger for callables declared without an explicit return type). The fix recovers the missing "no impure points" signal from the source file for the concrete call constructs.Pattern and where it was applied (the "concrete vs polymorphic call" axis, as the reporter framed it):
new X()— the reported case. Fixed.f()— concrete, same gap. Fixed.Foo::bar()/self::/parent::— concrete, same gap. Fixed.static::bar()excluded (polymorphic);parent::__construct()excluded (constructor).$x->bar()— intentionally not changed: the runtime object may be a subclass overridingbar(), so an empty body in the declared class does not prove the call is side-effect-free. This matches the existing collector design, which requires all possible runtime classes to be analysed and impure-point-free.Two classes of false positive were found via
make phpstanself-analysis and guarded against: built-in classes whose constructors are reflected from phpstorm-stubs with empty{}bodies (new DateTime(),new DateTimeZone(), ...), andparent::__construct()calls inside subclass constructors.Test
CallToConstructorStatementWithoutImpurePointsRuleTest::testBug14757— analyses a file that doesnew \PHPStan\Type\StringType()withoutStringType.phpin the analysed set; expectsCall to new PHPStan\Type\StringType() on a separate line has no effect.CallToFunctionStatementWithoutImpurePointsRuleTest::testBug14757—requires a definition file with an empty-bodied function and analyses a separate call file.CallToStaticMethodStatementWithoutImpurePointsRuleTest::testBug14757— same pattern for an empty-bodied static method.Each test fails before the change (no error reported) and passes after. The existing rule tests for all four families (including the instance-method
CallToMethodStatementWithoutImpurePointsRule, left unchanged) continue to pass, the full suite is green, and PHPStan self-analysis is clean.Fixes phpstan/phpstan#14757