Page MenuHomePhabricator

Phan should handle always-throw-function like ApiBase::dieWithError better
Open, Needs TriagePublic

Description

It seems that phan does not handle function with always throws and does not infer the type correctly after the if

Example:

		$title = CategoryTree::makeTitle( $params['category'] );
		if ( !$title || $title->isExternal() ) {
			$this->dieWithError( [ 'apierror-invalidtitle', wfEscapeWikiText( $params['category'] ) ] );
		}
[...]
		$html = $this->getHTML( $ct, $title, $depth, $ctConfig );

getHTML has a Type hint on $title but phan assumes that $title can be null here, which is not true, because dieWithError would throw and than the function is not reached.

It is possible to write a plugin for this case to avoid adding @phan-var?

Phan works correct when there is a return after dieWithError - so it should be possible to handle this case in a plugin

Such a plugin also can help to detect unreachable code with phan after this function

Current suppressed cases (incomplete list): https://codesearch.wmflabs.org/search/?q=T240141&i=nope&files=&repos=

Functions:

  • ApiBase::dieWithError
  • ApiBase::dieWithException
  • ApiBase::dieStatus
  • Maintenance::fatalError
  • \CirrusSearch\Maintenance\ConfigUtils::fatalError in CirrusSearch extension

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 8 2019, 8:55 PM
Daimona added a subscriber: Daimona.

I remember some related issue upstream, but cannot find it right now. @phan-assert annotations can help when the method can only throw conditionally, but unfortunately there doesn't seem to be a way to mark a method as "always throwing".

Anyway, instead of writing a plugin, this should be reported upstream.

I have found https://github.com/phan/phan/pull/1020/files where a BlockExitStatusChecker was written. But that is an old commit.

But I have no idea if that would look into functions or only see the throw in the current function.

Memo: after this is fixed, we should merge https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/560343/ as well.

...which was closed with a pointer to https://github.com/phan/phan/issues/2118

Do we want to just wait for phan upstream to make progress or should we be looking into a plugin or something?

Legoktm updated the task description. (Show Details)Sat, Dec 28, 7:34 AM
Daimona updated the task description. (Show Details)Sat, Dec 28, 7:03 PM

Do we want to just wait for phan upstream to make progress or should we be looking into a plugin or something?

Well, I guess that we could write a stopgap for the time being. Like some very simple plugin, which would only handle dieWithError. As long as it doesn't take too much time to write such a plugin, and it's easy to maintain, we can use it until the issue is fixed upstream.

I am not a fan of hardcoded list, but adding support for an annotation maybe more complex and time consuming, so that would be okay.

Another function for the list Maintenance::fatalError

I am not a fan of hardcoded list, but adding support for an annotation maybe more complex and time consuming, so that would be okay.

Yes, what I meant is, it must not be time consuming or complex, otherwise we can just wait for a solution upstream.

Another function for the list Maintenance::fatalError

Yeah, I guess we can handle them both.