Page MenuHomePhabricator

False positive phan issue MediaWikiNoIssetIfDefined for variable set inside try/catch
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
Phan reports issue in file src\FrontendModules\EventDetailsParticipantsModule.php

Found usage of isset() on expression $centralUser that appears to be always set. isset() should only be used to suppress errors. Check whether the expression is null instead. See https://w.wiki/98zs

		try {
			$centralUser = $this->centralUserLookup->newFromAuthority( $authority );
			$curUserParticipant = $this->participantsStore->getEventParticipant( $eventID, $centralUser, true );
		} catch ( UserNotGlobalException $_ ) {
			$curUserParticipant = null;
		}
[...]
		$otherParticipants = $this->participantsStore->getEventParticipants(
			$eventID,
			$otherParticipantsNum,
			null,
			null,
			null,
			$showPrivateParticipants,
			isset( $centralUser ) ? [ $centralUser->getCentralID() ] : null
		);

The variable maybe set from the first line of a try/catch block, but when an exception is thrown from the first line, the variable is unset.
Even without isset() phan does not report issue about "possibly undefined", so this could be an upstream issue as well.

What should have happened instead?:
No issue should be found.

It would be correct to report an issue about isset() when $centralUser is set before the try block.

Event Timeline

Daimona subscribed.

Yep, this seems to be an upstream bug. I could reproduce with just:

<?php

function mayThrow() {
    if ( rand() ) {
        throw new Exception;
    }
    return 42;
}

function doTest() {
    try {
	$val = mayThrow();
    } catch ( Exception $_ ) {
    }
    '@phan-debug-var $val';
    echo $val;
}

IIRC, phan has a few limitations when it comes to analyzing try...catch statements. Nonetheless, I've reported this upstream: https://github.com/phan/phan/issues/4884.

TheDJ subscribed.

This is fixed in phan 6, released last week.