Make VisitorArray shape explicit #1675
Conversation
When creating a node visitor that uses more specific types, we get this error: ``` Parameter webonyx#2 $visitor of static method GraphQL\Language\Visitor::visit() expects array<string, array<string, callable(GraphQL\Language\AST\Node): (GraphQL\Language\VisitorOperation|void|false|null)>|(callable(GraphQL\Language\AST\Node): (GraphQL\Language\VisitorOperation|void|false|null))>, array{OperationDefinition: array{enter: Closure(GraphQL\Language\AST\OperationDefinitionNode): void, leave: Closure(): void}, Field: array{enter: Closure(GraphQL\Language\AST\FieldNode): void, leave: Closure(GraphQL\Language\AST\FieldNode): void}, InlineFragment: array{enter: Closure(GraphQL\Language\AST\InlineFragmentNode): void, leave: Closure(GraphQL\Language\AST\InlineFragmentNode): void}, FragmentDefinition: array{enter: Closure(GraphQL\Language\AST\FragmentDefinitionNode): void, leave: Closure(GraphQL\Language\AST\FragmentDefinitionNode): void}, SelectionSet: Closure(GraphQL\Language\AST\SelectionSetNode): void} given. ``` For the following code: ```php $ast = Visitor::visit($ast, [ NodeKind::OPERATION_DEFINITION => [ 'enter' => function (OperationDefinitionNode $node) : void { $this->parents[] = $node->operation; // Remove the operation name as we don't need this $node->name = null; }, 'leave' => function () : void { array_pop($this->parents); }, ], NodeKind::FIELD => [ 'enter' => function (FieldNode $node) : void { $this->parents[] = $node->name->value; }, 'leave' => function (FieldNode $node) : void { array_pop($this->parents); }, ], // ... ``` By changing the signature to `covariant` the problem goes away.
|
I guess this doesn't work. When I tested it, it produced no error, but that's probably because PHPStan skipped the error completely. |
|
In order to make this work, we would have to define an array shape for the visitor. Something like this: How badly do you want this? 😉 |
|
If you would accept that, I can do it 😊 |
|
Yeah, I am fine with it. It should allow use to get rid of some |
ruudk
changed the title
|
These docs / cs fixer pushes are handy and annoying at the same time... they don't trigger CI to run.. So I have to pull them , force push it. |
|
CI does run for the previously pushed commit, you can check the results there.
|
This makes it easier to work with NodeList, if you don't care about it's type.
|
After this change, PHPStan time shoots from 30 seconds to multiple minutes on CI 😭 |
I am not sure if it is worth the added complexity and worse development experience then. I imagine it would also slow down PHPStan for consumers of the library significantly? |
|
Maybe @ondrejmirtes has some insights on how to solve this type of problem. I tried searching for this, but I probably use the wrong terminology so couldn't find similar things. |
|
Please formulate the question by providing full context, I have no idea what's going on here. |
|
You're right, I feel bad for not giving you the proper context when pinging you. My apologies. This library has an AST visitor that works like this: graphql-php/src/Language/Visitor.php Lines 30 to 90 in ed6b5cc For example: $ast = Visitor::visit($ast, [
NodeKind::OPERATION_DEFINITION => [
'enter' => function ($node) : void {
// $node is OperationDefinitionNode
},
],
NodeKind::FIELD => [
'enter' => function ($node) : void {
// $node is FieldNode
},
],
]);Depending on the node kind you add the visitor for, you get the corresponding Node type. I want to write my visitor like this: $ast = Visitor::visit($ast, [
NodeKind:: OPERATION_DEFINITION => [
'enter' => function (OperationDefinitionNode $node) : void {},
],
NodeKind::FIELD => [
'enter' => function (FieldNode $node) : void {},
],
]);But that doesn't work because you get an error like this:
In this PR I changed the type of the graphql-php/src/Language/Visitor.php Lines 115 to 210 in 606c093 It's not nice, but at least the developer now has full type safety. It works, except it now takes ±4 minutes to analyze. Before it would take 30 seconds. So this is a performance issue, and also something that might be expected when creating such a monstrous array shape. But that leads me to the question: can this be done simpler? Maybe it could be done once we have support for class-string-map and then leverage the info defined here: graphql-php/src/Language/AST/NodeKind.php Lines 76 to 137 in ed6b5cc |
|
Alright, so you want to report a performance PHPStan issue. Please create an isolated gist.github.com and open an issue on PHPStan's GitHub. Also you can try creating an example on phpstan.org/try with two or three different NodeKinds (so it finishes in time) and I can try rewriting it using https://phpstan.org/writing-php-code/phpdoc-types#offset-access or something like that. |
Done here: phpstan/phpstan#12661
This is the small variant that runs on the playground: https://phpstan.org/r/a3fde113-c526-4b87-be65-ee8c2586217c |
|
I'm sorry, I wasn't able to figure out a different syntax with the "offset access type", it's too complicated. |
When creating a node visitor that uses more specific types, we get this error:
For the following code: