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.

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

Use NullableTypeNode for unions in ConstantArrayType::toPhpDocNode #3191

Closed
wants to merge 1 commit into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jun 27, 2024

Currently, ConstantArrayType::toPhpDocNode() produces the following code when you have an array shape with nullable string type for key foo:

array{foo: (string | null)}

It would be great to convert those shapes to:

array{foo: ?string}

This feels more natural, especially because PHP also has the ?string type.

@ruudk ruudk changed the title Use NullableTypeNode for unions in ConstantArrayType Use NullableTypeNode for unions in ConstantArrayType::toPhpDocNode Jun 27, 2024
@ruudk ruudk force-pushed the constant-array-nullable-type branch from bd31b91 to fa33f8e Compare June 27, 2024 18:28
Currently, `ConstantArrayType::toPhpDocNode()` produces the following code
when you have an array shape with nullable string type for key `foo`:

```
array{foo: (string | null)}
```

It would be great to convert those shapes to:
```
array{foo: ?string}
```

This feels more natural, especially because PHP also has the ?string type.
@ruudk ruudk force-pushed the constant-array-nullable-type branch from fa33f8e to 978bec9 Compare June 27, 2024 18:35
@ondrejmirtes
Copy link
Member

Hi, I'm sorry, but I disagree with this. You're trying to change the code to adhere to your personal taste, otherwise the resulting PHPDoc node is equivalent before and after.

I have to disagree with this, because someone else might have the exact opposite preferences.

If you're using Type::toPhpDocNode() in your own code to achieve something, you can transform the resulting node with NodeTraverser (https://phpstan.github.io/phpdoc-parser/PHPStan.PhpDocParser.Ast.NodeTraverser.html). And not just in ArrayShapeItemNode but everywhere. That gives you the flexibility to make the node look exactly like you want it to.

Thanks for understanding.

@ruudk
Copy link
Contributor Author

ruudk commented Jun 28, 2024

Thanks for your reply. I knew this was a long shot so I get it! 😊

I did not think of traversing the PHPDoc's AST. That should work for the union case. 👍

I also noticed another thing. When I have a list of string and call toPhpDocNode on it, it produces array{string} instead of list<string>. Is this a bug? Or should I also try to fix this with traversing?

I think traversing would be hard, unless I'm able to connect all the PHPDocNode's to the original PHPStan types? Is that possible?

@ondrejmirtes
Copy link
Member

Yeah, that sounds like a bug, although very impropable, please try to reproduce it. There's a specific test case for toPhpDocNode in tests/.

What's in PHPStan\Type\Type that you're missing after conversion to AST?

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.

2 participants