Page MenuHomePhabricator

Have some fixers for Phan issues so that LibUp patches aren't so much work for humans
Open, Needs TriagePublic

Event Timeline

Daimona subscribed.

This is probably quite complex in most cases. There are just a handful of issues that can be safely fixed automatically. A category of easy-to-autofix issues is UnusedSuppression*, which are also amongst the issues that are seen most commonly. See https://github.com/phan/phan/issues/4304 upstream.

Would having libup automatically insert // @phan-suppress-next-line {whatever} comments be helpful?

This is probably quite complex in most cases. There are just a handful of issues that can be safely fixed automatically. A category of easy-to-autofix issues is UnusedSuppression*, which are also amongst the issues that are seen most commonly. See https://github.com/phan/phan/issues/4304 upstream.

I implemented this for eslint a while back, see https://gerrit.wikimedia.org/r/plugins/gitiles/labs/libraryupgrader/+/refs/heads/master/libup/ng.py#945 - probably the same could be done for phan as well.

Would having libup automatically insert // @phan-suppress-next-line {whatever} comments be helpful?

I don't think so. I mean, yes, it would allow merging the commit immediately. But it would create technical debt that someone will have to examine and fix sooner or later.

This is probably quite complex in most cases. There are just a handful of issues that can be safely fixed automatically. A category of easy-to-autofix issues is UnusedSuppression*, which are also amongst the issues that are seen most commonly. See https://github.com/phan/phan/issues/4304 upstream.

I implemented this for eslint a while back, see https://gerrit.wikimedia.org/r/plugins/gitiles/labs/libraryupgrader/+/refs/heads/master/libup/ng.py#945 - probably the same could be done for phan as well.

That might be an idea, although I guess having it implemented upstream would be great. I haven't really looked into how difficult it would be to implement, after creating that issue. Phan does have an autofixer, it's just not used by many plugins because usually, there isn't a clear/unique way to fix an issue. I guess LibUp might be instructed to run it; it should be as easy as adding --automatic-fix to the command.

Would having libup automatically insert // @phan-suppress-next-line {whatever} comments be helpful?

I don't think so. I mean, yes, it would allow merging the commit immediately. But it would create technical debt that someone will have to examine and fix sooner or later.

It's what we currently do for PHPCS and eslint (though we globally disable rules rather than per-line disabling). And then dashboards like https://phpcs.toolforge.org/ can help point people in the right direction of what needs fixing. I think we're starting to get to the point where phan is deployed so widely it's also on repos that aren't actively maintained, so patches are languishing, requiring people to take care of fixing them when it's probably not worth it.

This is probably quite complex in most cases. There are just a handful of issues that can be safely fixed automatically. A category of easy-to-autofix issues is UnusedSuppression*, which are also amongst the issues that are seen most commonly. See https://github.com/phan/phan/issues/4304 upstream.

I implemented this for eslint a while back, see https://gerrit.wikimedia.org/r/plugins/gitiles/labs/libraryupgrader/+/refs/heads/master/libup/ng.py#945 - probably the same could be done for phan as well.

That might be an idea, although I guess having it implemented upstream would be great. I haven't really looked into how difficult it would be to implement, after creating that issue. Phan does have an autofixer, it's just not used by many plugins because usually, there isn't a clear/unique way to fix an issue. I guess LibUp might be instructed to run it; it should be as easy as adding --automatic-fix to the command.

Oh, I'd much prefer having this implemented upstream :)) Should be straightforward to run an extra phan invocation with that added.

It's what we currently do for PHPCS and eslint (though we globally disable rules rather than per-line disabling). And then dashboards like https://phpcs.toolforge.org/ can help point people in the right direction of what needs fixing. I think we're starting to get to the point where phan is deployed so widely it's also on repos that aren't actively maintained, so patches are languishing, requiring people to take care of fixing them when it's probably not worth it.

It should also be noted that PHPCS is enabled on way more repos (~675 vs ~300). I think an interesting idea might be to have a list of actively-maintainted repos (or at least just maintained), and put automatic suppressions iff the current repo is not on that list. Such a list might be generated automatically by looking at git-log. But this is probably just a dream, I guess.

This is probably quite complex in most cases. There are just a handful of issues that can be safely fixed automatically. A category of easy-to-autofix issues is UnusedSuppression*, which are also amongst the issues that are seen most commonly. See https://github.com/phan/phan/issues/4304 upstream.

I implemented this for eslint a while back, see https://gerrit.wikimedia.org/r/plugins/gitiles/labs/libraryupgrader/+/refs/heads/master/libup/ng.py#945 - probably the same could be done for phan as well.

That might be an idea, although I guess having it implemented upstream would be great. I haven't really looked into how difficult it would be to implement, after creating that issue. Phan does have an autofixer, it's just not used by many plugins because usually, there isn't a clear/unique way to fix an issue. I guess LibUp might be instructed to run it; it should be as easy as adding --automatic-fix to the command.

Oh, I'd much prefer having this implemented upstream :)) Should be straightforward to run an extra phan invocation with that added.

What about patching upstream to autofix the unused suppressions and see how it goes? We can then enable auto-suppression if necessary.

POC: https://github.com/Daimona/phan/commit/0a15585d71633aa6452dcb14dbbc2d40816e0e88 As noted in the github issue, it's very much incomplete and not really functional, but I'm unsure what to do. If anyone wants to give it a try... :)