Page MenuHomePhabricator

conflict between codehealth and use of @phan-var annotation
Open, Needs TriagePublic

Description

There is a conflict between the codehealth job and phan job when used both in an extension.

While trying to give phan more information with an inline expression '@phan-var type $var'; that results in an new finding by codehealth or IDEs, because the string is unused.

The text in codehealth is:

Any statement (other than a null statement, which means a statement containing only a semicolon ;) which has no side effect and does not result in a change of control flow will normally indicate a programming error, and therefore should be refactored.

Just to want write this down for the moment. I have no idea how to resolve this.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 28 2019, 8:13 PM

I don't think there's a way to resolve this. In order to see the @phan-var, phan requires it to be in an actual statement. The best we can do is try to avoid using it as much as possible, but sometimes it's just not doable.

Making phan understand the Assert library (T226266) would certainly help, but it's not enough.

Tgr added a subscriber: Tgr.EditedOct 8 2019, 12:08 PM

Phan understands assert( $foo instanceof Bar ); (or assert( is_int( $foo ) ); etc) as an alternative. Not sure if we want to use asserts in our code though; we created the wikimedia/assert library so we won't have to. That was in the PHP 5 times and asserts are supposedly not so bad in PHP 7, I don't know if that's enough to change our mind though.

@kostajh reported this to the SonarQube upstream.

Phan understands assert( $foo instanceof Bar ); (or assert( is_int( $foo ) ); etc) as an alternative.

Correct.

Not sure if we want to use asserts in our code though; we created the wikimedia/assert library so we won't have to.

In theory it shouldn't be different, if we resolve T226266 (half done with annotations).

That was in the PHP 5 times and asserts are supposedly not so bad in PHP 7, I don't know if that's enough to change our mind though.

Yeah, probably it can be reconsidered. The Assert:: library is also way slower than assert(), so we cannot just spam calls to it.

@kostajh reported this to the SonarQube upstream.

Cool, thanks Kosta! At this point, it'd also be cool if we could report something similar upstream for PHPStorm (which emits warnings for '@phan-var' lines).

Cool, thanks Kosta! At this point, it'd also be cool if we could report something similar upstream for PHPStorm (which emits warnings for '@phan-var' lines).

What warnings do you see? PhpStorm doesn't complain about them for me.

Cool, thanks Kosta! At this point, it'd also be cool if we could report something similar upstream for PHPStorm (which emits warnings for '@phan-var' lines).

What warnings do you see? PhpStorm doesn't complain about them for me.

"Expression result unused", it's under PHP > probable bugs; I don't know whether it's enabled by default.