Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@psalm-pure, : void methods (assertions) and exceptions #2456

Closed
Ocramius opened this issue Dec 11, 2019 · 5 comments
Closed

@psalm-pure, : void methods (assertions) and exceptions #2456

Ocramius opened this issue Dec 11, 2019 · 5 comments

Comments

@Ocramius
Copy link
Contributor

Further follow-up from work being done to improve webmozart/assert @ webmozarts/assert#160 (comment)

@psalm-pure doesn't seem to play well with :void expressions. There are a few things weird about the following code (https://psalm.dev/r/e34695a167):

<?php

/** @psalm-pure */
function stopOn1(int $v):void
{
  if ($v === 1) {
    throw new \RuntimeException();
  }
}

stopOn1(1);
stopOn1(2);
stopOn1(3);
Psalm output (using commit af52590):

ERROR: UnusedFunctionCall - 11:1 - The call to stopon1 is not used

ERROR: UnusedFunctionCall - 12:1 - The call to stopon1 is not used

ERROR: UnusedFunctionCall - 13:1 - The call to stopon1 is not used

Specifically:

I think this is connected with #2160 (comment): thrown values are not considered function output.

Few possible resolutions:

  • ignore UnusedFunctionCall if that result is void
  • consider exceptions as return signature in all callers (this sounds like a mess, as it changes basic assumptions about the type system)
@muglug
Copy link
Collaborator

muglug commented Dec 11, 2019

What if we ignore UnusedFunctionCall when the function has @psalm-assert?

@weirdan
Copy link
Collaborator

weirdan commented Dec 11, 2019

To me, :void indicates the return value should not be used, so why should Psalm complain about unused return? Or am I misunderstanding the nature of UnusedFunctionCall?

@weirdan
Copy link
Collaborator

weirdan commented Dec 11, 2019

Oh, I think I get it now: pure functions should return values that are used, otherwise calls to them are deemed useless.

@Ocramius
Copy link
Contributor Author

What if we ignore UnusedFunctionCall when the function has @psalm-assert?

Seems reasonable.

@muglug muglug closed this as completed in 4b715cd Dec 11, 2019
@Ocramius
Copy link
Contributor Author

Thanks @muglug! 🎉

Ocramius added a commit to Ocramius/assert-1 that referenced this issue Dec 11, 2019
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Feb 18, 2020
BackEndTea pushed a commit to webmozarts/assert that referenced this issue Feb 19, 2020
…psalm-assert` functionality (#160)

* Added type assertions around `Assert::isMap()` to restrict key type

* Added minimal type assertions around `Assert::stringNotEmpty()`

* Added minimal type assertions for `Assert::isArrayAccessible()`

* Minimal type assertions around `Assert::ip()` type inference

* Minimal type assertions around `Assert::ip()` type inference

* Added minimal type assertions for `Assert::email()`

* Added type assertions for `Assert::same()` types

* Corrected type declaration: `Assert::contains()` only works with strings

Note: not a BC break, since the implementation uses `\strpos()`, which
is designed to reject non-string parameters.

* Corrected type declaration: `Assert::notContains()` only works with strings

Note: not a BC break, since the implementation uses `\strpos()`, which
is designed to reject non-string parameters.
* Corrected type declaration: `Assert::notWhitespaceOnly()` only works with strings

Note: not a BC break, since the implementation uses `\preg_match()`, which
is designed to reject non-string parameters.

* Corrected type declaration: `Assert::startsWith()` only works with strings

Note: not a BC break, since the implementation uses `\strpos()`, which
is designed to reject non-string parameters.

* Added type assertions for `Assert::startsWithLetter()`

* Corrected `Assert::endsWith()` parameter type: only works with `string` values anyway

* Corrected `Assert::regex()` parameter types: only works with `string` values anyway

* Corrected `Assert::notRegex()` parameter types: only works with `string` values anyway

* Added type assertions on `Assert::unicodeLetters()`

* Added type assertions on `Assert::alpha()`, `Assert::digits()`, `Assert::alnum()`, `Assert::lower()`, `Assert::upper()`

* Corrected `Assert::length()` types - method only works with strings/integers

* Corrected `Assert::minLength()`, `Assert::maxLength()`, `Assert::lengthBetween()` type declarations

These methods only work with integers, so we should restrict their types.

* Added type assertions for `Assert::fileExists()`

* Added type assertions for `Assert::file()` and `Assert::directory()`

* Corrected type declarations on `Assert::readable()`: works only with `string` values

* Corrected type declarations on `Assert::writable()`: works only with `string` values

* Added type assertions for `Assert::subclassOf()`

* Added type assertions for `Assert::implementsInterface()`

* Corrected type declarations for `Assert::count()`

* Corrected type declarations for `Assert::minCount()`, `Assert::maxCount()` and `Assert::countBetween()`

These methods only work with countable types

* Verifying that `Assert::isList()` preserves list item types

* Adding type assertions for `Assert::isNonEmptyMap()`

* Corrected type declarations around `Assert::uuid()` - method only works with `string` parameters

* Corrected type declarations for `Assert::throws()` - the method only works with `Throwable` class strings

* Applied automated CS fixes

* Skip static analysis tests for scenarios that require an upstream release of psalm first

* PHP 5.3 COMPATIBILITY OMG NGHHHHHHHHHHHHHHHHH LOUD TEAPOT NOISES!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

* Added `@psalm-assert !empty` assertions as per newer multiple-assertion upstream support

Ref: vimeo/psalm@74de32f

* Locking dependency of `vimeo/psalm` for the `ci` directory

We don't want to run static analysis with constantly changing psalm
semantics, as that jeopardizes the stability of CI for now

* Disallow installation of `vimeo/psalm` `<3.7.3` together with `webmozart/assert`

This is required due to upstream support of multiple `@psalm-assert`
annotations. Specifically, we need support that was introduced in
commit vimeo/psalm@74de32f

* `filter_var()` accepts `string|object`, so the input is asserted to be `string|object`

An object implementing `#__toString()` suffices for `filter_var()` to
work, so we can't assume that after the assertion, the value being
asserted upon is a `string`. Instead, `string|object` is our closest
bet.

