Forget the tracked ob_get_level() after impure calls that may open or close output buffers

[phpstan-bot]

Summary

After PR #5909, PHPStan tracks the output-buffering nesting
level (ob_get_level()) in scope and uses it to narrow ob_get_contents() /
ob_get_clean() / ob_get_flush() / ob_get_length() away from false while
a buffer is known to be active. That tracked value survived calls to arbitrary
impure code, so a call to a non-pure callable between ob_start() and
ob_get_clean() incorrectly kept the level narrowed — hiding the possible
false return. This change forgets the tracked level after any call that
PHPStan cannot inspect and that could open or close a buffer.

Changes

• Added src/Analyser/ExprHandler/Helper/OutputBufferHelper.php centralizing
  the level-tracking logic: getLevelDelta()/applyLevelDelta() (moved out of
  FuncCallHandler), a cheap isLevelTracked() guard, invalidateLevel(), and
  callImmediatelyInvokesImpureCallable().
• src/Analyser/ExprHandler/FuncCallHandler.php: after applying the known delta
  for ob_start()/ob_end_*(), forget the level for callable/closure variable
  invocations and unknown functions (unless the invoked callable is provably
  pure), for impure non-builtin functions, and for built-ins that immediately
  invoke an impure callable argument.
• src/Analyser/ExprHandler/MethodCallHandler.php and
  src/Analyser/ExprHandler/StaticCallHandler.php: forget the level after
  impure non-builtin (or unknown/dynamic) method and static-method calls.
• tests/PHPStan/Analyser/nsrt/output-buffering.php: added regression cases.

Root cause

The ob_get_level() value is scope state that assumes no un-inspected code
changed the buffer level. PR #5909 only accounted for direct calls to the
ob_* family; every other call left the narrowed value in place. Any impure
user function, method, static method, closure or callable — and any built-in
that immediately invokes an impure callable — can call ob_start() /
ob_end_*() transitively and therefore must invalidate the tracked level. The
fix invalidates it in all three call handlers, gated by a cheap
isLevelTracked() check so code that never uses output buffering pays only an
O(1) map lookup. Pure callees and ordinary built-ins (e.g. printf()) keep the
narrowing.

Test

Extended the existing nsrt/output-buffering.php type-inference test with
cases covering: impure/pure callable and closure invocation, impure/pure
user functions, impure/pure methods, impure static methods, __invoke,
call_user_func, array_map with an impure vs. a pure callback, usort,
later-invoked callables (register_shutdown_function), and an ordinary
built-in (printf). Verified the test fails before the fix (the impure-call
cases still narrowed to string / int<1, max>) and passes after. Full
NodeScopeResolverTest, the Pure/CallMethods/CallCallables rule tests, the
integration test, and make phpstan self-analysis all pass.

Fixes phpstan/phpstan#14895

[staabm]

missing invalidation after new someclass() when constructor might call ob_end()

[phpstan-bot]

Done. I addressed staabm's review requesting invalidation after new someclass() when the constructor might call ob_end().

Changes

1. NewHandler.php — forget the tracked level after impure constructor calls

Mirroring the existing logic in the func/method/static-call handlers, after a new expression I now invalidate the tracked ob_get_level() when the constructor could open or close a buffer, gated by the cheap isLevelTracked() guard:

• Impure user-defined constructor (!isBuiltin() && !hasSideEffects()->no()) → forget
• Unknown class (no reflection) → forget
• Dynamic instantiation of a non-final class (a subclass constructor may be impure) → forget
• Pure/builtin constructor, or class without a constructor → keep the narrowing

2. FuncCallHandler.php — fixed a latent bug this branch already had

make phpstan was failing on the PR's own code at line 580: count($parametersAcceptor->getImpurePoints()) was called on a ParametersAcceptor|null (an unknown function or non-callable target leaves it null — that path would have thrown at runtime, and getImpurePoints() only exists on CallableParametersAcceptor). I narrowed to CallableParametersAcceptor and treat a non-callable/unknown target as "forget the level," which is the correct semantics anyway.

3. Regression tests in nsrt/output-buffering.php covering impure constructor, pure constructor, no-constructor, unknown class (new $className()), and a builtin constructor (new \ArrayObject()).

Verification

• Confirmed the new tests fail before the NewHandler fix (impure-constructor and unknown-class cases narrowed incorrectly) and pass after.
• make phpstan — green (was red on the branch before this work).
• make tests — all 17512 tests pass.

Both commits are pushed to create-pull-request/patch-yyc5669.

[staabm]

At this point its so much code for a maybe not so relevant problem, that I think we should revert the initial PR and re-open the issue instead.

@VincentLanglet wdyt?

[VincentLanglet]

Why do we need a dedicated logic to invalid ob_get_level (calling invalidateLevel in every handler), while it works already automatically for code like

class CoffeeBreak
{
	public int $foo = 1;
	
	public function doFoo(): void {
		$this->foo = 1;
		$this->doImpure();
		PHPStan\dumpType($this->foo); // int
	}

	public function doImpure():void {
		$this->foo = 2;
	}
}

Can't we concentrate the invalidation of ob_get_level on one single place ?

[phpstan-bot]

I've addressed VincentLanglet's review. Here's a summary of what I did and why.

What the review asked

Why does invalidating the tracked ob_get_level() need dedicated logic in every call handler, when invalidation of things like $this->foo after an impure call happens automatically? Can it be concentrated in one place?

What I found

The automatic mechanism the reviewer points to only invalidates expressions on the callee object ($scope->invalidateExpression($normalizedExpr->var, ...)). It never touches unrelated global state like the synthetic ob_get_level() expression, so there's no existing pass that would forget it for free.

