Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix invariance composition #2054

Merged
merged 3 commits into from Dec 7, 2022
Merged

Fix invariance composition #2054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca86a9c
Fix invariance composition
jiripudil Nov 27, 2022
8a585ae
Fix GenericAncestorsCheck variance and add tests
jiripudil Dec 5, 2022
c1fe347
Hide fixed invariance composition behind a bleeding-edge toggle
jiripudil Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
View file
Open in desktop
Expand Up @@ -24,3 +24,4 @@ parameters:
nullContextForVoidReturningFunctions: true
unescapeStrings: true
duplicateStubs: true
invarianceComposition: true
2 changes: 2 additions & 0 deletions conf/config.neon
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@ parameters:
nullContextForVoidReturningFunctions: false
unescapeStrings: false
duplicateStubs: false
invarianceComposition: false
fileExtensions:
- php
checkAdvancedIsset: false
Expand Down Expand Up @@ -271,6 +272,7 @@ parametersSchema:
nullContextForVoidReturningFunctions: bool()
unescapeStrings: bool()
duplicateStubs: bool()
invarianceComposition: bool()
])
fileExtensions: listOf(string())
checkAdvancedIsset: bool()
Expand Down
2 changes: 2 additions & 0 deletions src/DependencyInjection/ContainerFactory.php
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Reflection\ReflectionProviderStaticAccessor;
use PHPStan\Type\Accessory\AccessoryArrayListType;
use PHPStan\Type\Generic\TemplateTypeVariance;
use Symfony\Component\Finder\Finder;
use function array_diff_key;
use function array_map;
Expand Down Expand Up @@ -174,6 +175,7 @@ public static function postInitializeContainer(Container $container): void

BleedingEdgeToggle::setBleedingEdge($container->getParameter('featureToggles')['bleedingEdge']);
AccessoryArrayListType::setListTypeEnabled($container->getParameter('featureToggles')['listType']);
TemplateTypeVariance::setInvarianceCompositionEnabled($container->getParameter('featureToggles')['invarianceComposition']);
}

public function clearOldContainers(string $tempDirectory): void
Expand Down
2 changes: 1 addition & 1 deletion src/Rules/Generics/GenericAncestorsCheck.php
View file
Open in desktop
Expand Up @@ -98,7 +98,7 @@ public function check(
$messages[] = RuleErrorBuilder::message(sprintf($invalidTypeMessage, $referencedClass))->build();
}

$variance = TemplateTypeVariance::createInvariant();
$variance = TemplateTypeVariance::createStatic();
Copy link
Member

Choose a reason for hiding this comment

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

What about this? What does it change? What's the impact? Shouldn't it be done under the feature toggle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I fixed the invariance composition, this check reported all template types in @extends as invariant. Therefore, this would not be allowed:

/** @template-covariant T */
interface Foo {}

/**
 * @template-covariant T
 * @implements Foo<T>
 */
class Bar implements Foo {}

I can add the feature toggle but I don't think it's necessary. This change has no practical impact because "static" variance has had the same composition logic as invariant until this PR. It even feels semantically more correct to me: the initial position in @extends is not invariance, it is nothing, similar to other static contexts like static methods or the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, great 😊

$messageContext = sprintf(
$invalidVarianceMessage,
$ancestorType->describe(VerbosityLevel::typeOnly()),
Expand Down
11 changes: 11 additions & 0 deletions src/Type/Generic/TemplateTypeVariance.php
View file
Open in desktop
Expand Up @@ -21,6 +21,8 @@ class TemplateTypeVariance
/** @var self[] */
private static array $registry;

private static bool $invarianceCompositionEnabled = false;

private function __construct(private int $value)
{
}
Expand Down Expand Up @@ -93,6 +95,10 @@ public function compose(self $other): self
return self::createInvariant();
}

if (self::$invarianceCompositionEnabled && $this->invariant()) {
return self::createInvariant();
}

return $other;
}

Expand Down Expand Up @@ -173,4 +179,9 @@ public static function __set_state(array $properties): self
return new self($properties['value']);
}

