Page MenuHomePhabricator

Treat integers as such in performing arithmetics
Closed, ResolvedPublic

Description

While writing some parserTests, I noticed that 1 + 1 === 2 evaluates to false! I still have to check, but I suspect operations are unnecessarily performed with float numbers, like it happened for edit_delta.

Event Timeline

Daimona created this task.Apr 7 2018, 9:12 AM
Restricted Application added subscribers: Scoopfinder, Aklapper. · View Herald TranscriptApr 7 2018, 9:12 AM
Daimona moved this task from Backlog to Internal bugs on the AbuseFilter board.Apr 7 2018, 9:12 AM
Daimona triaged this task as Normal priority.

Yeah, I was right. 1 + 1 === 2.0. I hope to send a patch later, it should be enough to cast to float iff at least one of the operands is float.

Daimona added a comment.EditedApr 7 2018, 10:40 AM

Oh wow, another weird discover: ladies and gentlemen, 1 / 2 === 0 is true!!! And so is e.g. 12 / 37, while integer division works well (36 / 12 === 3).
So, recapping, we treat integers as float when summing, and leave them as integers when dividing. Not bad.

From other parser tests I realized that the division behaviour is sort of intended: 5/2 === 2, 5. / 2 ===2.5. However, this isn't how it should work. Instead, it should be 5/2 === 2.5 and int(5/2) === 2, like anyone would expect.

Change 424753 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use integers in addition and subtractions

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

Change 424754 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Convert division/multiplication/modulo results after calculation

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

Change 424755 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Properly use integers in exponentiation

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

Daimona claimed this task.Apr 7 2018, 12:34 PM

There may be regressions from these patches. My suggestion is to run some queries to determine if there really is any risk. This should only affect uses of '===' and '!==' (type check) and of float numbers, leaving everything else unchanged. So basically those are the 3 things we should run queries for.

I decided for a different approach, i.e. use the same type as PHP. This leads to more uniform results and documentation. As for regressions: the main case here is when using triple compare operators like I was saying above, however there's an exception for the div/mul/mod patch: the result of division. Right now, 5/2 = 4 (this also contradicts the MW manual), which is highly undesirable. It'll produce a change in the value itself of the operation, so we may need to query usages of division.

Huji added a subscriber: Huji.Apr 9 2018, 1:59 PM

You mean right now 5/2 = 3, right?

Almost, right now 5/2 === 2, so not even rounded. It's simply casted to int, but we should do the opposite, e.g. return a float and the user may cast to int according to their needs.

Huji added a comment.Apr 10 2018, 2:35 AM

Agreed. So now, what is your plan for analysis of possible regressions?

Daimona added a comment.EditedApr 10 2018, 5:49 AM

My plan is not to have a plan :-) Jokes apart, I really can't see an easy way to analyse existing filters. Something to begin with could be to run a query for every use of triple-equality operators, since I don't think they're widespread in filters. Then, we need a separate query for divisions. Depending on the amount of data, we may then go on by either refining queries or fixing what is found.

Oh, and in case we won't be able to gather useful data from queries, then we'd need to add wfDebug to the changed functions so that they're only called if the result is different than before.

So... for the triple-check part, we can safely go with the following query:

select af_id from abuse_filter where af_pattern rlike '[!=]=='

I tried it on itwiki and got no results, and I believe there won't much neither in other wikis.
For the division part, we need a regex to only match slashes used for division. And to me, at the moment, it's not easy to think of a perfect, simple regex to do it without false positives.

Huji added a comment.EditedApr 10 2018, 11:22 PM

I think the number of cases in which this may change the meaning of a mathematical operation in a filter is so few (possibly even nonexistent) that we should go ahead and merge this.

Change 424753 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use integers in addition and subtractions

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

Change 424754 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Convert division/multiplication/modulo results after calculation

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

Change 424755 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Properly use integers in exponentiation

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

Huji edited projects, added User-notice; removed Patch-For-Review.Apr 10 2018, 11:42 PM
Huji closed this task as Resolved.