Page MenuHomePhabricator

Butcher the list of phan suppressions in MW core
Closed, ResolvedPublic

Description

MW core has a huge list of globally-suppressed phan issues. This weakens phan's analysis a lot, and can (and will!) cause actual errors to pass CI unnoticed. For a real, recent example, see T231488.
Running phan with all those issues suppressed exposes us to an increased risk of regressions; and on untested code, this could mean new production errors and/or train blockers.

As such, the proposal is to kill the list of suppressions (or at least, limit it to max 5 issue types), so that we can fully benefit from phan.

False positives will happen, of course. But in such a case, it's highly preferred to just suppress the issue inline (with @phan-suppress-next-line and similar), at block-level (via @suppress), or file-level (with @phan-file-suppress), so that the issue won't be suppressed in the remaining 99.9% of the code. Keeping mediawiki-phan-config up-to-date with phan should also be done in tandem: it'll be able to detect new issue types and reduce the amount of false positives.

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+4 -9
mediawiki/coremaster+17 -16
mediawiki/coremaster+40 -13
mediawiki/coremaster+21 -5
mediawiki/coremaster+43 -25
mediawiki/coremaster+23 -6
mediawiki/coremaster+44 -19
mediawiki/coremaster+33 -25
mediawiki/coremaster+24 -8
mediawiki/coremaster+20 -16
mediawiki/coremaster+32 -38
mediawiki/coremaster+11 -15
mediawiki/coremaster+26 -31
mediawiki/coremaster+20 -11
mediawiki/coremaster+27 -8
mediawiki/coremaster+21 -3
mediawiki/coremaster+32 -2
mediawiki/coremaster+657 -49
mediawiki/coremaster+61 -50
mediawiki/coremaster+109 -16
mediawiki/coremaster+237 -13
mediawiki/coremaster+108 -76
mediawiki/coremaster+75 -56
mediawiki/coremaster+95 -42
mediawiki/coremaster+77 -54
mediawiki/coremaster+1 K -29
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 533177 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Globally unsuppress phan issues with low count

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

Change 533206 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress other phan issues with low count

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

Daimona triaged this task as High priority.Aug 30 2019, 9:42 AM

Boldly triaging as high: I've already explained the risks of weakening phan's analysis.

Change 533516 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress more phan issues

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

Change 533177 merged by jenkins-bot:
[mediawiki/core@master] Globally unsuppress phan issues with low count

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

Change 533206 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress other phan issues with low count

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

Change 533547 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress other phan issues

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

Change 533566 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress more phan issues

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

Change 533567 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [WIP] Unsuppress phan issues part 6

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

Daimona added a subscriber: Umherirrender.

As of the last patch, there are 7 issues remaining. PhanParamReqAfterOpt needs a new version of phan to avoid false positives, and for PhanParamTooMany we have to drop HHVM support first. One was handled by @Umherirrender in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/517740/. I hope to work on the remaining 4 in the next days.

Change 533703 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress another phan issue

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

Change 533516 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress more phan issues (part 3)

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

Change 533547 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress other phan issues (part 4)

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

Change 533739 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress phan issues, part 8

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

Change 533746 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Upgrade phan config to 0.7.1

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

Change 533566 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress more phan issues (part 5)

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

Change 533567 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress phan issues part 6

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

Change 517740 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] phan: Enable PhanTypeMismatchReturn and PhanTypeMismatchArgument issue

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

Change 533895 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress another phan issue, part 9

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

Currently blocked on HHVM to remove the last suppressions: PhanTooManyParams has tons of false positives due to variargs, and the one for required parameters following optional ones has false positives due to nullables (PHP71 will allow us to use ?MyClass $x instead of MyClass $x = null).

Change 533703 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress another phan issue (part 7)

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

Change 533739 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress phan issues, part 8

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

Change 533746 merged by jenkins-bot:
[mediawiki/core@master] Upgrade phan config to 0.7.1

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

I am fine with work on issues found by phan and I support such work, but on an old codebase like mediawiki/core you cannot assume that 7 days and 10+ patch sets with 2000+ changed line will bring you to a better world without global suppressions. Phan is running since some months (or years) with all the suppressions for a reason (no one work on it).

I have understand phan as a tool to point you to places in your code which are not clean of types.
In my opinion each issue found by phan needs investigation and a deeper look how to fix the code at that location. A fix could be a refactor to avoid function calls or changing types, adding documentation or adding type checks to make the code safer, another way is to add suppressions or teaching phan with @phan-var hints, but both should be used not so often. Such a deep is not possible in such big patch sets as done here. Some places could be fixed easier and now have to changed again where some lines goes back to the previous state.
We have phan now and it is good to get it voting on new issues, but it could be big work to get rid of all the places now suppressed, because after this big bang nobody works on that later on.

Big patch sets also makes it impossible to add reviewer for a smaller code area where there have deeper knownledge and my say why the location is unclean.
Big patch sets with very different kinds of "fixes" are possible reviewed to fast to get the feedback of all reviewer - (but these patch sets gets reviewed, instead of smaller one with one line fixes and a big comment why things should be changed).
Having comments why changing code is a benefit of smaller patch set, it is also possible to run phan locally with a blank blacklist and see if the fix maybe result in another issue in that function/class or subclass and needs other fix.
Another way to uploading patch sets grouped by issue types (like missing use statements, class -> interface changes) or by code area (like api, media, file, etc.), that also avoids that one merge conflict blocks the whole process, because other areas can be still be merged.
Having patch sets only with suppressions and @phan-var and config changes are okay for me

