Page MenuHomePhabricator

Parser: wrong exception thrown for arrays
Closed, ResolvedPublic

Description

While writing https://gerrit.wikimedia.org/r/#/c/443217/ I found out some wild syntax that drives the parser mad. Here are three cases, try them in debugging tools:

  1. Write: a := [1,2,] Result: Valid syntax. This isn't a big deal, but shouldn't happen anyway. Also, the created array is actually [1,2], not [1,2,null], so no reason to allow it. Trailing commas may actually be useful
  2. Write: a := [ [ 1, 2, 3 ], [ 4, 5, 6 ] ]; lcase(a[1][2 ) Result: Bye bye. The parser enters some infinite loop and the spinner keeps running like it happened in T134124. Sorry, this was due to some debugging line I left in the code.
  3. Write: added_lines[1] = 5 Result: The exception raised is unrecognisedvariable. Since when checking syntax we don't have added_lines, I'm not sure about what should happen, but either we treat it as valid syntax (probably the best bet, since added_lines will likely be an array at runtime) or throw the notarray exception.

Update: So, the only problem here is point 3. I looked at it, and it's not trivial: if we decide to forgive the syntax, other exceptions are thrown, the last ones being in doLevelArrayElements, where we don't have enough context to determine whether we should let the syntax pass. I'll try again, but probably a more stable solution could be to clearly distinguish between really NULL variables and variables not yet computed.

Event Timeline

Daimona triaged this task as Medium priority.
Daimona moved this task from Backlog to Internal bugs on the AbuseFilter board.

Oh, there's another way out: let NULL be used as array. So, if a := null, we would have a[0] == null & a[1] == null & a[13468661725876] == null. A little dirty, but quicker.

Daimona renamed this task from Syntax check gets tripped by wild syntax to Parser: wrong exception thrown for arrays.Oct 11 2018, 4:32 PM

Change 527841 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove outdated comment about a fixed task

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

Daimona claimed this task.
Daimona removed a project: Patch-For-Review.

This was coincidentally fixed as part of T214674 with https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/527520/.

Now, unavailable variables have the DNONE type, and trying to access an offset returns another DNONE. Basically, this is T198531#4327293 but using the special data type mentioned in the task description.

The patch above only removes a comment mentioning this task, and has this task's ID in the commit message just for reference, thus calling resolved (and removing patch-for-review before we forget to).

Daimona added a project: Patch-For-Review.

It hasn't been fixed. We also need to allow built-in variables in doLevelSet. Either way, now the fix is easy and is included in the patch above.

Change 527841 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Allow accessing offsets of built-in variables

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

Daimona removed a project: Patch-For-Review.