Added type assertions for multiple methods not covered by existing @psalm-assert functionality

[Ocramius]

Related:

• Allow typing things as scalar&!empty? vimeo/psalm#2449
• non-empty-list type assertions are incorrectly considered redundant when list is encountered vimeo/psalm#2450
• @psalm-assert type assertions seem to ignore callable():T types vimeo/psalm#2451
• Type-assertions seem to be ignoring union types vimeo/psalm#2452
• @psalm-pure, : void methods (assertions) and exceptions vimeo/psalm#2456

[Ocramius]

Note: I do not know how to test @psalm-assert empty or !empty in combination with @param string or @param array. If anyone has suggestions on how to write an expression that infers as constant when one of the above applies, please let me know.

[Ocramius]

Note to self:

1. add "conflict": {"vimeo/psalm": "<3.7.3"} before calling this patch "done" (needs upstream release)
2. add type-checker tests for the above discussions

[Ocramius]

• Note: build will fail until Type-assertions seem to be ignoring union types vimeo/psalm#2452 has some sort of resolution (or revert here)

[Ocramius]

• Will still fail for now, as per Type-assertions seem to be ignoring union types vimeo/psalm#2452 (comment)

ERROR: TypeDoesNotContainType - tests/static-analysis/assert-email.php:9:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|string' to object
    Assert::email($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-email.php:16:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|object' to string
    Assert::email($value);


ERROR: RedundantCondition - tests/static-analysis/assert-email.php:16:13 - Found a redundant condition when evaluating $value and trying to reconcile type 'object' to !empty
    Assert::email($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ip.php:9:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|string' to object
    Assert::ip($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ip.php:16:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|object' to string
    Assert::ip($value);


ERROR: RedundantCondition - tests/static-analysis/assert-ip.php:16:13 - Found a redundant condition when evaluating $value and trying to reconcile type 'object' to !empty
    Assert::ip($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ipv4.php:9:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|string' to object
    Assert::ipv4($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ipv4.php:16:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|object' to string
    Assert::ipv4($value);


ERROR: RedundantCondition - tests/static-analysis/assert-ipv4.php:16:13 - Found a redundant condition when evaluating $value and trying to reconcile type 'object' to !empty
    Assert::ipv4($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ipv6.php:9:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|string' to object
    Assert::ipv6($value);


ERROR: TypeDoesNotContainType - tests/static-analysis/assert-ipv6.php:16:13 - Found a contradiction when evaluating $value and trying to reconcile type 'null|object' to string
    Assert::ipv6($value);


ERROR: RedundantCondition - tests/static-analysis/assert-ipv6.php:16:13 - Found a redundant condition when evaluating $value and trying to reconcile type 'object' to !empty
    Assert::ipv6($value);


------------------------------
12 errors found
------------------------------

The entire object|string scenario will always raise these if the type of $value is already known to be a string (see vimeo/psalm#2452 (comment)).

I don't have an immediate solution: will likely resort to dropping all @psalm-assert from ip* and email functions.

[Ocramius]

@BackEndTea I've added @psalm-pure where applicable. Don't think there are other issues with the patch, besides waiting for upstream psalm release.

[BackEndTea]

We still have the issue of type assertions being considered redundant by psalm: https://psalm.dev/r/33a97818c1

Another thing i'm not quite sure of is the effect of the psalm-pure annotations: https://psalm.dev/r/23518408b0 Because now we suddenly get a UnusedFunctionCall error in this example.

[Ocramius]

We still have the issue of type assertions being considered redundant by psalm: https://psalm.dev/r/33a97818c1

This one is indeed incorrect/to be removed, since having the types being constrained seemed more useful in the typical PHPUnit scenario:

/** @var T $a */
$a = someExpr();
$b = someOtherExpr();

Assert::same($a, $b);

$b->canNowCallSomethingOnThis();

I'll drop it though, since the same() will most certainly not be used that way (relying on $a is sufficient).

Would you consider sameType() as an alternate implementation that introduces the constraint?

Another thing i'm not quite sure of is the effect of the psalm-pure annotations: https://psalm.dev/r/23518408b0 Because now we suddenly get a UnusedFunctionCall error in this example.

Considering same($a, $b): void as "unused function call" seems weird upstream, since there is no function result by design (void signature). Will report there.

[BackEndTea]

I guess it is somewhat unusual for a pure method to have a void return type, because that usually would imply side effects, since you aren't getting a result from that.

[Ocramius]

Yeah, I'm raising an upstream issue: exceptions would be pure if it wasn't for the fact that they contain a stack trace.

In practice, any method that @throws in PHP is an Either ExceptionType ReturnType which affects all callers.

[Ocramius]

@BackEndTea I reported vimeo/psalm#2456 about :void unused return value

[BackEndTea]

I'd be okay with a sameType, but, that should probably be another PR.

The same example was more an indicator of a general problem, namely the 'side effect' assertions. Where the @psalm-assert assertion is a side effect of the actual assertion. An example would be the file assertion atm. it has an @psalm-assert string. Which means we get a redundant condition error when a string is passed in there.

To solve that we need to either remove them here, or we need a way to give psalm that information, without triggering the redundant condition error

[Ocramius]

@BackEndTea ok, will likely need to go through the assertions one-by-one and check them for redundancy.

An example would be the file assertion atm. it has an @psalm-assert string. Which means we get a redundant condition error when a string is passed in there.

We should probably remove the assert::string() bit from these then, and enforce the parameter to already be a string instead (BC break). Besides that, the issue is clear, and needs more work.

[Ocramius]

Just an update here: vimeo/psalm@4b715cd fixes @psalm-assert + @psalm-pure + void being considered a RedundantCondition

[BackEndTea]

I do have a concern about using @psalm-pure.

We don't analyse the Assert class with psalm yet, otherwise it would error on the reportInvalidArgument not being marked as pure.

But marking it as pure prevents our users from overriding that method and doing impure things, e.g. any kind of logging they may do from that class. But not marking it and leaving it as is, allows our users to do that, but it also means that we are lying about the methods being pure.

In all honesty i'm not sure what the best solution would be.

[Ocramius]

But marking it as pure prevents our users from overriding that method and doing impure things, e.g. any kind of logging they may do from that class.

Few things to consider:

1. If users have a subtype of their own, and they don't inspect it, they'll be fine for now, even if effects are introduced (because they didn't care about purity).
2. users that care about the functional purity of the API and relying on Assert::someMethod() (static reference) will have an execution path reaching Assert::reportInvalidArgument() (rather than Subclass::reportInvalidArgument()) so we're fine here.

Yes, this means that the library won't pass any sort of internal SA unless either:

1. static:: calls are replaced with self::calls
2. the Assert class is made final (not sure why one would extend it)

Consumers will be able to ignore this though, unless they subclass (which probably already has broken SA).

The static::reportInvalidArgument() (just noticed it as well, sorry) seems like a really bad hack to me: it jeopardizes stability of this class to allow DI via inheritance for very rare scenarios (logging is usually best done in the application layer).

[Ocramius]

@BackEndTea would you be OK with deprecating reportInvalidArgument()?

[BackEndTea]

I have to think about that. I'm not sure how many people make use of it.

[Ocramius]

Note: I'll also add this library to upstream psalm test suite, once through 👍

[tux-rampage]

I've added extensive tests (sorry @tux-rampage if we're duplicating work :S ) for all of this, and restricted @psalm-pure to API that is really pure.

No worries. I compared both branches and you already addressed a lot more here. When this one gets merged #167 can be considered obsolete. I don't mind how pure/immutable supports makes it into this lib as long as it does ;-)

[tux-rampage]

You didn't mark Assert::ip*() as pure. Is there a reason why not?

[BackEndTea]

I got a few comments/questions. When those are resolved this should be good to merge.
I'm not sure if i can release this as a patch version as it changes the version constraints on psalm.

[Ocramius]

@BackEndTea all done: let's see if CI passes, then we should be good to go 👍

[BackEndTea]

Found a few more things, didn't think of these edge cases being impure, but the filter_var based assertions are impure, so these should be as well

[Ocramius]

@BackEndTea adjusted according to comments above about purity: good looking out!

[BackEndTea]

Thanks for the work on this

[Ocramius]

W00P W00P! Thanks for the very careful review, @BackEndTea!

[tux-rampage]

🎉 Thanks for all the psalm improvements @Ocramius

[Ocramius]

@BackEndTea could you please push a 1.8.0 tag with this?

[Ocramius]

Or better: revert the merge on `master, then merge without squash? :S