Okay, the answer is simple and somewhat disturbing: the new parser just failed to parse the filter in question with the syntax error, so it presumably never got executed. I've expected this to fail more loudly than silently failing open, but I am not sure how the filter edit hooks actually handle this.

The specific parsing failure here is that in the old parser both of the following worked:

contains_any('foobar', 'foo', 'bar')

contains_any('foobar', 'foo', 'bar',)

Now, those are not strictly speaking equivalent. f(x,y) calls a function with two arguments, and f(x,y,) calls it with three arguments, last being null. Most functions would fail due to argument count mismatch, but
contains_any is vararg, so null gets turned into "", which then gets treated as no-op (see T62203), unless you have fss_* functions, in which case I have no idea what it actually does, but presumably the same thing, since this actually works in production.

I'll have to figure out how to reproduce this behavior, but I first have to reason carefully about what other funny comma placement can happen here.

$ python -c 'import this' | grep implicit
Explicit is better than implicit.

I'd suggest that a lone comma before the parenthesis should either fail loudly, or be parsed as if the comma weren't there. I'm not sure about php, but python 3 ((lambda *args: print(args))(1,2,3,)) treats the comma as non-existent and javascript / ecmascript 6 (((...args) => console.log(args))(1,2,3,);) treats it as a syntax error.

In T153251#2884699, @vvv wrote:

contains_any is vararg, so null gets turned into "", which then gets treated as no-op (see T62203), unless you have fss_* functions, in which case I have no idea what it actually does, but presumably the same thing, since this actually works in production.

Wikimedia uninstalled fss a while back. It's no longer promoted for PHP 5.5+ installs (so MW 1.27+) since the implementation in PHP is much faster. See https://www.mediawiki.org/wiki/FastStringSearch.

In T153251#2884737, @zhuyifei1999 wrote:

I'd suggest that a lone comma before the parenthesis should either fail loudly, or be parsed as if the comma weren't there.