public static function setInvarianceCompositionEnabled(bool $enabled): void
{
self::$invarianceCompositionEnabled = $enabled;
}

}
10 changes: 9 additions & 1 deletion tests/PHPStan/Rules/Generics/ClassAncestorsRuleTest.php
View file
Open in desktop
Expand Up @@ -95,9 +95,17 @@ public function testRuleExtends(): void
'Template type T is declared as covariant, but occurs in invariant position in extended type ClassAncestorsExtends\FooGeneric8<T, T> of class ClassAncestorsExtends\FooGeneric9.',
192,
],
[
'Template type T is declared as contravariant, but occurs in covariant position in extended type ClassAncestorsExtends\FooGeneric8<T, T> of class ClassAncestorsExtends\FooGeneric10.',
201,
],
[
'Template type T is declared as contravariant, but occurs in invariant position in extended type ClassAncestorsExtends\FooGeneric8<T, T> of class ClassAncestorsExtends\FooGeneric10.',
201,
],
[
'Class ClassAncestorsExtends\FilterIteratorChild extends generic class FilterIterator but does not specify its types: TKey, TValue, TIterator',
197,
215,
'You can turn this off by setting <fg=cyan>checkGenericClassInNonGenericObjectType: false</> in your <fg=cyan>%configurationFile%</>.',
],
]);
Expand Down
24 changes: 20 additions & 4 deletions tests/PHPStan/Rules/Generics/MethodSignatureVarianceRuleTest.php
View file
Open in desktop
Expand Up @@ -63,7 +63,11 @@ public function testRule(): void
35,
],
[
'Template type X is declared as covariant, but occurs in contravariant position in parameter k of method MethodSignatureVariance\Covariant\C::a().',
'Template type X is declared as covariant, but occurs in invariant position in parameter k of method MethodSignatureVariance\Covariant\C::a().',
35,
],
[
'Template type X is declared as covariant, but occurs in invariant position in parameter l of method MethodSignatureVariance\Covariant\C::a().',
35,
],
[
Expand Down Expand Up @@ -91,9 +95,13 @@ public function testRule(): void
65,
],
[
'Template type X is declared as covariant, but occurs in contravariant position in return type of method MethodSignatureVariance\Covariant\C::l().',
'Template type X is declared as covariant, but occurs in invariant position in return type of method MethodSignatureVariance\Covariant\C::l().',
68,
],
[
'Template type X is declared as covariant, but occurs in invariant position in return type of method MethodSignatureVariance\Covariant\C::m().',
71,
],
]);

$this->analyse([__DIR__ . '/data/method-signature-variance-contravariant.php'], [
Expand Down Expand Up @@ -122,7 +130,11 @@ public function testRule(): void
35,
],
[
'Template type X is declared as contravariant, but occurs in covariant position in parameter l of method MethodSignatureVariance\Contravariant\C::a().',
'Template type X is declared as contravariant, but occurs in invariant position in parameter k of method MethodSignatureVariance\Contravariant\C::a().',
35,
],
[
'Template type X is declared as contravariant, but occurs in invariant position in parameter l of method MethodSignatureVariance\Contravariant\C::a().',
35,
],
[
Expand Down Expand Up @@ -154,7 +166,11 @@ public function testRule(): void
65,
],
[
'Template type X is declared as contravariant, but occurs in covariant position in return type of method MethodSignatureVariance\Contravariant\C::m().',
'Template type X is declared as contravariant, but occurs in invariant position in return type of method MethodSignatureVariance\Contravariant\C::l().',
68,
],
[
'Template type X is declared as contravariant, but occurs in invariant position in return type of method MethodSignatureVariance\Contravariant\C::m().',
71,
],
]);
Expand Down
18 changes: 18 additions & 0 deletions tests/PHPStan/Rules/Generics/data/class-ancestors-extends.php
View file
Open in desktop
Expand Up @@ -194,6 +194,24 @@ class FooGeneric9 extends FooGeneric8

}

/**
* @template-contravariant T
* @extends FooGeneric8<T, T>
*/
class FooGeneric10 extends FooGeneric8
{

}

/**
* @template T
* @extends FooGeneric8<T, T>
*/
class FooGeneric11 extends FooGeneric8
{

}

class FilterIteratorChild extends \FilterIterator
{

Expand Down