Include null in return type of Closure::bindTo() and Closure::bind() dynamic return type extensions

[phpstan-bot]

Summary

Closure::bindTo() and Closure::bind() can return null when binding fails (e.g., binding a static closure to an object, or binding to an internal class). The dynamic return type extensions for these methods were stripping the null from the return type, causing PHPStan to miss potential null-pointer issues.

Changes

• src/Type/Php/ClosureBindToDynamicReturnTypeExtension.php — return TypeCombinator::union($closureType, new NullType()) instead of just $closureType
• src/Type/Php/ClosureBindDynamicReturnTypeExtension.php — same fix for the static Closure::bind() method
• tests/PHPStan/Analyser/nsrt/closure-return-type-extensions.php — updated assertions to expect (Closure(...))|null
• tests/PHPStan/Analyser/AnalyserIntegrationTest.php — updated testBug4734 to expect 4 new "Trying to invoke (Closure(): void)|null but it might not be a callable" errors (these are correct new diagnostics)
• tests/PHPStan/Analyser/nsrt/bug-5009.php — new regression test

Root cause

Both ClosureBindToDynamicReturnTypeExtension and ClosureBindDynamicReturnTypeExtension returned the bare ClosureType when the caller was a known closure, which preserved the closure's parameter/return type information but dropped the null that PHP's actual return type includes. The function signature map has __benevolent<Closure|null> as the default, but this was completely overridden by the extension returning a non-nullable type.

Analogous cases probed

• Closure::fromCallable() — always returns Closure, never null. Not affected.
• Closure::call() — returns the closure's return value, not a closure. Not analogous.

Test

• tests/PHPStan/Analyser/nsrt/bug-5009.php — verifies that bindTo() and bind() with various arguments all return (Closure(): void)|null
• Updated tests/PHPStan/Analyser/nsrt/closure-return-type-extensions.php to assert the nullable return

Fixes phpstan/phpstan#5009

[VincentLanglet]

Given the fact it's __benevolent<Closure|null> the dynamic return type should not lose the benevolentUnion

[phpstan-bot]

Done. The change addresses VincentLanglet's review by using BenevolentUnionType instead of a strict TypeCombinator::union() in both ClosureBindToDynamicReturnTypeExtension and ClosureBindDynamicReturnTypeExtension. This preserves the __benevolent<Closure|null> semantics from the function signature map, so code that calls bindTo()/bind() and immediately invokes the result won't get false-positive "might not be a callable" errors at lower rule levels. The bug-4734 integration test reverts to its original 5 errors (the 4 spurious "Trying to invoke" errors are gone).

[VincentLanglet]

Is it possible to exclude null in the return type when we're sure it cannot be returned ?

[phpstan-bot]

Pushed. Here's a summary of the change:

When $newThis is null, bindTo(null) / bind($closure, null) only changes the closure's scope — it never fails, so the return type is now just Closure (no null). This is true regardless of whether the closure is static.

When $newThis is non-null, binding can still fail if the closure is static (PHP returns null), so the return type remains BenevolentUnionType(Closure|null).

Fully excluding null for non-static closures with non-null $newThis would require tracking static-ness in ClosureType, which is a larger change — ClosureType currently has no $isStatic property. That could be a follow-up improvement (adding an optional ?TrinaryLogic $isStatic parameter to the @api constructor would be BC-safe).

[VincentLanglet]

Fully excluding null for non-static closures with non-null $newThis would require tracking static-ness in ClosureType, which is a larger change — ClosureType currently has no $isStatic property. That could be a follow-up improvement (adding an optional ?TrinaryLogic $isStatic parameter to the @api constructor would be BC-safe).

Try the implementation

[phpstan-bot]

Pushed successfully. Here's a summary of what was done:

Added isStatic tracking to ClosureType to exclude null from bindTo()/bind() return types when we can prove binding will succeed.

Changes:

