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

Mention nullable types in PSR-5 draft for phpdoc? (e.g. @param ?string $x) #153

Closed
TysonAndre opened this issue Oct 21, 2017 · 17 comments
Closed
Assignees

Comments

@TysonAndre
Copy link

TysonAndre commented Oct 21, 2017

Currently, the phpdoc2 standard says nothing about nullable types (?string), and would suggest writing it as string|null instead.
I don't see any open PRs or issues mentioning "nullable"

Questions:

  • Do you believe that nullables are something that belongs in the current or future phpdoc standard?
    (I find @param ?string $x much more convenient to use than @param string|null $x, and it's consistent with the way a real type would be written.)
  • Are you willing to make/accept contributions updating phpdoc.md?

Background:

Issues to consider when implementing this:

  • There would be an ambiguity in ?string[] that should be addressed in a standard. One way to resolve this is to parse it as ?(string[]), and require (?string)[] or (string|null)[] to indicate arrays of nullable strings.
    (And maybe to reject ??T without brackets)
@ashnazg
Copy link
Member

ashnazg commented Oct 5, 2018

Since nullable types are now part of the language syntax, I think it makes sense for the spec to add it as well. Having the ?string syntax in no way prevents historical use of string|null, so I see no deep concerns here.

Ping @ondrejmirtes @muglug @neuro159 @mindplay-dk @GaryJones @mvriel @jaapio for opinions.

@ashnazg ashnazg self-assigned this Oct 5, 2018
@jaapio
Copy link
Member

jaapio commented Oct 5, 2018

The current implementation of the type resolver that we wrote for phpdocumentor3 supports nullables. It is a language feature that should be available in the docblocks.
To be backwards compatible with older docblocks we do support both ?string and string|null als as array notation.

I don't see any issues to keep both in the standard format.

@muglug
Copy link

muglug commented Oct 6, 2018

Agree!

@rob006
Copy link

rob006 commented Oct 7, 2018

I don't see any issues to keep both in the standard format.

How does ?string|array or string|?array is interpreted?

@muglug
Copy link

muglug commented Oct 7, 2018

?string|array === string|null|array

@ashnazg
Copy link
Member

ashnazg commented Oct 7, 2018

In the case of x|y|z, if any one is nullable, then all have to be nullable.

So, ?x|?y|?z is super explicit.

But based on my first point, ?x|y|z should be good enough because one type can't be considered non-nullable if any other one is.

If this is all correct, then presumably we can establish the Nullable ? to only be allowed as the first character in the overall Union | string, and that it applies to all unioned types.

@neuro159
Copy link

neuro159 commented Oct 8, 2018

I'll make PhpStorm to accept ?x...
But... what shall IDE generate? )

@TysonAndre
Copy link
Author

what shall IDE generate?

I'd hope that implementations would accept any input (e.g. int|?string, and then silently convert it to their preferred format when showing it to the user, as long as that format has the necessary information)

Ideas:

  1. ?int|string is fine
  2. ?(int|string)
  3. int|string|null
  4. ?int|?string if being verbose (Remove null for inputs such as int|?string|null if one value is nullable)

Aside: Unfortunately, Phan currently represents it as int|?string for historic reasons and to make it easier for users to figure out why Phan does/doesn't emit an issue when analyzing the given code

@jaapio
Copy link
Member

jaapio commented Oct 8, 2018

I think the nullable? shouldn't be allowed as a solo type hint so ?|x|y is invalid. I do agree that ?x|y === ?x|?y === x|y|NULL

From that point of view I think it depends on the settings of the users project what a ide should generate. Pre php 7.1 should not use ? I do prefer the most explicit notation to be generated in the IDE. ?x|?y

However I'm not sure if phpstorm is already able to detect the multiple types? I can't remember since I don't like the multi type inputs. But that is up to you.

@neuro159
Copy link

neuro159 commented Oct 11, 2018

However I'm not sure if phpstorm is already able to detect the multiple types? I can't remember since I don't like the multi type inputs. But that is up to you.

@jaapio I was the one who had to invent them exactly because of "@return string or false" ppl used to write...

@TysonAndre braces - like this ?(int|string) are kind of overdoing it IMO.

@ashnazg
Copy link
Member

ashnazg commented Oct 11, 2018

On the surface, I'd agree that ?(int|string) seems like overkill.

However, it does visually remind the reader that the ? applies to each member in the (|) grouping. As such, I think I'd rather go ahead and standardize on that notation. At least it's shorter than ?int|?string, right?

@GaryJones
Copy link

GaryJones commented Oct 12, 2018

?int|?string is effectively WET code - the indication to accept a null value shouldn't be needed in multiple places (in the docs). So scrap this version.

At least it's shorter than ?int|?string, right?

Nope 😄

  • ?int|?string
  • ?(int|string)

Of ?(int|string) and ?int|string, I think I prefer the latter. Why? Because if I have ?int, and later decide to support a (nullable) string, then I only have to add |string, and not mess around adding ( and ) around it as well.

Likewise, if I have int|string, and decide it can be be a null value, then I only have to add the ? at the start, and not the parentheses.

So, I'd vote for ?int|string.

@ashnazg
Copy link
Member

ashnazg commented Oct 12, 2018

We somewhat have a () wrapper precedent already -- @return (int|string)[], where it implies the [] array marker applies to each possible union type inside (|).

@neuro159
Copy link

Thanks for bringing it out!
IMO what @GaryJones said and I support is also true for @return (int|string)[] - bit overkill...
We had to immediately open braces for internal symbolic type computation and it is bit harder to edit - plus NOT using () for different things (see doctrine style attributes and @method) usually makes parsing more robust..

@ashnazg
Copy link
Member

ashnazg commented Oct 15, 2018

(EDITED: operator listing)

Ok, so we're talking about using order of precedence with regard to operators. If we can use the same order in all places where the various operator characters are used, we can outline the precedence in an appendix.

  • | first
  • []
  • ?
  • ... last

We can probably highlight at the same time that using ? nullable operator means that explicitly listing |null becomes unnecessary.

@JanTvrdik
Copy link

I would strongly advice against relying on people remembering the correct precedence of most operators and instead suggest disallowing most ambiguous formats. For example all of the following are explicitly disallowed by PHPStan to avoid confusion.

  • ? string [] because it can mean either (null|string)[] or null|(string[])
  • A | B & C because it can mean either (A | B) & C or A | (B & C)
  • ? A & B because it can mean either (null|A) & B or null|(A & B)
  • ? A | B for consistency with ? A & B

The following is allowed for historical reasons

  • A | B [] even though it can mean either (A | B)[] or A | (B[])

(See ABNF grammar for more precise description)

@ashnazg
Copy link
Member

ashnazg commented Oct 15, 2018

@TysonAndre , if you wish to pursue this further, please bring this up as a new thread on the FIG mailing list for discussion -- https://groups.google.com/forum/#!forum/php-fig

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

No branches or pull requests

8 participants