phpDocumentor / fig-standards

Standards either proposed or approved by the Framework Interop Group
http://www.php-fig.org/
Other
361 stars 85 forks source link

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

Closed TysonAndre closed 5 years ago

TysonAndre commented 6 years ago

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:

Background:

Issues to consider when implementing this:

ashnazg commented 5 years ago

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.

jaapio commented 5 years ago

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 commented 5 years ago

Agree!

rob006 commented 5 years ago

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

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

muglug commented 5 years ago

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

ashnazg commented 5 years ago

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 commented 5 years ago

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

TysonAndre commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

?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 😄

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 commented 5 years ago

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

neuro159 commented 5 years ago

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 commented 5 years ago

(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.

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

JanTvrdik commented 5 years ago

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.

The following is allowed for historical reasons

(See ABNF grammar for more precise description)

ashnazg commented 5 years ago

@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