Page MenuHomePhabricator

Array appending causes a crash
Closed, ResolvedPublicBUG REPORT

Description

Steps to Reproduce:

At https://th.wikipedia.org/wiki/Special:Abusefilter/test, paste the following code:

a := [];
a[] := 0

and then click the "Test" button

Actual Results:

An internal error occurs:

[XbkMfApAICoAAHOmRuQAAAAL] 2019-10-30 04:07:24: การยกเว้นข้อผิดพลาดร้ายแรงของประเภท "TypeError"

Expected Results:

I think the semantics of <e1>[] := <e2> should be, mutating <e1> to append <e2>, and the whole expression evaluates to <e2>.

Event Timeline

Nullzero created this task.Oct 30 2019, 4:08 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 30 2019, 4:08 AM
Nullzero updated the task description. (Show Details)Oct 30 2019, 4:12 AM

Change 547171 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Fix AbuseFilterCachingParser violating return type constraint

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

Daimona claimed this task.Oct 30 2019, 11:30 AM
Nullzero added a comment.EditedOct 30 2019, 5:00 PM

Hi Daimona,

I just installed the latest version of abuse filter on my local wiki and found that this issue doesn't occur there, so I am now very confused. Could it be the case that the extension in Thai Wikipedia is very outdated and the issue was already fixed? The fact that you just submitted a patch confuses me even more because it suggests that there's still a bug...

I just installed the latest version of abuse filter on my local wiki and found that this issue doesn't occur there, so I am now very confused.

That's actually expected. The issue only happens with the new AF parser (called "AbuseFilterCachingParser"), which was recently enabled on all wikis (see e.g. T156095 or tech news). Since it's still beta (sort of), it's not enabled by default, hence it's not enabled on your wiki.

Could it be the case that the extension in Thai Wikipedia is very outdated and the issue was already fixed?

As a side note, this isn't possible. All extensions are updated whenever MediaWiki is updated, for all wikis. The only situation where two different wikis can have different versions is during the weekly rollout, see e.g. https://wikitech.wikimedia.org/wiki/Deployments/One_week for some info.

Nullzero added a comment.EditedOct 30 2019, 9:57 PM

I was led to believe that Thai Wikipedia was outdated because the syntax () is not valid in Wikipedias, but is valid in my local wiki. Now I see that this is intentionally disallowed in the new parser (https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/parser/AFPTreeParser.php$554), which is a good thing IMO. Should we disallow () too in AbuseFilterParser for consistency?

Nullzero added a comment.EditedOct 30 2019, 11:06 PM

Off-topic: AbuseFilterCachingParser should also really be called an interpreter or evaluator and moved outside of the parser directory... There's no parsing here.

I was led to believe that Thai Wikipedia was outdated because the syntax () is not valid in Wikipedias, but is valid in my local wiki. Now I see that this is intentionally disallowed in the new parser (https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/parser/AFPTreeParser.php$554), which is a good thing IMO.

Exactly :) The new parser brings some sanity to the AF syntax. See also the examples at T156096, which are all banned in the new parser.

Should we disallow () too in AbuseFilterParser for consistency?

I'm trying not to change the old parser anymore, since it will eventually be removed. Let's give people the possibility to upgrade with no (much hurry). The old parser will already emit a warning if it finds a syntax that is forbidden in the new parser.

Off-topic: AbuseFilterCachingParser should also really be called an interpreter or evaluator

For sure. And the "Caching" prefix should also be removed. But that should be done as soon as the other parser is killed. Personally, I was thinking to "Evaluator", but let's see.

and moved outside of the parser directory... There's no parsing here.

Actually, it should stay in that directory, which contains *everything* related to parsing (e.g. also data types, parsing exceptions etc.)

Actually, it should stay in that directory, which contains *everything* related to parsing (e.g. also data types, parsing exceptions etc.)

To be picky, the precise meaning for parsing (as it's used in the programming language research community) is the process that turns a textual data into an abstract syntax tree. Anything else (e.g., type checking/inference, static analysis, interpretation, compilation) is not parsing.

But again, this is really a minor issue. I don't really care about it much.

Actually, it should stay in that directory, which contains *everything* related to parsing (e.g. also data types, parsing exceptions etc.)

To be picky, the precise meaning for parsing (as it's used in the programming language research community) is the process that turns a textual data into an abstract syntax tree. Anything else (e.g., type checking/inference, static analysis, interpretation, compilation) is not parsing.

But again, this is really a minor issue. I don't really care about it much.

I understand. But what I meant is, it's only a directory name, and that directory contains classes useful in various steps of the parsing+evaluation process. It's not meant to distribute classes very precisely, it's just a way of grouping everything that has to do with the AF scripting language (and not, say, special pages or API modules). If we were to be that precise, we'd have lots of folders like "tokenizer/", "evaluator/", "data-types/", "ast-elements/" etc., each containing one or two files. That wouldn't very practical, would it? :)

Change 547171 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Fix AbuseFilterCachingParser violating return type constraint

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

Daimona closed this task as Resolved.Dec 27 2019, 2:42 PM