Page MenuHomePhabricator

SecurePoll's phan CI has been configured such that it did not catch a missing comma
Closed, ResolvedPublic

Description

Few moments ago, I backported rESPO2d6c378fe509: Add missing comma as an UBN fix for T316150. This patch adds a comma that was missing in rESPO6bb33491cec6: Use real transactions when managing the voter list for a poll.

This kind of error should be catched by CI. Copying a relevant discussion from #mediawiki-core at IRC:

18:25 <zabe> someone willing to quickly +2 https://gerrit.wikimedia.org/r/c/mediawiki/extensions/SecurePoll/+/827529/ ?
18:26 <zabe> Thanks Lucas_WMDE :)
18:27 <Reedy> I was going to ask how CI didn't complain...
18:27 <Lucas_WMDE> shouldn’t CI have caught that? even if we don’t have any tests actually running the code, at least Phan or something like that?
18:27 <Reedy> but that's valid syntax xD
18:27 <Lucas_WMDE> oh damn
18:27 <Lucas_WMDE> grr, right
18:27 <Reedy> And as most of the parameters are optional...
18:27 <Reedy> it won't complain about lack of
18:27 <legoktm> how is [] on a constant valid syntax? like, if it were an array sure
18:28 <Lucas_WMDE> it’s “valid” enough that the result is a fatal error at runtime, but not a parse error, I guess?
18:28 <Reedy> and no test coverage?
18:28 <taavi> isn't [] the array append operator? how is that valid as a parameter value?
18:28 <legoktm> I would've expected phan to have caught it
18:28 <Reedy> hmm
18:28 <legoktm> s/expected/hoped/
18:28 <Reedy> php -l doesn't like it
18:28 <Reedy> does parallel lint for some reason?
18:29 <Lucas_WMDE> hm, in php -a it seems to be a parse error after all

Production repos with scalar_implicit_cast ignored (search):

Production repos with null_casts_as_any_type ignored (search):

Event Timeline

Urbanecm renamed this task from CI did not catch a missing dot to CI did not catch a missing comma.Aug 29 2022, 6:01 PM

From my local testing (on PHP 8.1.9 CLI):

php > define( 'FOO', 'bar' );

php > var_dump( FOO [] );
PHP Fatal error:  Cannot use [] for reading in php shell code on line 1

php > var_dump( strval( FOO
php ( ) );
string(3) "bar"

Clearly a \n is a fundamentally different parse context than a different whitespace character for PHP. 🙄

I checked out the broken version locally and ran parallel-lint; no error, as expected. Then I ran php -l on that file with PHP 7.2 and then 8.1, and neither of them reported anything. I then opened the file in PHPStorm and it was also not reporting a syntax error. I get the same result if I remove all the whitespace between the constant and the array access. Then I tried a simplified snippet on 3v4l and it seems to be a syntax error, not a parse error. This is consistent with the CLI:

$ php -r "class Foo { public const FOO = 42;} var_dump( Foo::FOO[] );"
PHP Fatal error:  Cannot use [] for reading in Command line code on line 1

In fact, it seems definitely parseable according to this util found in phan:

$ php internal/dump_fallback_ast.php --php-ast 'class Foo { public const FOO = 42;} var_dump( Foo::FOO[] );'
AST_STMT_LIST [] #1
	0 => AST_CLASS [] #1:1
		name => Foo
		docComment => null
		extends => null
		implements => null
		stmts => AST_STMT_LIST [] #1
			0 => AST_CLASS_CONST_GROUP [MODIFIER_PUBLIC] #1
				const => AST_CLASS_CONST_DECL [] #1
					0 => AST_CONST_ELEM [] #1
						name => FOO
						value => 42
						docComment => null
				attributes => null
		attributes => null
		type => null
		__declId => 0
	1 => AST_CALL [] #1
		expr => AST_NAME [NAME_NOT_FQ] #1
			name => var_dump
		args => AST_ARG_LIST [] #1
			0 => AST_DIM [] #1
				expr => AST_CLASS_CONST [] #1
					class => AST_NAME [NAME_NOT_FQ] #1
						name => Foo
					const => FOO
				dim => null

Since the code is syntactically correct, parallel-lint is right. The only way of catching this would be with static analysis, so phan.

Now, I can confirm locally that running phan on that code yields no warning. Same if I update to phan master and with several combinations:

$x = ILoadBalancer::DB_PRIMARY;
'@phan-debug-var $x';
var_dump( $x[] );
$dbw = $lb->getConnectionRef( ILoadBalancer::DB_PRIMARY[], false, ILoadBalancer::CONN_TRX_AUTOCOMMIT );
$dbw2 = $lb->getConnectionRef( $x[], false, ILoadBalancer::CONN_TRX_AUTOCOMMIT );
$dbw3 = $lb->getConnectionRef( ILoadBalancer::DB_PRIMARY[] );
$y = -2;
$dbw3 = $lb->getConnectionRef( $y[] );

Yet the trivial snippet fails immediately:

input:12: PhanTypeArraySuspicious Suspicious array access to $y of type -2

I was about to wonder if it might have been due to the intrinsic complexity of a MediaWiki core + extension system, but it turns out the answer is much simpler:

SecurePoll/.phan/config.php
// These are too spammy for now. TODO enable
$cfg['scalar_implicit_cast'] = true;

While that option (and also null_casts_as_any_type that was previously disabled, hence the "these") is sometimes spammy, it's also necessary for catching these issues. If I enable it, phan reports all my tests as having a suspicious array access -- together with 28 unrelated errors.

Long story short: we should disable scalar_implicit_cast in the phan config for SecurePoll. And possibly for everything WMF-deployed (codesearch). Same for null_casts_as_any_type, just in case. Maybe even making it a requirement for deployed code, just like running phan is.

Jdforrester-WMF renamed this task from CI did not catch a missing comma to SecurePoll's phan CI has been configured such that it did not catch a missing comma.Aug 29 2022, 9:19 PM

Thanks for the quick investigation @Daimona!

Change 828046 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/OAuth@master] [WIP] build: Don't allow risky scalar_implicit_cast in phan

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

Change 828074 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/SecurePoll@master] phan: Disable phan option scalar_implicit_cast and make pass

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

Change 723721 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/Quiz@master] build: Disable phan option scalar_implicit_cast and make pass

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

Change 724446 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/OAuth@master] build: Disable phan option scalar_implicit_cast and make pass

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

Change 828082 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/CentralNotice@master] build: Disable phan option scalar_implicit_cast and make pass

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

As Translate, Wikibase and parsoid are bigger repositiory and under active development the teams should handle that, created/linked separate tasks for that.
In that situation it could be easier to create smaller patches solving only one or some more issues in a small code area to make reviewing the small thing easier, even the big change is not visible right now.
But that was very helpful to get the issues fixed on core, where one big patch set was run in merge conflicts very soon.

Change 828046 abandoned by Jforrester:

[mediawiki/extensions/OAuth@master] [WIP] build: Don't allow risky scalar_implicit_cast in phan

Reason:

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

Change 724446 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] build: Disable phan option scalar_implicit_cast and make pass

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

Change 828074 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] phan: Disable phan option scalar_implicit_cast and make pass

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

Jdforrester-WMF claimed this task.

Thanks all! The remaining ones have their own tasks.

Change 723721 merged by jenkins-bot:

[mediawiki/extensions/Quiz@master] build: Disable phan option scalar_implicit_cast and make pass

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

Change 828082 merged by jenkins-bot:

[mediawiki/extensions/CentralNotice@master] build: Disable phan option scalar_implicit_cast and make pass

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