Page MenuHomePhabricator

Phan failed to report seemingly trivial mistyped parameter
Closed, ResolvedPublic

Description

From https://gerrit.wikimedia.org/r/c/mediawiki/core/+/763347

- 			$ok = $this->replaceLostConnection( __METHOD__, null );
+ 			$ok = $this->replaceLostConnection( null, __METHOD__ );

Which fixed the following from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761514:

class Database {
	public function ping( &$rtt = null ) {
			$ok = $this->replaceLostConnection( __METHOD__, null );
	}
	/**
	 * @param int|null $lastErrno
	 * @param string $fname
	 * @return bool
	 */
	protected function replaceLostConnection( $lastErrno, $fname ) {
		$this->connLogger->warning( $fname . ': lost connection' );
	}
}

Event Timeline

I suspect this might be because for MW core, the strict type checking rules are turned off:

.phan/config.php
// These are too spammy for now. TODO enable
$cfg['null_casts_as_any_type'] = true;
$cfg['scalar_implicit_cast'] = true;

It'd be great to enable them some day, but as you may imagine, they would currently result in lots of new issues (mostly related to documentation or implicit assertions).

See also T242536

There are about 1000 issues reported by phan in about 375 files (without some merges from the last days).

  • A bigger area which needs suppression is Title::castFromPageReference, the factory function and some other casts function. All functions are documented to return null, but that only happen when the input is null, but phan cannot see the difference and reports possible nullable issues.
  • Another issue is with Block::getId or RevisionRecord::getId, both are documented to return null, because one of the subclass implementation may does it (if the block or revision is not persist in the database), but in the most cases it safe to use the value. This may correct in terms of LSP.
  • pass-by-reference arguments are sometimes initalized on the caller side with null or false, while the documented type is not null or false, because there would be set by the called function, not the caller. Not sure if ouer code should just omit the set of the variable or phan could be relaxed on this part.
  • Some possible fatal errors are also found - for example https://gerrit.wikimedia.org/r/c/mediawiki/core/+/763857
  • Many investigation to find a possible root caught for the phan issues (possible upstream bugs also in there)
  • Some php version differences on scalar types needs suppression

Change 769514 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] phan: Enable strict checks

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

Change 771705 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] phan: Disable scalar_implicit_cast setting

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

Change 771705 merged by jenkins-bot:

[mediawiki/core@master] phan: Disable scalar_implicit_cast setting

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

Change 769514 merged by jenkins-bot:

[mediawiki/core@master] phan: Disable null_casts_as_any_type setting

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

Umherirrender claimed this task.

See also T242536

There are about 1000 issues reported by phan in about 375 files (without some merges from the last days).

  • A bigger area which needs suppression is Title::castFromPageReference, the factory function and some other casts function. All functions are documented to return null, but that only happen when the input is null, but phan cannot see the difference and reports possible nullable issues.
  • Another issue is with Block::getId or RevisionRecord::getId, both are documented to return null, because one of the subclass implementation may does it (if the block or revision is not persist in the database), but in the most cases it safe to use the value. This may correct in terms of LSP.
  • pass-by-reference arguments are sometimes initalized on the caller side with null or false, while the documented type is not null or false, because there would be set by the called function, not the caller. Not sure if ouer code should just omit the set of the variable or phan could be relaxed on this part.
  • Some possible fatal errors are also found - for example https://gerrit.wikimedia.org/r/c/mediawiki/core/+/763857
  • Many investigation to find a possible root caught for the phan issues (possible upstream bugs also in there)
  • Some php version differences on scalar types needs suppression

I have fixed most of the issues with different patch sets, but still some suppression could be fixed with a bigger refactor or by reporting upstream issues for real false positives.
But ourer phan is some version behind upstream, so I hope the next version o fphan can resolve some false positives as well.
Thanks for the reviews.