utopia-php / utopia-php/monorepo

Rector rewrites nullable null checks into fully-qualified instanceof checks

Open
#97 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
3
Forks
4
Avg merge
12h 25m
Merged PRs (30d)
103

Description

Raised in review on #94: https://github.com/utopia-php/monorepo/pull/94#discussion_r3664574226

The code

packages/pools/src/Pools/Pool.php, in reclaim():

/**
 * @param  Connection<TResource>|null  $connection  Reclaims every active connection when null.
 */
public function reclaim(?Connection $connection = null): static
{
    if ($connection instanceof \Utopia\Pools\Connection) {
        return $this->push($connection);
    }

    foreach ($this->active as $active) {
        $this->push($active);
    }

    return $this;
}

The instanceof was not hand-written. It was produced by Rector from if ($connection !== null).

Positions

@abnegate: keep the null check instead of the type check. The parameter is declared ?Connection, so !== null says exactly what the branch means — "was a connection passed?" — and the rewritten form says something narrower than the intent.

The argument for the rule: !== null only proves not-null. If the runtime value is ever wider than the declaration — a docblock-only type, a mixed, a union with false in it — the null check passes values the body cannot handle, while instanceof proves the thing you are about to call methods on. The same file has a genuine instance of that case in pop(), where $this->adapter->pop() returns Connection|false|null and instanceof Connection is load-bearing, not cosmetic — the old code needed $connection === false || $connection === null to cover it. The rule cannot tell the two situations apart; it fires on any nullable object type.

The rule

Rector\CodeQuality\Rector\Identical\FlipTypeControlToUseExclusiveTypeRector — "Flip type control from null compare to use exclusive instanceof object". Its documented sample:

// before
function process(?DateTime $dateTime)
{
    if ($dateTime === null) {
        return;
    }
}

// after
function process(?DateTime $dateTime)
{
    if (! $dateTime instanceof DateTime) {
        return;
    }
}

It matches Identical and NotIdentical nodes where one side resolves to null, asks PHPStan's NullableTypeAnalyzer whether the other side is a nullable object type, and if so replaces the comparison with instanceof (wrapped in ! for the === null direction). It emits a FullyQualified name node, which is why the output is \Utopia\Pools\Connection and not Connection — even though Pool and Connection share the Utopia\Pools namespace and the short name is already in scope. Pint does not shorten it back; nothing in pint.json touches class references.

Interaction with PHPStan and types

Both forms narrow identically for PHPStan when the declared type really is ?Connection, so the rewrite buys no analysis strictness here. The value the rule adds is where the declaration and the runtime type diverge — precisely the case a stricter PHPStan level is supposed to eliminate. packages/pools/phpstan.neon runs level 8; the monorepo baseline phpstan.neon is level 5. So the rule's payoff varies by package: closer to redundant at level 8, more defensible at level 5, where nullable-object types are less reliably enforced.

Our setup

  • rector.php at the repo root enables the rule via the codeQuality prepared set, alongside deadCode, typeDeclarations, earlyReturn, phpunitCodeQuality and withPhpSets(). The rule is not listed individually and there is no withSkip().
  • rector/rector: ^2.4 is a root dev dependency; packages do not require it themselves.
  • bin/monorepo check runs rector process --dry-run per package (--fix drops --dry-run), falling back to the root rector.php when a package has no rector.php of its own. No package currently has one.
  • bin/monorepo check <package> runs in CI in .github/workflows/tests.yml, so a --dry-run diff fails the build. The rewrite is not advisory.
  • On main today there are 159 instanceof uses across packages/*/src and none are fully-qualified, so this rewrite has not landed anywhere yet — #94 is the first place it surfaces.

Scope of the decision

The rule is configured monorepo-wide, so this is a monorepo-wide call, not a pools one: either every nullable-object null check becomes a fully-qualified instanceof, or the rule is skipped everywhere and the pop()-style cases are written by hand.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with rector.php and the Rector rule described in the issue, then inspect packages/pools/src/Pools/Pool.php and packages/pools/phpstan.neon to compare the nullable checks. Review bin/monorepo and .github/workflows/tests.yml to understand how dry-run differences are enforced. Done means the monorepo-wide decision is reflected consistently in Rector checks without producing the disputed rewrite.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
ci-cd, tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.