1. src/Type/ClosureType.php — Added ?TrinaryLogic $isStatic = null as the last constructor parameter (BC-safe since it's optional with null default). Added isStaticClosure(): TrinaryLogic getter. Preserved $isStatic through traverse(), traverseSimultaneously(), and describe().

2. src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php — All three new ClosureType(...) calls now pass isStatic: TrinaryLogic::createFromBoolean($expr->static), capturing the static keyword from the PHP parser AST.

3. src/Reflection/InitializerExprTypeResolver.php — The static closure path (already gated by $expr->static) now passes isStatic: TrinaryLogic::createYes().

4. src/Type/Php/ClosureBindToDynamicReturnTypeExtension.php and ClosureBindDynamicReturnTypeExtension.php — When $closureType->isStaticClosure()->no(), return just $closureType (no null). Non-static closures always bind successfully regardless of $newThis. Otherwise, fall through to the existing null-$newThis check and BenevolentUnionType logic.

5. tests/PHPStan/Analyser/nsrt/bug-5009.php — Updated assertions: non-static closures now return Closure(): void (not nullable) even with non-null $newThis. Added test cases for Closure::bind() with static closures and $maybeNull with static closures.

[VincentLanglet]

fix failing tests

[phpstan-bot]

The review asked to "fix failing tests." Done. Here's what was failing and why.

Root cause

The PR's last commit (e76d96347) added static-ness tracking to ClosureType, which had two ripple effects on test expectations that weren't updated:

1. Static closures now describe with a static- prefix (e.g. static-Closure(): void).
2. The bindTo()/bind() extensions now return non-null for non-static closures (binding always succeeds), but (...|null) for static or unknown-staticness closures (binding can fail).

Three test data files had stale expectations:

• closure-return-type-extensions.php — Closure::fromCallable() yields a non-static closure, so bindTo() no longer adds null. Reverted these back to Closure(object): true.
• bug-5009.php — static closures now render with the static- prefix; updated the five static-closure assertions.
• closure-static-type.php (pre-existing, from PR Introduce ClosureType::isStaticClosure() #5699) — asserted the old non-null bindTo behavior. Updated to ((static-Closure(): void)|null) etc., which is semantically correct: binding an instance to a static closure fails in PHP (returns null), and an unknown closure may be static.

Verification

• NodeScopeResolverTest (all 1648 nsrt files) — pass
• AnalyserIntegrationTest + ClosureTypeTest (233 tests) — pass
• make phpstan — no errors

Committed as dfac90854 and pushed to the PR branch.

[VincentLanglet]

I think the logic should be

$args = $methodCall->getArgs();
    if (!isset($args[0])) {
        return null;
    }

    $closureType = $scope->getType($args[0]->value);
    if (!($closureType instanceof ClosureType)) {
        return null;
    }

    if ($closureType->isStaticClosure()->yes()) {
        $newThisIsNull = !isset($args[1]) || $scope->getType($args[1]->value)->isNull()->yes();

        return $newThisIsNull ? $closureType : new NullType();
    }

    return new BenevolentUnionType([$closureType, new NullType()]);

[phpstan-bot]

Done. I implemented the reviewer's suggested logic restructure and pushed it.

What changed

ClosureBindDynamicReturnTypeExtension.php and ClosureBindToDynamicReturnTypeExtension.php now follow VincentLanglet's exact structure:

if ($closureType->isStaticClosure()->yes()) {
    $newThisIsNull = !isset($args[N]) || $scope->getType($args[N]->value)->isNull()->yes();
    return $newThisIsNull ? $closureType : new NullType();
}
return new BenevolentUnionType([$closureType, new NullType()]);

The behavioral difference from the previous version:

• Static closure + non-null $newThis → null (was (Closure|null)). This is correct: PHP returns null when binding a static closure to an object.
• Static closure + null $newThis → the closure unchanged (binding only changes scope).
• Non-static / unknown-staticness closure → BenevolentUnionType<Closure|null> in all cases (was non-null for proven non-static). Keeps the __benevolent<Closure|null> semantics the reviewer asked to preserve back in the first review.

Test updates (all driven by actual analyser output, not guesses)

• bug-5009.php, closure-static-type.php, closure-return-type-extensions.php — updated assertions to the new types.
• AnalyserIntegrationTest::testBug4734 — now expects 6 errors; the new one (Trying to invoke null but it's not a callable. at line 43) is a genuine diagnostic, since that test binds a static closure to new Foo() and invokes the result, which is null at runtime.

Verification

• NodeScopeResolverTest — 1648/1648 pass
• AnalyserIntegrationTest + ClosureTypeTest — 233/233 pass
• make phpstan — no errors

Committed as 79d4b219e and pushed.

One note for the reviewer's awareness: with this logic, $static->bindTo($maybeNull) where $maybeNull is stdClass|null now infers null (since isNull()->yes() is false for the maybe-null case), rather than (static-Closure|null). That's a slight precision loss in that edge case, but it's a direct consequence of the suggested structure — happy to special-case ->maybe() if you'd prefer.

[VincentLanglet]

Handle the case where isNull return maybe, the value should be

new BenevolentUnionType([$closureType, new NullType()])

[phpstan-bot]

Done. I addressed VincentLanglet's review comment about the maybe-null case.

What changed

In both ClosureBindDynamicReturnTypeExtension and ClosureBindToDynamicReturnTypeExtension, the static-closure branch previously collapsed isNull() to a boolean via ->yes(), which lumped the maybe-null case in with the definitely-non-null case and returned a bare NullType. Now it dispatches on all three TrinaryLogic outcomes of $newThis's nullness:

if ($closureType->isStaticClosure()->yes()) {
    $newThisIsNull = isset($args[N]) ? $scope->getType($args[N]->value)->isNull() : TrinaryLogic::createYes();
    if ($newThisIsNull->yes()) {
        return $closureType;        // binding a static closure with null $newThis only changes scope
    }
    if ($newThisIsNull->no()) {
        return new NullType();      // binding a static closure to an object always fails
    }
    return new BenevolentUnionType([$closureType, new NullType()]);  // maybe-null → either outcome
}

This fixes the precision loss I flagged in my earlier note: $static->bindTo($maybeNull) (where $maybeNull is stdClass|null) now infers (static-Closure(): void)|null instead of null.

Verification

• NodeScopeResolverTest — 1648/1648 pass (updated the bug-5009.php assertion for $staticBoundMaybe)
• AnalyserIntegrationTest + ClosureTypeTest — 233/233 pass
• make phpstan — no errors

Committed as a separate logical change and pushed to the PR branch.

[staabm]

we should have a test that we no longer have a error in https://phpstan.org/r/6cb58802-5290-42da-ac62-35f999f6ee8a

[phpstan-bot]

Done. I addressed VincentLanglet's review request.

What the review asked for

we should have a test that we no longer have an error in https://phpstan.org/r/6cb58802-5290-42da-ac62-35f999f6ee8a

That playground snippet binds a closure inside a constructor and coalesces the result:

$this->callback = $callback->bindTo($this, $this) ?? $callback;

Before the fix, bindTo() returned a non-nullable Closure, so the ?? triggered a false positive: "Expression on left side of ?? is not nullable." (nullCoalesce.expr). With the fix returning __benevolent<Closure|null>, the left side is nullable and the error is gone.

What I did

• tests/PHPStan/Rules/Variables/data/bug-5009.php — the playground snippet (verbatim, with a namespace added to avoid a clash on the generic class name Test).
• tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php — added testBug5009() asserting no errors.

Verification (followed the stash-revert protocol)

• Temporarily reverted the bindTo extension to return the bare closure → test failed with exactly Expression on left side of ?? is not nullable. at line 16 (the ?? line). Restored the fix → test passes.
• NullCoalesceRuleTest — 31/31 pass.
• make phpstan — no errors.

Committed as 1ab9e5a75 and pushed.

[staabm]

looking at the CI error in
https://github.com/phpstan/phpstan-src/actions/runs/27432899018/job/81087916773?pr=5675

it looks like we are missing some benevolent handling?

---

created another followup phpstan/phpstan#14818