Types (re) and static analysis.

Hello

I realize this has been discussed before, but, after recently discussing this with a colleague, have some additional questions regarding PSR-5's stance on this. Take the following example:

/**
 * @param string $bar
 */
function foo($bar) { ... }

foo(5);

In this example, I'm not passing a string as parameter. Is this in violation of the standard? Should I be explicitly casting 5 to a string? The documentation states that I should be passing a string and that is what the function's internals will be expecting, but I'm actually passing an integer. Due to PHP's type coercion, if I use the passed in number as a string in the function body, things will probably work out fine. The same applies here:

declare(strict_types=0);

/**
 * @param string $bar
 */
function foo(string $bar) { ... }

foo(5); // Just fine, int will be coerced to string.

However, things are not so fine and dandy when I'm using strict typing:

declare(strict_types=1);

/**
 * @param string $bar
 */
function foo(string $bar) { ... }

foo(5); // Oops!

For PHP < 7, I've always seen type declarations such as @param as a way to strictly specify the type of a parameter in a way that wasn't possible in the language itself. Since PHP 7, we do however have scalar type hinting for this and it naturally seems to complement PSR-5.

If PSR-5 takes the stance that loose coercion is simply allowed in the cases above and the first example is correct, does this then also apply to the following case?

/**
 * @param string $bar
 */
function foo($bar) { ... }

foo(null);

I did not indicate that $bar is nullable, but null coerces to a string in the sense that you can use it in situations where strings are expected (such as concatenations), so is the string|null indication now obsolete? One could argue that no, that is not the case, since with PHP 7 that is also not the case:

/**
 * @param string $bar
 */
function foo(string $bar) { ... }

foo(null); // Error in both strict and non-strict mode.

So this makes me think that docblocks should be strict about the type and explicitly state the types they allow, in this case string|null. But now we are in violation of our first conclusion: we were passing an int that loosely coerces to a string, which is wrong, since int is not an explicitly allowed type in our docblock, i.e. it says string|null, not string|int|null or even mixed.

This brings me to the final question: how strict are docblocks regarding typing? Are they interpreted in the same way in strict (PHP 7) and non-strict typing mode? If not, will we have to update all our docblocks if we transition from one mode to another in a file? Do they perhaps follow the strict typing mode of the file?

The most important reason for asking this question is with regards to tools and IDE's: If we leave this open for interpretation by tools doing static analysis for types, we will get varying results between them, and if we get varying results and the types are still open to interpretation, what good is specifying them at all?

[cebe]

Imo PHPDoc annotation is not meant to enforce types on the parameters but to document the kind of parameter that the programmer should pass. If you pass something that does not comply with the documentation there is no way to throw an error based on phpdoc. if you want to enforce types there are strict typing methods which come with php 7 but I do not think this relates to phpdoc much.

[krzysiekpiasecki]

Good argument @cebe and additionally the context of strict_types is created by the caller.

@cebe I tend to agree: PHP docblocks are not physical code and thus there is no actual kind of validation. However, take the following example:

function internalFunction($foo)
{
    if (is_string($foo)) {
        // ...
   } elseif (is_array($foo)) {
        // ...
   } /*else {
        // The world explodes. 
   }*/
}

/**
 * @param string $foo
 */
function publicFunction($foo)
{
    // Do something.

    internalMethod($foo);

    // Do some other things.
}

Imagine both of these methods are part of a framework. The docblock indicates it wants a string, and numbers implicitly coerce to a string. To prevent the world from exploding, I really need that parameter for publicFunction to be a string. The caller has no knowledge of the internals of the framework and does not know what will happen if he doesn't pass a string (granted, he'll notice soon enough if he tries!). Should I be catching this in my framework and be adding a $foo = (string) $foo; or something similar at the start of my method? That is a possibility, which is not unlike strict typing with PHP 7 where the caller would be casting the argument to a string before passing it. The advantage would be that if I switch to PHP 7 I don't need to do anything else than add the scalar type hint and remove the cast, which all stays confined to the framework, i.e. user code doesn't need to care about it.

As a counter-point: aren't docblocks meant to document that which was impossible to enforce with type hints before PHP 7 (and still mostly is, as things such as union types or nullable return types still don't exist in PHP 7)? I always felt like the following was supposed to be equivalent:

/**
 * @param string $foo
 */
function prePhp7($foo) {}

function php7(string $foo) {}

To make matters worse, the same also applies to the return type:

/**
 * @return string
 */
public function publicFunction()
{
    if (...) {
        return 'Hello';
    }

    return 5;
}

In my client code I'm now really expecting the output to be a string, but the developer of the framework method never documented it could also be returning an integer. An integer implicitly coerces to a string, and the docblock isn't a contract, so this is fine, right? Well, it could be seen as such, but at this point the documentation isn't entirely accurate, as it's not returning a string, it's returning more of a 'stringlike'. I still can't be sure I will be getting a string if I call this method, I could even be getting an object that implements the __toString() method.

Previously there was the argument that docblocks are not a contract. But.. aren't they? They are not in the sense that there won't be any actual validation by the language if you don't abide by them, but they are in the sense that if you don't pass in the types that the docblock is specifying, you know that all bets are off at that point and calling the function or method may result in undefined or unexpected behavior.

[cebe]

well, your last example is simply a mistake in the documentation, it should just say:

/**
 * @return string|int
 */
public function publicFunction()
{
    if (...) {
        return 'Hello';
    }

    return 5;
}

If you use undocumented features of a function you need to be careful, if it expects only string then do not pass anything else. If you need strict contract enforcement you have to either check input type or use php 7 features. phpdoc is not made for enforcing interface contracts, it is for documenting intended behavior.

@cebe Ok, so that would mean that if I specify a @param string, callers should be passing in a string to this function (i.e. the function expects its input parameter to pass is_string). The moment they pass in something else, such as an int, even if it coerces implicitly, the behavior of my method is no longer guaranteed as I didn't list that as being supported. So that would also mean that an IDE such as PHPStorm or a static analysis tool could be showing me a strictness warning when I do this in PHP 5 (it wouldn't surprise me if the former already did that with scalar type hinting in PHP 7):

// Library:
/**
 * @param string $foo
 */
function foo($foo) {}

// Client:
foo(5); // WARNING: Function 'foo' expects string, int given.
foo((string) 5); // Ok, the caller ensures what is passed in is a string.

This seems the most logical out of the possible options. Also, seeing as in PHP 7 the caller decides strict typing, it also seems to elegantly match scalar type hinting:

// Library:
function foo(string $foo) {}

// Client:
declare(strict_types=0);

foo(5); // Ok, type coercion is explicitly allowed, foo is using a scalar type hint,
        // so the parameter being a string is guaranteed without the need for casting.

That does seem to answer my original question and was the answer I was looking for: it is indeed not a runtime enforcement, but it documents intended behavior, thus any allowed input types (even if they could otherwise coerce implicitly), should be mentioned explicitly.