Page MenuHomePhabricator

Strengthen phan's default config
Closed, ResolvedPublic

Description

There are two options currently disabled which make the analysis weaker:

  • scalar_implicit_cast: as the name would suggest, it considers scalar types to be equivalent. While MW is pretty lax, this could hide bugs, and there are cases where scalars are not equivalent (e.g. T232651)
  • null_casts_as_any_type: does what it says on the tin. Setting this to true will produce some false positives, so it should probably be done as the last step.

The idea is to strengthen the default config, and re-relax it on a per-repo basis if needed. For instance, enabling scalar_implicit_cast would create roughly 1200 new issues for core, which is not something that can be fixed in one go. At the same time, having it enabled as default will allow us to fix repos with fewer errors, and start enforcing higher standards there.

Event Timeline

Change 536651 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan@master] Upgrade phan, disable implicit scalar casts

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

Change 536651 merged by jenkins-bot:
[mediawiki/tools/phan@master] Disable implicit scalar and null casts

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

While I don't want to turn this task into a never-ending tracking task, I must also note that there's another option that we could use: dead code detection. It will have a lot of false positives for (e.g.) unused public/protected methods/properties, but maybe we could start by enabling dead code detection and selectively disabling those issues.

If needed that should be another bug, maybe stalled for the begin as dead code detection takes more memory and more cpu.
It can bring in more suppressions as we have at the moment with the redundant detection plugin.

There are possible more plugins which can help us on code quality, but it is hard work to get them passed on the old code base.

If needed that should be another bug

Yes, I'm going to open it.

It can bring in more suppressions as we have at the moment with the redundant detection plugin.

That's why I wanted to start very mildly.

I'm experimenting this locally.