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

Support for nullable type notation #349

Open
hemberger opened this issue Jul 10, 2022 · 3 comments
Open

Support for nullable type notation #349

hemberger opened this issue Jul 10, 2022 · 3 comments

Comments

@hemberger
Copy link

I was wondering if it would be possible to support nullable type notation (e.g. ?type) instead of requiring type|null. The benefit of supporting nullable type notation is that it would help improve parity between inline PHP typehints and docblock typehints.

I have found that the following sniffs do not support nullable types, though there may be more:

  • Spryker.Commenting.DocBlockParamAllowDefaultValue.Default
  • Spryker.Commenting.DocBlockReturnNullableType.ReturnNullableMissing

Here is a specific example:

LINE 16: ERROR [x] Possible doc block error: `?array<string, string> $token` seems to be missing type `null`.
                   (Spryker.Commenting.DocBlockParamAllowDefaultValue.Default)
--------------------------------------------------------------------------------------------------------------
   14:  
   15:  »   /**
>> 16:  »    * @param ?array<string, string> $token
   17:  »    */
   18:  »   private static function getTwitterObj(?array $token = null): TwitterOAuth {

Thanks for your time!

hemberger added a commit to hemberger/smr that referenced this issue Jul 10, 2022
The following sniffs do not support nullable type notation:

* Spryker.Commenting.DocBlockParamAllowDefaultValue.Default
* Spryker.Commenting.DocBlockReturnNullableType.ReturnNullableMissing

See spryker/code-sniffer#349.
@dereuromark
Copy link
Contributor

That is an interesting one.
What should it do with adding another type foo on top of ?type?
Would it become ?type|foo or type|foo|null then?

I wonder if that normalization overhead makes sense now that we have the verbose way of using |null even within the actual types. Which could be the main way of declaring it moving forward?

@hemberger
Copy link
Author

The RFC for union types (https://wiki.php.net/rfc/union_types_v2) has a section on nullable types that explicitly addresses this issue. Specifically, it notes:

Union types and the ?T nullable type notation cannot be mixed. Writing ?T1|T2, T1|?T2 or ?(T1|T2) is not supported and T1|T2|null needs to be used instead.

Therefore, only ?T support would need to be added.

Personally, I prefer to use the ?T notation for my PHP types because it feels more succinct. Not being allowed to match that notation in the phpdoc adds to the cost of visually digesting the code.

Though I'm not involved in the development of PHP, I think it'd be more likely that the shorthand syntax gets expanded to union types in the future, rather than being replaced by the longhand syntax, as Nikita goes on to say:

I'm open to permitting the ?(T1|T2) syntax though, if this is considered desirable.

@dereuromark
Copy link
Contributor

dereuromark commented Sep 13, 2022

We could do the following

  • Allow docblocks to use ?T or T|null
  • Add a sniff that normalizes it as it so far does
  • You would silence this new sniff on project level (ignore)

This would keep the behavior while allowing a project (yours e.g.) to use your preferred way of documentation.
It would just touch quite a few sniffer rules I guess.

Would you be interested in providing a PR?

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

No branches or pull requests

2 participants