Ref: #160 (comment)

Note: will fail until vimeo/psalm#2452 has some
sort of resolution.

* `ctype_*()` functions can operate with integer ranges `-128~256` too

These are extremely fucky interfaces, but PHP is a shitty language anyway.

We need to support an input of `string|int` when validating for the
`ctype_*()` of an input, but at least we can assert for the value to
not be `empty()` when it passes such validation.

Ref: vimeo/psalm#2452

* Bump vimeo/psalm CI dependency to try out results of vimeo/psalm#2452 discussion

* Removing `@psalm-assert string|object` from `ip`, `ipv4`, `ipv6` and `email` validators (for now)

Psalm currently *cannot* deal with union types in `@psalm-assert`, because all `@psalm-assert` expressions
are implicitly inlined by the tool as `assert(type)` language expression.

This means that `@psalm-assert string|object $foo` becomes `assert(is_string($foo) || is_object($foo))`,
which is non-sensical in contexts where `$foo` may never be a `string` or an `object`, leading to impossible
to avoid `TypeDoesNotContainType` or `RedundantCondition` reported psalm errors.

Therefore, dropping the type assertion is a simplistic (for now) solution, hoping that upstream tooling will
someday be able to deal with this sort of assertions in a more atomic manner.

Ref: vimeo/psalm#2452 (comment)
Ref: vimeo/psalm#2452 (comment)

* More precise type-assertions around `!empty` for `Assert::alpha()`

* Exclude `/ci` dir from package release (now includes a larger `composer.lock` that is otherwise downstream waste)

* Marking pure API with `@psalm-pure` for usage in pure downstream consumers

* Ensure that the lack of usage of the result of a pure operation such as `Assert::string()` will not be considered "redundant"

Ref: vimeo/psalm#2456
Ref: vimeo/psalm@4b715cd
Ref: #160 (comment)

* Marked `Assert::reportInvalidArgument()` as `@psalm-pure`

While this method can be replaced via subclassing, adding side-effects to it would break the basic
invariants of the assertion library, specifically that it is *NOT* a hook for more business logic,
but rather just assertions, pure where possible.

* Simpler type tests for `Assert::subclassOf()`, which can lead to a union type result

* Added type restrictions on `Assert::isAOf()` API

* `Assert::same()` is not supposed to couple two types, since the types may already be the same

If the types are already the same, then the type checker would expand the assertion into a redundant
condition (unexpected)

* `Assert::isNotA()` type restrictions added

* `Assert::isAnyOf()` is pure, declared type restrictions

* `Assert::isInstanceOfAny()` is pure, declared type restrictions

* Overhauled static analysis tests, hunted down all `RedundantCondition`

All imprecise `RedundantCondition` raised by `@psalm-assert` restricting on something that may have
already been restricted have been hunted down with extensive type-specific tests.

All methods that may not necessarily be pure have also been removed from purity annotations.

* Conflict with anything but latest `vimeo/psalm`, which understands the `lowercase-string` type

Ref: https://github.com/vimeo/psalm/releases/tag/3.9.0

* `Assert::isMap()` does not imply non-empty array anymore

Ref: #160 (comment)

* Removed integer value support from `ctype_*`-based functions

While these functions support integers, we do not necessarily intend
downstream consumers to rely on the widened type.

Ref: #160 (comment)

* Removed exclusions from static analysis test existence checks

All functions that have `@psalm-assert` annotations now have static
analysis tests.

Ref: https://github.com/webmozart/assert/pull/160/files#r381317558

* Allow both `int` and `float` on limits for count-based assertions

Ref: #160 (comment)

* `Assert::eq()`, `::notEq()` and `::uniqueValues()` are not pure due to `__toString`

In fact, the following explains it:

```php
'foo' == new class { public function __toString() : string {echo 'hi'; return 'bar'; }};
```

This is a fucking mess of a language, but it's what we have to deal with :-(

Ref: #160 (comment)
Ref: #160 (comment)
Ref: #160 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants