Allow dangling commas in variargs

This is because there are many filters using this feature. Moreover, it
could make it a little easier to add new arguments, just like dangling
commas in PHP arrays do.
Also re-align the CachingParser code of doLevelFunctions to the one in
the old Parser.

Bug: T153251
Change-Id: Ie4325159f47310788da57415a5e36e62aa4efad0

includes/parser/AFPTreeParser.php

@@ -608,11 +608,24 @@ class AFPTreeParser {
 			$next = $this->getNextToken();
 			if ( $next->type !== AFPToken::TBRACE || $next->value !== ')' ) {
 				do {
-					$args[] = $this->doLevelSemicolon();
+					$thisArg = $this->doLevelSemicolon();
+					if ( $thisArg === null && !$this->functionIsVariadic( $func ) ) {
+						throw new AFPUserVisibleException(
+							'unexpectedtoken',
+							$this->mPos,
+							[
+								$this->mCur->type,
+								$this->mCur->value
+							]
+						);
+					} elseif ( $thisArg !== null ) {
+						$args[] = $thisArg;
+					}
 				} while ( $this->mCur->type === AFPToken::TCOMMA );
 			} else {
 				$this->move();
 			}
+
 			if ( $this->mCur->type !== AFPToken::TBRACE || $this->mCur->value !== ')' ) {
 				throw new AFPUserVisibleException( 'expectednotfound',
 					$this->mPos,
@@ -741,4 +754,16 @@ class AFPTreeParser {
 			*/
 		}
 	}
+
+	/**
+	 * @param string $fname
+	 * @return bool
+	 * @see AbuseFilterParser::functionIsVariadic
+	 */
+	protected function functionIsVariadic( $fname ) {
+		if ( !array_key_exists( $fname, AbuseFilterParser::$funcArgCount ) ) {
+			throw new InvalidArgumentException( "Function $fname is not valid" );
+		}
+		return AbuseFilterParser::$funcArgCount[$fname][1] === INF;
+	}
 }

includes/parser/AbuseFilterParser.php

@@ -1002,7 +1002,7 @@ class AbuseFilterParser {
 				do {
 					$r = new AFPData( AFPData::DEMPTY );
 					$this->doLevelSemicolon( $r );
-					if ( $r->getType() === AFPData::DEMPTY ) {
+					if ( $r->getType() === AFPData::DEMPTY && !$this->functionIsVariadic( $fname ) ) {
 						$this->logEmptyOperand( 'function argument', __METHOD__ );
 					}
 					$args[] = $r;
@@ -1912,4 +1912,16 @@ class AbuseFilterParser {
 			]
 		);
 	}
+
+	/**
+	 * @param string $fname
+	 * @return bool
+	 * @see AFPTreeParser::functionIsVariadic
+	 */
+	protected function functionIsVariadic( $fname ) {
+		if ( !array_key_exists( $fname, self::$funcArgCount ) ) {
+			throw new InvalidArgumentException( "Function $fname is not valid" );
+		}
+		return self::$funcArgCount[$fname][1] === INF;
+	}
 }

tests/parserTests/contains-all.t

@@ -1 +1,10 @@
-contains_all("the foo is on the bar", "foo", "is on", "bar") & !(contains_all(['foo', 'bar', 'hey'], 'foo', 'bar', 'sup')) & contains_all([1, 2, 3], '1', '2', '3')
+contains_all("the foo is on the bar", "foo", "is on", "bar") &
+!(contains_all(['foo', 'bar', 'hey'], 'foo', 'bar', 'sup')) &
+contains_all([1, 2, 3], '1', '2', '3') &
+contains_all(
+	'base',
+	'b',
+	'a',
+	's',
+	'e',
+)

tests/parserTests/contains-any.t

@@ -1 +1,4 @@
-contains_any("like anyone else", "else", "someone") & contains_any("street fighter", "fight") & !(contains_any('My foo is cute', 'bar', 'wtf')) & contains_any([[1], [2,3]], 1)
+contains_any("like anyone else", "else", "someone") &
+contains_any("street fighter", "fight") &
+!(contains_any('My foo is cute', 'bar', 'wtf')) &
+contains_any([[1], [2,3]], 1)

tests/phpunit/unit/AbuseFilterParserTest.php

@@ -958,8 +958,9 @@ class AbuseFilterParserTest extends AbuseFilterParserTestCase {
 			[ '1**', 'power operand' ],
 			[ '"string" contains', 'keyword operand' ],
 			[ '1 in', 'keyword operand' ],
-			[ "contains_any('a','b','c',)", 'function argument' ],
-			[ "equals_to_any('a','b',)", 'function argument' ],
+			[ "str_replace('a','b','c',)", 'function argument' ],
+			[ "count('a','b',)", 'function argument' ],
+			[ "ccnorm('a',)", 'function argument' ],
 			[ "(!)", 'bool inversion' ],
 			// `(false &!)` and `(true &!)`, originally reported in T156096,
 			// should be used in the future to test that they throw. However,
@@ -981,4 +982,79 @@ class AbuseFilterParserTest extends AbuseFilterParserTestCase {
 			[ 'if (false) then (1) else () end', 'else body' ],
 		];
 	}
+
+	/**
+	 * For the old parser, ensure that dangling commas for variadic functions aren't logged
+	 * @param string $code
+	 * @dataProvider provideDanglingCommasInVariargs
+	 */
+	public function testDanglingCommasInVariargsNotLogged( $code ) {
+		/** @var PHPUnit\Framework\MockObject\MockObject|AbuseFilterParser $mock */
+		$mock = $this->getMockBuilder( AbuseFilterParser::class )
+			->setConstructorArgs(
+				[ $this->getLanguageMock(), new EmptyBagOStuff(), new NullLogger() ]
+			)
+			->setMethods( [ 'logEmptyOperand' ] )
+			->getMock();
+
+		$mock->expects( $this->never() )
+			->method( 'logEmptyOperand' )
+			->with( 'function argument' );
+
+		$mock->toggleConditionLimit( false );
+		$mock->parse( $code );
+	}
+
+	/**
+	 * Ensure that the both parsers won't throw for dangling commas in variadic functions
+	 * @param string $code
+	 * @dataProvider provideDanglingCommasInVariargs
+	 */
+	public function testDanglingCommasInVariargsAreValid( $code ) {
+		foreach ( $this->getParsers() as $parser ) {
+			$pname = get_class( $parser );
+			try {
+				$parser->parse( $code );
+				$this->assertTrue( true );
+			} catch ( AFPException $e ) {
+				$this->fail( "Got exception for dangling commas with parser $pname:\n$e" );
+			}
+		}
+	}
+
+	/**
+	 * @return array
+	 */
+	public function provideDanglingCommasInVariargs() {
+		return [
+			[ "contains_any('a','b','c',)" ],
+			[ "contains_all(1,1,1,1,1,1,1,)" ],
+			[ "equals_to_any(1,'foo',)" ],
+		];
+	}
+
+	/**
+	 * Ensure that an exception is thrown where there are extra commas in function calls, which
+	 * are not the kind of allowed dangling commas.
+	 *
+	 * @param string $code
+	 * @dataProvider provideExtraCommas
+	 */
+	public function testExtraCommasNotAllowed( $code ) {
+		$this->exceptionTest( 'unexpectedtoken', $code, 'doLevelAtom' );
+	}
+
+	/**
+	 * @return array
+	 */
+	public function provideExtraCommas() {
+		return [
+			[ "norm(,,,)" ],
+			[ "str_replace(,'x','y')" ],
+			[ "contains_any(,)" ],
+			[ "contains_any(,,)" ],
+			[ "contains_any(1,2,,)" ],
+			[ "contains_any(1,2,,3,)" ],
+		];
+	}
 }