Given that these filters exist already, and there is no mechanism to warn the users of incompatible filters when upgrading or changing config settings, I think ignoring the trailing comma would be a reasonable option. There are 53 affected filters on 32 Wikimedia wikis (T153217#2888136; 73 filters on 33 wikis, if you could the disabled/deleted ones).

So what should we do? My proposal is to throw an error if the function is not variadic, and take a null argument otherwise. At any rate, this would take a great benefit from the runtime error page introduced in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/429381/ (failures would be listed there, and not be silent).

I think both either of these are good ideas for moving forward.

1. Ignore all trailing commas (unconditionally, not function-dependant). This would change behaviour from today, but keep the filters enabled (today they cause an additional null/empty string parameter, possibly leading to false positive/negative outcome of filters).
2. Disallow trailing commas in the syntax. If I understand correctly, this would lead to the filter effectively being auto-disabled, with AF-admins being able to inspect them on the AF-Problems page.

I slightly lean in favour of option 1, because it would likely be easier to implement, with lower risk/impact. And thus get us to resolving the parent task sooner, which is more important in my opinion. We can always integrate this with the AF-Problems page at a later time, after we get caching working.

The problem here is the usual one. Option A is less sane, but should be totally back-compat (unless a function call relied on this feature to pass a null parameter, but that would be crazy). Option B is the one I like most, since it helps having a saner syntax. However, it would break any filter using trailing commas. Unfortunately, filter editor won't have any way to know that, as filter failures are only sent to logstash. The patch for it is https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/429381/ (and related ones), which adds a new page for filter failures at runtime.

However, I also agree with the last two lines. Specifically, we can first start ignoring trailing commas (and maybe send a debug log whenever we find them), and then completely disallow them as we'll have an on-wiki page to monitor filter failures.

Change 483707 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Ignore trailing commas in function calls

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

Change 483707 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Ignore trailing commas in function calls

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

Change 483707 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Ignore trailing commas in function calls

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

So, now wmf.7 is everywhere and it includes the logging. The patch I wrote sends data to debug files, so I cannot see it on logstash. @Krinkle could you please grep the log files to see how many entries we have for "Found null param for function"? Ideally, we'd need it on a per-filter basis. However, the Parser is currently unaware of the filter it is parsing (although it will know in the future). Thus, I guess we're fine with a per-wiki count. Thanks!

Logstash shows many occurrencies of channel:AbuseFilter AND "Empty operand of type function". Like for T156096#5407058, we have to add the filter ID.

Change 529474 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add the filter ID to empty operand logging

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

Change 529474 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add the filter ID to empty operand logging

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

List of broken filters, to be announced together with T156096#5464310:

• ruwiki 14
• enwiktionary 82
• zhwiki 6, 31, 38, 50, 53, 55, 64
• cswiki 2, 74
• commonswiki 31, 206
• jawiki 18, 80, 105, 106
• fiwiki 124
• fawiki 3, 9, 19, 33
• nowiki 4
• zh_yuewiki 3, 20
• kkwiki 3
• dawiki 11
• elwiki 3
• hiwiki 43, 61, 80
• mlwiki 3
• nnwiki 5
• mrwiki 21
• zh_classicalwiki 18, 19
• jawikisource 1, 2, 4
• jawikibooks 2
• fawiktionary 9
• eswiktionary 6
• fawikiquote 2, 25, 26
• jawikiquote 1
• fawikivoyage 2
• fawikinews 23
• fawikibooks 10
• zhwikinews 12

Given the high amount of affected filters, I'd like to wait a bit. We could also tolerate dangling commas in variadic functions. I'd like to take some time to discuss whether that's a best option, and maybe implement it.

Change 534842 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Allow dangling commas in variargs

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

Change 534842 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Allow dangling commas in variargs

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

@Daimona Looks like this is resolved?

In T153251#5479553, @Krinkle wrote:

@Daimona Looks like this is resolved?

Not yet... Ideally, we'd need to:

• Throw AFPExceptions instead of TypeErrors (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/535169/)
• See what filters are left to fix in production after the patch above (https://gerrit.wikimedia.org/r/534842)
• Fix them, if any.

The rest can be done as part of T156096.

In T153251#5479572, @Daimona wrote:

• Throw AFPExceptions instead of TypeErrors (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/535169/)

Ah, actually that was handled separately for func args in https://gerrit.wikimedia.org/r/#/c/534842/. So only checking & fixing on-wiki filters is left to do.

Linked version of T153251#5464352:

• ruwiki: filter 14
• enwiktionary: filter 82
• zhwiki: filter 6, filter 31, filter 38, filter 50, filter 53, filter 55, filter 64
• cswiki: filter 2, filter 74
• commonswiki: filter 31, filter 206
• jawiki: filter 18, filter 80, filter 105, filter 106
• fiwiki: filter 124
• fawiki: filter 3, filter 9, filter 19, filter 33
• nowiki: filter 4
• zh_yuewiki: filter 3, filter 20
• kkwiki: filter 3
• dawiki: filter 11
• elwiki: filter 3
• hiwiki: filter 43, filter 61, filter 80
• mlwiki: filter 3
• nnwiki: filter 5
• mrwiki: filter 21
• zh_classicalwiki: filter 18, filter 19
• jawikisource: filter 1, filter 2, filter 4
• jawikibooks: filter 2
• fawiktionary: filter 9
• eswiktionary: filter 6
• fawikiquote: filter 2, filter 25, filter 26
• jawikiquote: filter 1
• fawikivoyage: filter 2
• fawikinews: filter 23
• fawikibooks: filter 10
• zhwikinews: filter 12

I'm going to sample-check some of them to see how many of those are for variadic functions (which will be allowed since this week).

wmf.22 is now everywhere, so I checked logstash. We don't have any filter using trailing commas for non-variadic functions. Hence, I'm going to close this task per plan in the description.