Skip to content

Do not traverse into GenericObjectType type arguments in RuleLevelHelper::transformAcceptedType()#5459

Closed
phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-uyayspy
Closed

Do not traverse into GenericObjectType type arguments in RuleLevelHelper::transformAcceptedType()#5459
phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-uyayspy

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When checkNullables is false (rule levels 0–7), RuleLevelHelper::transformAcceptedType() traverses into GenericObjectType's type arguments and removes null from them. This causes invariant template type checking to produce false positives where the error message shows identical types on both sides (e.g. "should return Attribute<string|null, never> but returns Attribute<string|null, never>").

Changes

  • Added early return in RuleLevelHelper::transformAcceptedType() when the traversed type is a GenericObjectType, preventing the removeNull transformation from modifying generic type arguments (src/Rules/RuleLevelHelper.php)
  • Added checkNullables configuration field to ReturnTypeRuleTest, TypesAssignedToPropertiesRuleTest, and ClosureReturnTypeRuleTest to enable testing with checkNullables: false

Root cause

RuleLevelHelper::transformAcceptedType() uses TypeTraverser::map() to walk the accepted type tree. When checkNullables is false, it removes null from union types via TypeCombinator::removeNull(). The traversal descends into GenericObjectType::traverse(), which iterates over the generic type arguments. This means a type argument like string|null gets transformed to string in the accepted type, while the accepting type's argument remains string|null.

For invariant templates, TemplateTypeVariance::isValidVariance() uses $a->equals($b) to compare type arguments. After the transformation, (string|null)->equals(string) returns false, producing a false positive error.

The fix stops the traversal at GenericObjectType boundaries. This is correct because:

  1. Generic type arguments represent template bindings whose identity must be preserved for invariant comparison
  2. The removeNull logic is meant for top-level type leniency at lower rule levels, not for modifying the internal structure of generic types
  3. For covariant/contravariant templates, isSuperTypeOf() is used instead of equals(), which handles nullable differences correctly without needing the transformation

Test

  • ReturnTypeRuleTest::testBug12490 — reproduces the original issue: method returning Attribute<string|null, never>, Attribute<float|null, never>, Attribute<int|null, never> with checkNullables: false
  • CallMethodsRuleTest::testBug12490 — analogous case for method call parameters with Container<string|null>, Container<int|null>, Container<float|null>
  • TypesAssignedToPropertiesRuleTest::testBug12490 — analogous case for property assignments
  • ClosureReturnTypeRuleTest::testBug12490 — analogous case for closure return types

All four tests fail without the fix and pass with it.

Fixes phpstan/phpstan#12490

…Helper::transformAcceptedType()`

- When `checkNullables` is false (levels 0-7), `transformAcceptedType()` removes
  null from types via `TypeCombinator::removeNull()`. The traversal descended into
  `GenericObjectType`'s type arguments, removing null from them. This caused
  invariant template type checking (which uses `equals()`) to fail when comparing
  identical types like `Container<string|null>` vs `Container<string|null>`, because
  the accepted type's argument was transformed to `string` while the accepting type's
  argument remained `string|null`.
- The fix stops traversal at `GenericObjectType` boundaries, preserving type
  arguments as-is. This is correct because generic type arguments represent template
  bindings whose identity must be preserved for invariant template comparison.
- Verified the same bug affected method call parameters, property assignments, and
  closure return types — all use the same `RuleLevelHelper::accepts()` path and are
  fixed by this single change.
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure

/**
 * @template T
 */
class Container
{
	/** @var T */
	public $value;

	/**
	 * @param T $value
	 */
	public function __construct($value)
	{
		$this->value = $value;
	}
}

class Foo
{
	/** @return Container<string> */
	public function test(?string $val): Container
	{
		return new Container($val);
	}
}

is still not reported with checkNullables: false

…ObjectType type arguments

Instead of completely blocking traversal into GenericObjectType, use
traverseSimultaneously to pair up accepted and accepting type arguments.
Only remove null from accepted type arguments where the corresponding
accepting type argument does not contain null.

This ensures that:
- Container<string|null> accepted where Container<string|null> is expected
  does not produce a false positive (null not removed when accepting also has null)
- Container<string|null> accepted where Container<string> is expected
  correctly has null removed (accepted matches after transformation)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Changes have been committed and pushed. Here's a summary of what was done:

Problem: The reviewer (VincentLanglet) pointed out that the original fix — which completely blocked traversal into GenericObjectType type arguments — was too aggressive. It prevented the removeNull transformation from working when the accepting type's generic argument does NOT contain null (e.g., Container<string> accepting Container<string|null> should work with checkNullables: false).

Fix: Instead of returning $acceptedType unchanged when it's a GenericObjectType, use traverseSimultaneously to pair up type arguments from both the accepted and accepting types. For each pair:

  • If the accepting type argument contains null → leave the accepted argument unchanged (prevents false positives like Container<string|null> vs Container<string|null>)
  • If the accepting type argument does NOT contain null → remove null from the accepted argument (allows Container<string|null> to match Container<string>)

Test case added: The reviewer's exact example — Container<string> return type with ?string parameter — is now covered in the test data.

Results: All 11,858 tests pass and make phpstan reports no errors.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using directly SimultaneousTypeTraverser::map instead of TypeTraverser::map ?

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

I can't post directly to the PR, but the instructions mention the results will be posted separately. Let me summarize my findings:


Summary

After thorough investigation of VincentLanglet's suggestion to use SimultaneousTypeTraverser::map directly instead of TypeTraverser::map, I found that it cannot work as a direct replacement. I implemented the change, and it caused 3 test failures:

Why SimultaneousTypeTraverser::map doesn't work here

  1. Structural matching requirement: SimultaneousTypeTraverser only descends into inner types when both types match structurally. When they don't (e.g., list<string|null> vs array<string>|null), the traversal stops and removeNull never reaches inner types. This broke testListWithNullablesUnchecked.

  2. Callable parameter handling: CallableType::traverseSimultaneously traverses both parameters and return type. The current code intentionally only traverses the return type — removing null from callable parameters changes the callable's contract incorrectly for contravariant parameter checking. This broke testCallablesWithoutCheckNullables.

  3. The containsNull condition can't work uniformly: The null-removal condition needs to differ at generic-argument boundaries (use containsNull) vs. top level (use instanceof NullType). A single SimultaneousTypeTraverser callback can't distinguish these contexts. This broke testBug8846.

Current design is correct

TypeTraverser::map provides unconditional depth-first traversal of the accepted type (needed for removeNull to reach all inner types). traverseSimultaneously is used specifically at the GenericObjectType boundary, where types are guaranteed to match structurally. This hybrid approach is the right design.

No code changes needed — the current implementation (TypeTraverser::map + GenericObjectType special case with traverseSimultaneously) is correct and all 11,858 tests + make phpstan pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants