Page MenuHomePhabricator

Phan should understand IResultWrapper::numRows()
Open, Needs TriagePublic

Description

In the process of re-enabling PhanPossiblyUndeclaredVariable warnings (T283298) we found that in a number of places there was code like

$res = (something returning IResultWrapper);
if ( !$res->numRows() ) {
	break;
}
foreach ( $res as $row ) {
	// Do something
}
// Do something with $row <-- Phan complains it might be undeclared

Phan should be told somehow that since we returned early if $res->numRows() was false, it must have some entries, and so the foreach loop must have run at least once and $row must therefor be declared and represent the last row in the result

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Been thinking about this, I think what makes this non-trivial is that numRows is an instance method. As such, some annotations that could have been useful cannot be used here (e.g. @phan-assert). Also, I'm not sure how we're supposed to document an iterable class for phan. Even if we could use something like MyClass<ElementType>, I don't see how that could be made non-empty.

All in all, we'd need a custom plugin at the very least. Or maybe there's an issue upstream, I haven't checked that. Anyhow, this shouldn't be an issue, we can just leave the suppression in place for now.