Page MenuHomePhabricator

redundant_condition_detection is still suppressed
Closed, ResolvedPublic

Description

In mediawiki's phan config, there is the following

// This helps a lot in discovering bad code, but unfortunately it will always fail for
// hooks + pass by reference, see phan issue #2943.
// @todo Enable when the issue above is resolved and we update our config!
$cfg['redundant_condition_detection'] = false;

https://github.com/phan/phan/issues/2943 appears to have been resolved with https://github.com/phan/phan/pull/3176 in September 2019; has the config been updated to allow enabling this?

Regardless, removing the disabling just to see the result shows that there are some issues that would have been caught that are not related to hooks and should be fixed

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 renamed this task from PhanRedundantCondition is still suppressed to redundant_condition_detection is still suppressed.Mar 25 2020, 1:13 AM
DannyS712 updated the task description. (Show Details)

https://github.com/phan/phan/issues/2943 appears to have been resolved with https://github.com/phan/phan/pull/3176 in September 2019;

My fix was incomplete, I should try it again for the third time...

<file name="includes/Autopromote.php">
    <error line="213" severity="info" message="Suspicious attempt to compare $result of type null to null of type null with operator '==='" source="PhanSuspiciousValueComparison"/>
  </file>
  <file name="includes/BadFileLookup.php">
    <error line="74" severity="info" message="Redundant attempt to cast $bad of type false to bool" source="PhanRedundantCondition"/>
  </file>
  <file name="includes/CategoryViewer.php">
    <error line="198" severity="info" message="Suspicious attempt to compare $link of type null to null of type null with operator '==='" source="PhanSuspiciousValueComparison"/>
  </file>

These are all false positives, and that's just for the first three issues reported... Still too buggy to enable.

The most issues are coming from hooks where argument are passed by ref and after the hook checked if non-empty/changed. Phan seems to assume that the var is always empty and does not take the pass-by-ref into account. That may happen due to deep call to call_user_func or such. Maybe the new hook system with explicit functions to call at the first level makes that better (but call_user_func still there, just another place).

The most issues are coming from hooks where argument are passed by ref and after the hook checked if non-empty/changed. Phan seems to assume that the var is always empty and does not take the pass-by-ref into account.

Correct.

That may happen due to deep call to call_user_func or such.

Also correct; this is what I tried to mitigate with my previous fix(es), but didn't succeed.

Maybe the new hook system with explicit functions to call at the first level makes that better (but call_user_func still there, just another place).

IIRC, it will. That's because the reference is in now in the callee signature, rather than inside the array of arguments.

Change 605559 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] [WIP][DNM] phan: Enable redundant_condition_detection

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

Change 605559 merged by jenkins-bot:
[mediawiki/core@master] phan: Enable redundant_condition_detection

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /605559

Daimona assigned this task to Umherirrender.