Page MenuHomePhabricator
Create Task
Maniphest T237610

Syntax validation should flag unused variables
Open, Needs TriagePublic
Actions

Description

It would be nice if Check Syntax can see if there are any unused variables. I have implemented it already (albeit not in PHP) and it does catch mistakes in existing filters:

There are of course some correctly functional filters that do have unused variables, like https://en.wikipedia.org/wiki/Special:AbuseFilter/828. While I will argue that these are nonetheless mistakes, it might be worth implementing a warning mode for Check Syntax (which allows filters to be saved despite the issues it finds) to not totally break the otherwise functional filters.

Details

SubjectRepoBranchLines +/-
Add a static analyzer for the filter languagemediawiki/extensions/AbuseFiltermaster+1 K -171
Customize query in gerrit

Related Objects
Search...

Event Timeline

Nullzero created this task.Nov 7 2019, 9:14 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 7 2019, 9:14 AM
Daimona subscribed.Nov 7 2019, 11:30 AM
DannyS712 renamed this task from Unused variable to Syntax validation should flag unused variables.Nov 12 2019, 11:31 AM
gerritbot added a comment.Nov 20 2019, 7:58 AM

Change 551966 had a related patch set uploaded (by Nullzero; owner: Nullzero):
[mediawiki/extensions/AbuseFilter@master] [DO NOT MERGE] Add a static analyzer for the filter language

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

gerritbot added a project: Patch-For-Review.Nov 20 2019, 7:58 AM
Daimona assigned this task to Nullzero.Dec 17 2019, 1:32 PM
Daimona added a parent task: T260903: Create a static analyzer for the filter language.Aug 20 2020, 12:03 PM
Daimona added a comment.Sep 1 2021, 10:50 PM

I just realized that this can be implemented with non-fatal warnings, available since r650530 / T269770

gerritbot added a comment.Sep 3 2021, 7:52 PM

Change 551966 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Add a static analyzer for the filter language

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

Maintenance_bot removed a project: Patch-For-Review.Sep 3 2021, 8:11 PM
Daimona added a comment.Sep 6 2021, 4:58 PM

I think this should be enabled by default now, using a non-fatal warning as per my comment above. Right now this can't really be implemented cleanly, because the parser and the syntax checker would become more coupled. As a general plan, this should happen after the progress on the "RuleChecker" (r716040). The ParserStatus class could be reworked (and renamed) to have a narrower parent with just errors+warnings (+time?); SyntaxChecker could use that new class, and then there might be a merge() method or something to merge the errors from the evaluator.

Nullzero added a comment.Sep 6 2021, 5:10 PM

That would be really nice! You can also run the liberal mode, catch exceptions, and report the problem as a warning, too. The conservative mode (which is the default mode) aims to be compatible with the current filters, but it could result in the undefined value. The liberal mode, while not compatible with the current filters, will make sure that the undefined value never arises.

Unfortunately, I'm very busy and not free to work on this in foreseeable future.

Daimona mentioned this in T290443: Report errors from the liberal mode of the syntax checker as warnings.Sep 6 2021, 5:41 PM
In T237610#7334755, @Nullzero wrote:

That would be really nice! You can also run the liberal mode, catch exceptions, and report the problem as a warning, too. The conservative mode (which is the default mode) aims to be compatible with the current filters, but it could result in the undefined value. The liberal mode, while not compatible with the current filters, will make sure that the undefined value never arises.

Right, that's a good idea! I wonder if it would be possible to have a single mode, which would report an error whenever the conservative mode does, and a warning where the liberal mode would error. But maybe that's too much, and we can just report a single error at a time by catching exceptions. I've filed T290443 for this.

Unfortunately, I'm very busy and not free to work on this in foreseeable future.

No worries, your work on the syntax checker is already amazing!

Daimona added a subtask: T290454: Create a new RuleChecker class to transform string rules into result objects.Sep 6 2021, 9:16 PM
Aklapper removed Nullzero as the assignee of this task.Mar 13 2022, 7:42 PM

Removing task assignee due to inactivity, as this open task has been assigned for more than two years. See the email sent to the task assignee on February 06th 2022 (and T295729).

Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.

If this task has been resolved in the meantime, or should not be worked on ("declined"), please update its task status via "Add Action… 🡒 Change Status".

Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL