Phpinspectionsea: Property should declares @type

[rentalhost]

Following #252. After declares a phpdoc on property, you should declares the @type property.

// Warning: property @type was not declared here.
/**
 * It's a property.
 */
public $property;

[rentalhost]

For lasts #254, #253 and #252, if no one work on that, I'll try on weekend. @fabioi, can you confirm if that is related or exactly what you have described on #250?

[fabioi]

Hi yes it seems related thanks :-). Fields / property should always have a phpdoc which specify the type

[funivan]

We can validate property type.
For example:
@var mixed <- invalid
@var string|int <- invalid
@var string|null <- valid
@var null <- valid

The main feature is to have only single type or null. No mixed types. What do you think?

[rentalhost]

I guess very valid and really useful, but maybe it should be part of a new issue. Can you do open that?

[funivan]

It can be done with the checkbox in the confirmation panel. I think this feature is related to this inspection. If it is important I can create new issue.
@kalessil how to deal with this situation. New issue for each feature?

[rentalhost]

I mean that because it could not be related specifically to properties, but to parameters or variables. For instance, the next example should be warn.

/** @param string|int|null $myParameter */
function myFunction($myParameter = null) {
}

[funivan]

Hmm. Interesting. I think this is bad idea. Check properties and method arguments should be placed into the different​ inspections. And here is why:

1. Check property inspection:
   1.1 warn if empty type
   1.2 warn if invalid type (aka mixed)
   1.3 if type is valid and version is > 7.0 then add QF (add strict type to the property) in php7.1 we can do that.

1. Check argument inspection:
   2.1 check if mixed
   2.2 if argument has single type in phpdoc and argument definition doesn't. Then we can create QF (like return type QF)

[rentalhost]

About 1.3, you mean this rfc?

I guess that it should be splitted into two issues. One is that current (#254), that will treat only if @type was defined. Another is check if @type (or @var) and @param have mix types as you mentioned.

• 1.1: could be check from this issue (if is _undefined_ or _empty_);
• 1.2: should be checked by _new feature_ (read as _new issue_);
• 1.3: I guess that is not possible currently, but could be QF by _new feature_ if I am wrong;
• 2.1: should be checked by _new feature_, as in 1.2;
• 2.2: should be done by _another feature_ (a third one), maybe;

[funivan]

You are correct, i will create new issue.

P.S. It's my bad, i think that properties can have strict type declaration.

[rentalhost]

No problem!

I feel so bad when I see that RFC declined. :( I guess that the internals don't like too much of changes on properties. haha

[funivan]

Hmm. Interesting. I think this is bad idea. Check properties and method
arguments should be placed into the different​ inspections. And here is why:

1. Check property inspection:
   1.1 warn if empty type
   1.2 warn if invalid type (aka mixed)
   1.3 if type is valid and version is > 7.0 then add QF (add strict type to
   the property) in php7.1 we can do that.

1. Check argument inspection:
   2.1 check if mixed
   2.2 if argument has single type in phpdoc and argument definition doesn't.
   Then we can create QF (like return type QF)

On 17 Apr 2017 3:40 p.m., "David Rodrigues" [e-mail]
wrote:

I mean that because it could not be related specifically to properties, but
to parameters or variables. For instance, the next example should be warn.

/** @param string|int|null $myParameter */function
myFunction($myParameter = null) {}

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/kalessil/phpinspectionsea/issues/254#issuecomment-294477389,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAZ8-N_lxLGegmw1sFCUhb8D1KDiq9hRks5rw13VgaJpZM4M-zDX
.

[Fleshgrinder]

The _typed properties_ RFC was not accepted because the proposed implementation was not good enough, not because the feature per se is not wanted. 😉

Do you plan to activate this inspection by default? Not sure if I like it. There are various situations where mixed types are required (e.g. uint64), and even more where strict types are actually unwanted.

[rentalhost]

The mixed still will works for cases like that. But is a good pratice avoid mixes types where it is not really need.

[Fleshgrinder]

Absolutely, it is, however, using mixed instead of int|string is definitely not a good idea. People run into float very fast if they are not paying attention due to type coercion. Ideally the IDE and/or extension would be able to follow that, and warn about it. Instead there are still many situations where PhpStorm is not capable of understanding PHP.

That being said, go further with it. It should be helpful for many, and that's what counts. 😉

[davidmpaz]

I had always the feeling that having mixed type was some kind of design problem in APIs. After reading you @Fleshgrinder

... using mixed instead of int|string is definitely not a good idea. People run into float very fast if they are not paying attention due to type coercion...

I feel i should read more about it. Could you please point out to some resources or give some more examples when having mixed is needed?

It will helps to keep the whole concept in docs later on. Whatever the desicion is taken about to keep it or discard it.

[Fleshgrinder]

Well, as I said, mixed is definitely bad. mixed is PHP’s supertype and basically says that it can contain absolutely everything. More constrained union types like int|string are perfectly valid, and sometimes even required. The best example for this are uint64 values which we cannot represent in PHP with an int type because it is always signed. However, it is very rare to encounter such values, and probably not worth to derail this otherwise useful inspection for a few edge cases. People who know what they are doing will be able to place appropriate _ignore inspection_ comments.

[stale[bot]]

Bebo beep, the StaleBot is here. For one year nothing have happened here. It would be great if someone looked into details here within next 21 days when I'll close it.

[kalessil]

Will not happen.