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().

Good idea!

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()]

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".

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.)

@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.

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.

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.

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.

Did you run tests on the release build?