I also checked whether the invalidation could be driven purely from the ExpressionResult's impure points in the single dispatch point (NodeScopeResolver::processExprNode). That doesn't work: built-ins such as printf() and register_shutdown_function() produce impure points but never change the nesting level, so an impure-points-based rule would forget the level after them and reintroduce the exact string|false false positives that PR #5909 fixed. The decision genuinely needs per-callee reflection (known/built-in/pure).

What I changed

I concentrated the policy (not just the invalidateLevel() call) into a single method, OutputBufferHelper::invalidateLevelAfterCall($scope, $calleeKnown, $calleeIsBuiltin, $calleeIsPure), which also holds the cheap isLevelTracked() guard. The four handlers no longer each carry a duplicated if (isLevelTracked && (reflection === null || (!builtin && !pure))) invalidate block — they just describe their callee and delegate. MethodCallHandler and StaticCallHandler were byte-identical and now share the exact same one-liner.

Verification

• nsrt/output-buffering.php passes unchanged (it asserts both the "keeps level" int<1, max> and "forgets level" int<0, max> cases, so both directions of the policy are confirmed).
• Full NodeScopeResolverTest (1668 tests), the Pure + CallMethods rule tests, and make phpstan self-analysis all green.

The remaining maintainer question (staabm suggesting the whole feature may not be worth the code) is a scope decision for the maintainers rather than something to act on here.

[VincentLanglet]

The issue found for ob_get_level can be reproduced with some other functions, for instance with the code

public function run(string $signature): void
    {
        assertType('string|false', openssl_error_string());

        if (false === \openssl_error_string()) {
            assertType('false', openssl_error_string());
            openssl_sign('1', $signature, '');
            assertType('string|false', openssl_error_string());
        }

		
        if (false === \openssl_error_string()) {
            assertType('false', openssl_error_string());
            $this->doImpure($signature);
            assertType('string|false', openssl_error_string()); // Currently it's false
        }
    }

	public function doImpure(string $signature): void
	{
		openssl_sign('1', $signature, '');
	}

Without starting the implementation, propose some general strategy which avoid having to manually invalidate all the expr at 4 differents places (funccall, methodcall, new, staticcall). The question is "What should be automatically invalidated after any possibly-impure call)
One idea could be exposing a method in the scope "onImpureCall" but maybe there are others

[VincentLanglet]

At this point its so much code for a maybe not so relevant problem, that I think we should revert the initial PR and re-open the issue instead.

@VincentLanglet wdyt?

Do you really encounter this issue ? @staabm

The issue exists for multiple others function
https://phpstan.org/r/61b76d44-65f0-465a-99c5-9f4868195280 and nobody reported it so far. And nobody reported it yet (except you) for ob_get_level ; so I wouldn't be in a hurry

Rather than focusing on ob_get_level, I think we should look for a general strategy "What should be invalidated on any impure call".

[phpstan-bot]

I've addressed VincentLanglet's review. Here's the summary that will be posted.

The general problem

The reviewer is right: ob_get_level() is not special. It's one instance of a broader category — argument-less function-call expressions tracked in scope whose value reflects mutable global state, not their arguments. openssl_error_string() is another, and the review demonstrates it has the exact same bug: a narrowed openssl_error_string() (from if (false === openssl_error_string())) survived a call to impure user code ($this->doImpure()) that reaches openssl_*() transitively, so PHPStan kept it narrowed to false.

Why a fully-automatic single dispatch point doesn't work

I evaluated the onImpureCall() idea driven purely from impure points at one place (processExprNode). It can't work: PHP built-ins like printf() produce impure points yet never touch the output-buffer / openssl state. Invalidating on any impure point would re-introduce the string|false false positives #5909 fixed. The keep/forget decision genuinely needs per-callee reflection (known / built-in / pure), which only exists inside the four call handlers.

Also, the automatic invalidation the reviewer pointed to ($this->foo becoming int after $this->doImpure()) only touches expressions on the callee object (invalidateExpression($var)) — it never reaches unrelated global state like ob_get_level().

What I changed

I concentrated the policy and the list of volatile expressions into a single MutatingScope method (alongside the existing afterOpenSslCall() / afterClearstatcacheCall() domain methods), rather than a per-feature helper spread across handlers:

• MutatingScope::invalidateVolatileExpressions() — forgets every tracked volatile global-state expression, listed once in self::VOLATILE_GLOBAL_STATE_FUNCTIONS = ['ob_get_level', 'openssl_error_string']. O(1) fast path via exact key lookups (these functions take no arguments), so code that never uses them pays nothing.
• MutatingScope::invalidateVolatileExpressionsAfterCall($calleeKnown, $calleeIsBuiltin, $calleeIsPure) — the shared keep/forget decision. The four handlers now just describe their callee and delegate one line each; adding a new volatile family in future means editing only the constant.
• OutputBufferHelper shrinks back to just the ob_* delta bookkeeping (getLevelDelta/applyLevelDelta); its isLevelTracked/invalidateLevel/invalidateLevelAfterCall are gone.
• This fixes the openssl bug for free — added the reviewer's scenario as a regression case in nsrt/bug-7106.php.

Verification

• The new openssl regression fails before the fix (bug-7106.php:33 → false) and passes after (string|false).
• Full NodeScopeResolverTest (1668), Pure + CallMethods + CallCallables rule tests, and make phpstan self-analysis all green.

On the naming: the tracked value is still a hardcoded family list rather than derived from reflection, because "reads mutable global state" isn't hasSideEffects() — these functions are side-effect-free but state-dependent, so PHPStan has no reflection signal to detect them automatically. The list is the pragmatic ground truth, now in one place.

(The remaining maintainer question — staabm suggesting the whole feature may not be worth the code — is a scope decision for the maintainers.)

[staabm]

I like this fix way more than mine

thank you