bpo-39871: Fix possible SystemErrors in math.{atan2,copysign,remainder}()

[ZackerySpytz]

In math_2(), the first PyFloat_AsDouble() call should be checked
for failure before the second call.

https://bugs.python.org/issue39871

[mdickinson]

LVGTM!

[vstinner]

The purpose of this method wasn't obvious at the first look. Would you mind to add a comment explaining that it's written to ensure that the float() is never called?

Maybe you could use a mock object and ensure that mock.__float__.assert_not_called().

[serhiy-storchaka]

Good idea!

[mdickinson]

Using mock doesn't seem to work well as a regression test here. I'm not sure why. For example, on Python 3.8.2:

>>> import math, unittest.mock
>>> y = unittest.mock.MagicMock()
>>> math.atan2("not a number", y)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/mock.py", line 2111, in __get__
    return self.create_mock()
TypeError: must be real number, not str
>>> y.__float__.mock_calls  # expect to see one call here, but there's nothing
[]

Contrast with:

>>> math.atan(y)
0.7853981633974483
>>> y.__float__.mock_calls
[call()]

[serhiy-storchaka]

It looks correct to me. In the first example y.__float__ is not called, as expected, because an error is raised for "not a number".

[mdickinson]

This was on Python 3.8.2, which still has the bug that this PR solves: namely, both arguments are converted to float before checking for errors. So the expectation is that because the bug is still present, y.__float__ is still called. (And then with Zackery's fix, of course, it won't be any more.)

[mdickinson]

@serhiy-storchaka: I'm quite baffled here; I really don't understand what your objection is (if you have an objection), or what change you're suggesting (if you're suggesting a change). Victor suggested using mock.__float__.assert_not_called(). You said that was a good idea. I pointed out that that doesn't work, but tweaked the test to add something equivalent that does work. As far as I'm concerned, the bug is fixed, it has a regression test, and there's nothing more to do here.

If there's something you feel needs to change, please open a new PR. If not, please let's stop wasting time and energy on a bug that's already fixed.

[serhiy-storchaka]

I do not suggest any changes. To me, there is absolute no difference between using a handmade class with the __float__ method and a mock, except that the latter would be several lines shorter. In both cases the test crashes without the fix applied and passed with the fix applied. This is why it looked a good idea to me -- simpler solution is better.

I am just puzzled why do you see a difference.

[mdickinson]

Consider the following two unit tests:

    def test_with_mock(self):
        y = unittest.mock.MagicMock()
        try:
            math.atan2("not a number", y)
        except:
            pass
        y.__float__.assert_not_called()

    def test_without_mock(self):
        class F:
            def __init__(self):
                self.float_called = False

            def __float__(self):
                self.float_called = True
                return 1.0

        y = F()
        try:
            math.atan2("not a number", y)
        except:
            pass
        self.assertFalse(y.float_called)

Both tests assert the exact same thing: that the failed math.atan2 call did not call ys __float__ method. The first test uses a MagicMock. The second uses a custom class.

So the tests should be equivalent. However: before the fix in this PR, the first test will pass, while the second one will fail. (After the fix, they both pass, of course.)

Please note: I am not claiming that these are good unit tests; I'm just trying to demonstrate the difference I explained earlier in an explicit way.

[serhiy-storchaka]

I do not claim that the current test is bad. But both tests crash before the fix and I do not see a difference between these crashes. They are equivalent to me.

[serhiy-storchaka]

Did you run tests on the release build?

[miss-islington]

Thanks @ZackerySpytz for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry, @ZackerySpytz and @mdickinson, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5208b4b37953a406db0ed6a9db545c2948dde989 3.8

[miss-islington]

Sorry @ZackerySpytz and @mdickinson, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5208b4b37953a406db0ed6a9db545c2948dde989 3.7

[bedevere-bot]

GH-18989 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

GH-18990 is a backport of this pull request to the 3.7 branch.