utopia-php / utopia-php/monorepo
Rector rewrites nullable null checks into fully-qualified instanceof checks
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.phpat the repo root enables the rule via thecodeQualityprepared set, alongsidedeadCode,typeDeclarations,earlyReturn,phpunitCodeQualityandwithPhpSets(). The rule is not listed individually and there is nowithSkip().rector/rector: ^2.4is a root dev dependency; packages do not require it themselves.bin/monorepo checkrunsrector process --dry-runper package (--fixdrops--dry-run), falling back to the rootrector.phpwhen a package has norector.phpof its own. No package currently has one.bin/monorepo check <package>runs in CI in.github/workflows/tests.yml, so a--dry-rundiff fails the build. The rewrite is not advisory.- On
maintoday there are 159instanceofuses acrosspackages/*/srcand 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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