Page MenuHomePhabricator

Create a phan plugin to handle the Assert library
Open, Needs TriagePublic

Description

Phan can already handle PHP native assertions, so if you have this code

$x = getUnknownType();
assert( $x instanceof MyClass );

phan will know that $x is instance of MyClass.

However assert() cannot be used in MediaWiki, and we have the Assert library for that need. Thus, it'd be great to create a plugin to tell phan how our Assert::* functions behave. At least some of them are easy, e.g. Assert::parameterType.

Event Timeline

Of note, the code should be similar to phan's analyzeAssert in PostConditionsVisitor (here). I believe we'd have to adjust it to 1-work inside a plugin 2-read our Assert syntax when it's different from assert's (e.g. Assert::parameterType). In theory, it shouldn't be too hard.

Unrelated, I wonder why AssertException is extending Exception. If Assert:: calls are meant to be real assertions, i.e. something that people are not supposed to catch (even unintentionally, if they end up being inside a try{...}catch (Exception $e) block), maybe extending AssertionError would be better.

I am not sure if @phan-assert on the lib code would be read by phan

https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#assertions

  • Assert::precondition @phan-assert-true-condition $condition
  • Assert::parameter @phan-assert-true-condition $condition
  • Assert::parameterType This may use a @phan-template to declare the dependency between the first and the second arg. This may not work when the type is an array or a list of |-separated types
  • Assert::parameterKeyType May also use template
  • Assert::parameterElementType May also use template
  • Assert::nonEmptyString nothing needed, because phan only checks types
  • Assert::postcondition @phan-assert-true-condition $condition

I have nothing tested, just want write it down

I am not sure if @phan-assert on the lib code would be read by phan

Interesting idea! I tested your examples, and they work!

  • Assert::precondition @phan-assert-true-condition $condition
  • Assert::parameter @phan-assert-true-condition $condition
  • Assert::postcondition @phan-assert-true-condition $condition

and also Assert::invariant.

  • Assert::parameterType This may use a @phan-template to declare the dependency between the first and the second arg. This may not work when the type is an array or a list of |-separated types
  • Assert::parameterKeyType May also use template
  • Assert::parameterElementType May also use template

I don't know if templates already allow to do that, but as you said, if there are |-separated types that's not going to work anyway. But we can leave it for later.

  • Assert::nonEmptyString nothing needed, because phan only checks types

Indeed

Change 539557 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/libs/Assert@master] Use @phan-assert annotations

https://gerrit.wikimedia.org/r/539557

The wiki page says

This can be used in combination with Phan's template support.

Change 539557 merged by jenkins-bot:
[mediawiki/libs/Assert@master] Use @phan-assert annotations

https://gerrit.wikimedia.org/r/539557

So it turns out this feature adds all sorts of false positives to the Parsoid code base, where we use code like:

switch($c) {
case 1: ....;
case 2: ....;
default:
    Assert::invariant(false, "Should never happen!");
}

a lot to indicate unreachable code paths.

After we upgrade to wikimedia/assert 0.5.0 each one of these triggers:

src/Utils/PHPUtils.php:227 PhanImpossibleCondition Impossible attempt to cast false of type false to truthy

Yeah, that's the point!

Perhaps we need a new Assert::unreachable('should never happen') type call.

cscott renamed this task from Create a phan plugin to handle the Assert library to Add Assert::unreachable with appropriate phan annotations.Mar 6 2020, 3:24 PM
cscott updated the task description. (Show Details)
cscott renamed this task from Add Assert::unreachable with appropriate phan annotations to Create a phan plugin to handle the Assert library.Mar 6 2020, 3:26 PM
cscott updated the task description. (Show Details)

(Sorry, edited the bug instead of cloning it, as I meant to.)

Anyway, I created T247093: Add Assert::UnreachableException as a follow up to my comment above: T226266#5948571

Another issue found in Parsoid: or-conditions in assertions seem to confuse phan.

In DOMDataUtils we have:

Assert::invariant( empty( $options['discardDataParsoid'] ) || empty( $options['keepTmp'] ), ...);

This causes phan errors on the later lines:

$discardDataParsoid = !empty( $options['discardDataParsoid'] );

and

if ( !empty( $options['keepTmp'] ) ) {

apparently because phan thinks those conditions are always true based on the assertion passing. The actual phan errors are:

src/Utils/DOMDataUtils.php:610 PhanRedundantCondition Redundant attempt to cast $options['discardDataParsoid'] of type ?''|?'0'|?0|?0.0|?array{}|?false to empty
src/Utils/DOMDataUtils.php:628 PhanRedundantCondition Redundant attempt to cast $options['keepTmp'] of type ?''|?'0'|?0|?0.0|?array{}|?false to empty

Change 572331 had a related patch set uploaded (by C. Scott Ananian; owner: Jforrester):
[mediawiki/services/parsoid@master] Increase wikimedia/assert dependency to 0.5.0

https://gerrit.wikimedia.org/r/572331

Change 572331 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Increase wikimedia/assert dependency to 0.5.0

https://gerrit.wikimedia.org/r/572331

The wiki page says

This can be used in combination with Phan's template support.

Hah, I've realized this just now. And there's more: it works! I'm going to send a patch.

Change 589293 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/libs/Assert@master] Add phan assertions for instanceof checks

https://gerrit.wikimedia.org/r/589293

The wiki page says

This can be used in combination with Phan's template support.

Hah, I've realized this just now. And there's more: it works! I'm going to send a patch.

Meh, never mind. It does work for simple cases, if you use e.g.

/**
 * @template T
 *
 * @param string|array $type
 * @phan-param class-string<T> $type
 * @param mixed $var
 * @phan-assert T $var
 */

but all methods in the Assert library take multiple types. Phan can work with multiple types, and it also seems to use OR, not AND, but I'm not sure.
Moreover, Assert:: methods take pipe-imploded types (as opposed to arrays), so we can't use something like @phan-param class-string<T>|class-string<T>[] $type, as phan would complain about Foo|Bar not being a valid FQSEN.

If we get rid of the pipe-imploded syntax (which was deprecated in 0.4.0 anyway), we should be able to get this done.

...But then it wouldn't work with scalar types. Alright, I think this is not doable, there are too many things to watch out for.

In addition to the above, we should at least have different methods for scalars and classes, which wouldn't probably look pretty.

Change 589293 merged by jenkins-bot:
[mediawiki/libs/Assert@master] Add phan assertion for Assert::nonEmptyString

https://gerrit.wikimedia.org/r/589293