Wrong conversion of division to old_div

[wagnerpeer]

Example code

The following code contains an old style division.

(x / 2 * 3.0)

The expected result

The result running the above code with arbitrary values assigned to x should be the following, where the parentheses explicitly show the order of the operations (x / 2) * 3.0. Or written in another order the result should be evaluated as x * (3.0 / 2).

The issue

Once we run futurize on the code above, an old_div gets inserted by the fix_division_safe fixture. However as one can see in the diff below, the function gets called with a wrong order of arguments.

$ futurize --stage2 src/example.py
RefactoringTool: Refactored src/example.py
--- src/example.py      (original)
+++ src/example.py      (refactored)
@@ -1 +1,3 @@
-(x / 2 * 3.0)
+from __future__ import division
+from past.utils import old_div
+(old_div(x, 2 * 3.0))
RefactoringTool: Files that need to be modified:
RefactoringTool: src/example.py

Expected conversion

As already stated in the section 'expected result' the correct conversion should have been: (old_div(x, 2) * 3.0)

[QuLogic]

Or written in another order the result should be evaluated as x * (3.0 / 2).

While mathematically equivalent, this is not equal on Python 2. If x is an int, then x / 2 will truncate first in some cases, whereas (3.0 / 2) will always be 1.5 and never truncate.

Though of course, that doesn't change the bug.

[wagnerpeer]

I also thought so, which is why I used a Python 2.7.15 as well as Python 3.7.1 to run the example and validate that they show the same behavior. It is good to hear that my first intention was not completely wrong.

Maybe it's also connected to the OS? I am running the example on Windows 7 x64.

Can you give an example in which cases Python 2 would evaluate the expression in another way?

[QuLogic]

>>> y = range(10)

>>> [x / 2 * 3.0 for x in y]
[0.0, 0.0, 3.0, 3.0, 6.0, 6.0, 9.0, 9.0, 12.0, 12.0]

>>> [x * (3.0 / 2) for x in y]
[0.0, 1.5, 3.0, 4.5, 6.0, 7.5, 9.0, 10.5, 12.0, 13.5]

(Your Expected conversion is of course the correct result.)

[wagnerpeer]

Totally what I would have expected. However I explicitly tested using 1 for x and got 1.5 as a result.

I also forgot to mention, that there is a different output of the code if you don't use the parentheses. In this case futurize will yield the correct conversion.

1 / 2 * 3.0 - - > old_div(1, 2) * 3.0

[fbessho]

I encountered the same bug and am working on the fix on my branch.
It seems my changes fix this issue as follows:

$ cat sample.py          
x =  10
(x / 2 * 3.0)

Original output

$ futurize sample.py 
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored sample.py
--- sample.py	(original)
+++ sample.py	(refactored)
@@ -1,2 +1,4 @@
+from __future__ import division
+from past.utils import old_div
 x =  10
-(x / 2 * 3.0)
+(old_div(x, 2 * 3.0))
RefactoringTool: Files that need to be modified:
RefactoringTool: sample.py

New output

$ futurize sample.py
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored sample.py
--- sample.py	(original)
+++ sample.py	(refactored)
@@ -1,2 +1,4 @@
+from __future__ import division
+from past.utils import old_div
 x =  10
-(x / 2 * 3.0)
+(old_div(x, 2) * 3.0)
RefactoringTool: Files that need to be modified:
RefactoringTool: sample.py

I'm going to create a pull request when I see time..

[fbessho]

Here are more examples.

I also forgot to mention, that there is a different output of the code if you don't use the parentheses. In this case futurize will yield the correct conversion.

1 / 2 * 3.0 - - > old_div(1, 2) * 3.0

Contrary to what you said, I see 1 / 2 * 3.0 is converted to old_div(1, 2 * 3.0) incorrectly using the current code on master..

$ cat sample2.py
1 / 2 * 3.0
x =  10
x / 2
x / 2.0
x / 2 * 3
x / (2 * 3)
x * 2 / 3

Original output

$ futurize sample2.py
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored sample2.py
--- sample2.py	(original)
+++ sample2.py	(refactored)
@@ -1,7 +1,9 @@
-1 / 2 * 3.0
+from __future__ import division
+from past.utils import old_div
+old_div(1, 2 * 3.0)
 x =  10
-x / 2
+old_div(x, 2)
 x / 2.0
-x / 2 * 3
-x / (2 * 3)
-x * 2 / 3
+old_div(x, 2 * 3)
+old_div(x, (2 * 3))
+old_div(x * 2, 3)
RefactoringTool: Files that need to be modified:
RefactoringTool: sample2.py

New output

$ futurize sample2.py
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored sample2.py
--- sample2.py	(original)
+++ sample2.py	(refactored)
@@ -1,7 +1,9 @@
-1 / 2 * 3.0
+from __future__ import division
+from past.utils import old_div
+old_div(1, 2) * 3.0
 x =  10
-x / 2
+old_div(x, 2)
 x / 2.0
-x / 2 * 3
-x / (2 * 3)
-x * 2 / 3
+old_div(x, 2) * 3
+old_div(x, (2 * 3))
+old_div(x * 2, 3)
RefactoringTool: Files that need to be modified:
RefactoringTool: sample2.py

[fbessho]

PR created (#441)

[mattgathu]

I have also observed that the old_div gets inserted even when both the left and right operands are floats e.g.

1.0/1024.0

becomes

old_div(1.0,1024.0)

I think this conversion is unnecessary since the division where both operands are floats produces the same result, both in Python 2 and Python 3.

[fbessho]

@mattgathu Which version of futurize and Python do you use? With Python 3.7.2 and futurize 0.17.1 (the latest in PyPI), 1.0/1024.0 stays intact when futurized.

$ python --version
Python 3.7.2
$ futurize --version
0.17.1

$ cat test.py
1.0 / 1024.0
1   / 1024.0
1.0 / 1024

$ futurize test.py
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: No files need to be modified.

[mattgathu]

Here are my versions:

$ python -V
Python 2.7.16
$ futurize --version
0.16.0

I have upgraded to 0.17.0 and it seems not to have the issue. Thanks

[fbessho]

Indeed it seems v0.17.0 includes an enhancement to skip unnecessary conversions.

https://github.com/PythonCharmers/python-future/releases/tag/v0.17.0

Fix fix_division_safe to support better conversion of complex expressions, and skip obvious float division.