I am fine with work on issues found by phan and I support such work, but on an old codebase like mediawiki/core you cannot assume that 7 days and 10+ patch sets with 2000+ changed line will bring you to a better world without global suppressions.

Probably not better, but neither worse, IMHO. I think it's necessary to remove global suppressions before going further: some issues may depend on suppressed stuff, and with global suppression you cannot tell which parts of the code are hiding bugs.

I have understand phan as a tool to point you to places in your code which are not clean of types.

Not only for types, IMHO. Phan has a huge potential in discovering lots of different types of bugs.

In my opinion each issue found by phan needs investigation and a deeper look how to fix the code at that location.

That's what I mostly tried to do, but I prefer to leave the "hard" fixes for later.

A fix could be a refactor to avoid function calls or changing types,

These are "hard" fixes IMHO.

adding documentation or adding type checks to make the code safer,

That's what I tried to do.

@phan-var [...] but both should be used not so often.

I don't agree with that. If you have a method taking a config array with clearly defined options (e.g. objectcache or HTMLForm), there's no alternative to @phan-var. And actually, I believe it also helps in having a more readable documentation. I obviously agree about suppressions.

We have phan now and it is good to get it voting on new issues, but it could be big work to get rid of all the places now suppressed, because after this big bang nobody works on that later on.

That's not true, I'm actually planning to re-check all suppressions. It'll take some time, of course, but at least now we know where to look, and not having global suppressions could make fixing stuff easier.

Big patch sets also makes it impossible to add reviewer for a smaller code area where there have deeper knownledge and my say why the location is unclean.
Big patch sets with very different kinds of "fixes" are possible reviewed to fast to get the feedback of all reviewer - (but these patch sets gets reviewed, instead of smaller one with one line fixes and a big comment why things should be changed).
Having comments why changing code is a benefit of smaller patch set, it is also possible to run phan locally with a blank blacklist and see if the fix maybe result in another issue in that function/class or subclass and needs other fix.
Another way to uploading patch sets grouped by issue types (like missing use statements, class -> interface changes) or by code area (like api, media, file, etc.), that also avoids that one merge conflict blocks the whole process, because other areas can be still be merged.
Having patch sets only with suppressions and @phan-var and config changes are okay for me

That's true, especially for what concerns review. OTOH, sometime you have to make more changes to unhid actual issues, like I said above.

@phan-var [...] but both should be used not so often.

I don't agree with that. If you have a method taking a config array with clearly defined options (e.g. objectcache or HTMLForm), there's no alternative to @phan-var. And actually, I believe it also helps in having a more readable documentation.

Uh-oh, I thought we were talking about @phan-param and other annotations used to specify the shape of arrays. As for '@phan-var' inline, yes, I agree - although sometime a proper fix would require a lot of time.

Change 534907 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Suppress PhanUndeclaredProperty for custom properties and phan bugs

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

Change 533895 abandoned by Daimona Eaytoy:
Unsuppress another phan issue, part 9

Reason:
Done in Iee73ddb554e354abe52d13dcfc453f9a15bb8877 and the patches it depends-on.

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

Change 534907 merged by jenkins-bot:
[mediawiki/core@master] Suppress PhanUndeclaredProperty for custom properties and phan bugs

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

Daimona changed the task status from Open to Stalled.Sep 20 2019, 10:20 AM

The remaining issues are blocked on dropping HHVM support, so that we can use real variargs and nullable types to avoid false positives.

Change 517740 merged by jenkins-bot:
[mediawiki/core@master] phan: Enable PhanTypeMismatchArgument issue

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

Change 539339 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress PhanTooManyParams

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

Daimona changed the task status from Stalled to Open.Oct 5 2019, 1:12 PM

Change 541094 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Unsuppress PhanParamReqAfterOpt, use PHP71 nullable types

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

Change 539335 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/core@master] Fixup phan warning toomanyparams (part 2)

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

Change 539333 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix some phan warnings for too many params (part 1)

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

Change 539333 merged by jenkins-bot:
[mediawiki/core@master] Fix some phan warnings for too many params (part 1)

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

Change 541094 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress PhanParamReqAfterOpt, use PHP71 nullable types

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

Change 539335 merged by jenkins-bot:
[mediawiki/core@master] Fixup phan warning toomanyparams (part 2)

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

Change 539339 merged by jenkins-bot:
[mediawiki/core@master] Unsuppress PhanParamsTooMany

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

Resolved?

Before seeing https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/541774/, I would've said yes. But now, I'd say no... Let's fix the new errors, too.

Change 542427 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 1

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

Change 542436 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 2

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

Change 542444 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 3

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

Change 542447 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 4

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

Change 541774 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] build: Upgrade mediawiki-phan-config to 0.8.0

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

Change 542427 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 1

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

Change 542436 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 2

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

Change 542444 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 3

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

Change 542447 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 4

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

Change 542672 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 5

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

Change 542673 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 6

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

Change 542677 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 7

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

Change 542678 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 8

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

Change 542693 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Fix new phan errors, part 9

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

Change 542672 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 5

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

Change 542673 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 6

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

Change 542677 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 7

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

Change 542678 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 8

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

Change 542693 merged by jenkins-bot:
[mediawiki/core@master] Fix new phan errors, part 9

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

Change 541774 merged by jenkins-bot:
[mediawiki/core@master] build: Upgrade mediawiki-phan-config to 0.8.0

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