Support PHP 8's mixed type

[carusogabriel]

PHP 8 has a new type: mixed as per this RFC.

@jrfnl @gsherwood It shouldn't be that difficult to add support, right? Mind to guide me for this one? 😄

[jrfnl]

@carusogabriel Yup, that was already on my radar, as well as union types and the static return type.
I just hadn't pulled it yet as I haven't got a working version of PHP 8 locally to verify how PHP 8 tokenizes this.

File::getMethodParameters() needs adjusting for sure, but depending on how its tokenized, the PHP tokenizer may need some changes too.

Want to work on this yourself or would you like me to finish this off ? In which case, I'd be looking for a PHP 8 download for Windows to test with. Would you happen to know where I can find one ?

[carusogabriel]

Want to work on this yourself or would you like me to finish this off ?

I want to give it a try, but if I get stuck or something, I'll let you know!

In which case, I'd be looking for a PHP 8 download for Windows to test with. Would you happen to know where I can find one ?

@cmb69 Do you know where we can find it? We don't keep a master version out there (https://php.net/downloads.php), right?

[cmb69]

There are master snapshot builds at https://windows.php.net/downloads/snaps/master/.

[jrfnl]

@cmb69 Thanks for the link! That's useful.

@carusogabriel Let me know how you get on and if you want me to look something over.

Once the initial changes are in (File::getMethodParameters() and potentially tokenizer), unit tests will need to be added to various sniffs which use the method and/or register the T_FUNCTION/T_CLOSURE/T_FN tokens to see if they need additional changes or if the sniff will handle things correctly as-is.

Depending on how involved the initial changes are, I'd normally wait with making changes to sniffs to get the initial changes approved & accepted, cause if something would be changed in the chosen implementation, the sniff changes might be affected.

Have I given you enough to go on with that ?

[carusogabriel]

@jrfnl Yeah, thank you very much. I'll try to come up with something during the following days, I keep this issue updated

[carusogabriel]

@jrfnl Sorry for the late noticed, but I didn't find time to work on this 😕

Feel free to close this issue or assign it to someone else.

[jrfnl]

@carusogabriel I'm not an admin here, so can't assign or close issues.

Aside from that: thanks for letting me know. I had been wondering how you were getting on. I'll see if I can find some time over the next few weeks to sort it out. The constructor property promotion also needs addressing in the same function.

[jrfnl]

Methods involved:

• File::getMethodParameters()
• File::getMethodProperties()
• File::getMemberProperties()

Status update:

• Verify support for "mixed" type.
  Verified in all three relevant methods and added unit tests to safeguard support. Will pull these soon.
• Add support for union types.
• Add support for constructor property promotion
• Verify that sniffs using the methods take the PHP 8 features into account (if necessary).

[jrfnl]

Ok, so I'm running into a decision point - @gsherwood please weight in on this:

