Page MenuHomePhabricator

Update phan-taint-check-plugin to a newer phan (1.2.x)
Open, Needs TriagePublic

Description

the plugin is built on top of phan 0.8.0 which is pretty old now. It would be good to update it on top of a modern version of phan, probably 1.2.x.

Event Timeline

Legoktm created this task.Feb 25 2019, 12:24 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 25 2019, 12:24 AM

This would indeed be awesome. However, see my comment in T207344#5033198. Basically, phan has many breaking changes even in patch versions, so upgrading to 0.8.13 would already break a lot of stuff. Today I tried to get there, but eventually gave up due to this.
For whoever will want to proceed with this, here are the main obstacles I found while upgrading to 0.8.13. Note that I don't really know how phan works so it could be fault of mine (but I'm still happy to help).

  • Clazz::getPropertyByNameInContext now has an extra required parameter, $is_static. However, I couldn't find a way to determine if the property is static where we call it.
  • This commit causes 2 troubles:
    • It adds the new class UnaddressableTypedElement, which is extended by Variable. This breaks all typehints for TypedElementInterface in TaintednessBaseVisitor. Just removing them (and adding UnaddressableTypedElement to the docblock) seems to work fine, but it should be checked more deeply.
    • Parameter::getContext() is replaced by Parameter::getFileRef(), which however doesn't have the getScope() method used in setTaintedness.

There are some other changes too, but all of them are easy to do.

After 0.8.13 one may upgrade to 0.9.6. I didn't really check it, but it sounds like there aren't many differences between those, so it's likely that the upgrade will be possible without changing anything.

At this point the plugin should be upgraded to PluginV2, as support for old-style plugins was dropped in 0.9.7 (once again, that's what I meant with "phan doesn't comply with semver"). I think this will require a decent amount of work, but it should make the plugin execute faster thanks to the new design.

After 0.9.7, I don't know, but I guess that the road to 1.2.6 will be pretty long.

Please note that all of the above assumes the goal to just make seccheck work with new phan versions. Using new phan methods to speed up the analysis/make it more precise/avoid hacks/you name it is a whole different story.

At any rate, IMVHO we should stop adding any new feature to seccheck before upgrading to 1.2.x, as it's likely that every addiction will add extra code to be reworked.

Change 497707 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Upgrade phan to 0.8.13

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

Change 497709 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Upgrade phan to 0.9.6

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

So I'm looking at the upgrade to PluginV2. The main change is that *Visitor classes are now instantiated by phan itself, which only wants a "get*ClassName" method instead of a constructor. This mostly means that we cannot pass in an instance of the plugin anymore. Moreover, Visitor and PreVisitor must inherit from different classes, which prevents them from having TaintednessBaseVisitor as common parent (should be fixable by turning TaintednessBaseVisitor into a trait). Plus, the MediaWiki checker runs both the generic visitor and the MW visitor, which again isn't possible in V2 because you can only provide a single class name. I'll probably do some gradual changes before the v2 upgrade.

Change 497797 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Upgrade phan to 0.12.15

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

Change 498121 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Upgrade phan to 1.2.6

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

@Daimona - @Bawolff and I created a 2.0.0 branch from master just now for development of new versions of the SecurityCheckPlugin that leverage PluginV2 and newer versions of PHP and Phan.

Great! Thanks @sbassett and @Bawolff! I guess we're currently blocked on T218719 which makes https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/497583/ fail, plus any further fix which I may have missed and will show up from CI.

@sbassett Why skipping CI? Wouldn't it help to have regression tests in place to ensure that nothing was missed in the upgrade? We can still vote V+2 for spurious failures.

Is it helpful to see a bunch of failed instances of composer-package-php70-docker? I'm not sure how valuable that information would be. And the thought here would be to eventually use the composer-package-php7(2|3)-docker whenever we're ready.

It depends on the reason of the failure :-) Right now it fails because it has ast 0.1.2 installed, which is a very old version anyway, so it'd be better to just upgrade it. But yes, we'll have to use php7.[123] later on due to different phan requirements.

Legoktm added a comment.EditedTue, Apr 9, 9:53 PM

@Daimona - @Bawolff and I created a 2.0.0 branch from master just now for development of new versions of the SecurityCheckPlugin that leverage PluginV2 and newer versions of PHP and Phan.

Yay! but please please don't create branches that have the same exact format as tags - composer is going to get confused and now we can't actually have a 2.0.0 release. Typically we use "2.x" or something.

Change 504395 had a related patch set uploaded (by Brian Wolff; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 0.8.13

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

Change 504395 merged by Brian Wolff:
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 0.8.13

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

Change 504396 had a related patch set uploaded (by Brian Wolff; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 0.9.6

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

Change 504396 merged by Brian Wolff:
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 0.9.6

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

Change 504400 had a related patch set uploaded (by Brian Wolff; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 1.0.0

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

Change 504400 merged by Brian Wolff:
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 1.0.0

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

Change 504402 had a related patch set uploaded (by Brian Wolff; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 1.2.6

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

Change 504402 merged by Brian Wolff:
[mediawiki/tools/phan/SecurityCheckPlugin@2.x] Upgrade phan to 1.2.6

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

Change 497707 abandoned by Brian Wolff:
Upgrade phan to 0.8.13

Reason:
Moved this to 2.x branch

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

Change 497709 abandoned by Brian Wolff:
Upgrade phan to 0.9.6

Reason:
Moved this to 2.x branch

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

Change 497797 abandoned by Brian Wolff:
Upgrade phan to 1.0.0

Reason:
Moved this to 2.x branch

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

Change 498121 abandoned by Brian Wolff:
Upgrade phan to 1.2.6

Reason:
Moved this to 2.x branch

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