Type mixed includes the pseudotype null and is therefore automatically "nullable", even though the type cannot be combined with the nullability operator ? (or rather, that is the reason why it can't be combined).

Similarly, with union types, the nullability operator cannot be used and an explicit int|float|null should be declared.

function foo(mixed $var) {}
function foo(int|float|null $var) {}

A decision is needed on how to handle this for the nullable_type array index.
The documentation states that the array index indicates "TRUE if the var type is nullable."

In my perspective, there are two possible approaches:

1. Functional: if the type mixed is used or if null is used as part of a union type, the parameter is by definition nullable, so nullable_type should be set to true.
2. Technical: nullable_type is only set to true if the nullability operator ? is used.

I'm leaning towards option 1, the functional approach, but would like a second/third/fourth opinion on this.

Other considerations:
It could be argued that, using the same reasoning, when no type is given and the variable has a = null default value, the nullable_type array index should be set to true as well.

function foo($var = null) {}

In my opinion, this is not the same thing as a "type" can only be nullable if a type was declared.

Refs:

• https://wiki.php.net/rfc/mixed_type_v2#nullability
• https://wiki.php.net/rfc/union_types_v2#supported_types

[gsherwood]

The nullable_type array index was added so sniffs know there is a nullability operator is use. It's used exactly in this way in the core, where Squiz.Functions.FunctionDeclarationArgumentSpacing injects the operator into the error message if it is in use.

Note that the same behaviour is also present in the getMethodProperties() helper as well, which has a nullable_return_type array index. That's used in PSR12.Functions.ReturnTypeDeclaration to know that a T_NULLABLE token exists somewhere before the type, so the sniff goes looking for it.

This functionality must remain in version 3 as it would be a significant BC break. I get that the functionality of PHP is changing and the array index name is a bit vague, but it means something very specific in the current release.

Version 4 is where we could make changes to this, although I'm more inclined to change the name of the array index (or do nothing) than to change its meaning. I'd need more use cases of where it is important for a coding standards checker to know that a param would accept null, or that a method may return null, so I know if it is worth even returning this. It makes perfect sense for static analysis tools that are checking for coding errors, but probably less so for formatting errors.

[jrfnl]

@gsherwood Ok, thanks for pitching in. That means in effect that I'll need to update the documentation of the nullable_type property to make it very clear that it indicates whether the nullability operator was used, not whether the type is nullable.

On another note - how would you prefer I pull the above mentioned PHP 8 related changes to the utility method (File::getMethodParameters(), File::getMethodProperties() and File::getMemberProperties()), including some related tokenizer changes:

• One PR with everything (in a series of commits)
• One PR per utility method, but with all these PHP 8 changes for that method.
• One PR per PHP 8 feature covering all the affected utilities ?

I have made the changes locally in a series of commits, so I can split them out in various ways or pull them in one go, whatever you prefer.

[jrfnl]

I've pulled the changes for supporting mixed and union types per method now as PRs #3009, #3010, #3011.

The changes for supporting constructor property promotion will be pulled separately once #3009 has been merged.

[jrfnl]

Update: I've now also checked which sniffs would need adjusting for union types and could only find two for which this applies:

• Generic.PHP.LowerCaseType - I've made all necessary adjustments to that sniff and will pull these once the above mentioned open PRs have been merged.
• Squiz.Commenting.FunctionComment - I'm going to leave that one for someone else as that sniff is out of date regarding types anyhow. This would have been fixed with the refactor of the utilities, but as that wasn't merged, I can't be bothered to do the same work all over again.

[jrfnl]

I've prepared the necessary changes for supporting constructor property promotion. These can be pulled once #3009 has been merged.

[jrfnl]

@gsherwood I've been thinking about the union types a little more and I wonder if it wouldn't be better to change the T_BITWISE_OR token used in union types to a T_TYPE_UNION token or something along those lines.

Reason being that any sniff which currently looks for (bitwise) operators and adjusts the spacing around them would possibly start changing the spacing in union types too, while most of the time, that is not the intention of those sniffs.

What do you think ?

[gsherwood]

I wonder if it wouldn't be better to change the T_BITWISE_OR token used in union types to a T_TYPE_UNION token or something along those lines

I think you're right.

[jrfnl]

@gsherwood Thanks for getting back to me. In that case, it will probably be better to split the PRs on feature rather than on utility method, like I've done now, as all three will need to take the new token into account. I will close the currently open PRs and replace them.

First PR will then add tests for mixed type (already works, no extra changes needed). - See PR #3018
Second PR (after the first is merged) will add support for union types, including the tokenizer changes.
Third PR will add support for constructor property promotion (to getMethodParameters() only).

[jrfnl]

FYI: I've prepared all the tokenizer changes for converting T_BITWISE_OR to T_TYPE_UNION. Once #3018 and #3019 have been merged, these can be pulled.

[jrfnl]

Re: sniffs which need checking - for constructor property promotion, sniffs which examine namingconventions for properties and function parameters will probably need adjusting.