Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confused traceback while floordiv or mod happens between Fraction and complex objects #102840

Closed
Eclips4 opened this issue Mar 20, 2023 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@Eclips4
Copy link
Member

Eclips4 commented Mar 20, 2023

from fractions import Fraction
a = Fraction(1, 2)
b = a // 1j
>
Traceback (most recent call last):
  File "C:\Users\KIRILL-1\CLionProjects\cpython\example.py", line 5, in <module>
    b = a // 1j
        ~~^^~~~
  File "C:\Users\KIRILL-1\AppData\Local\Programs\Python\Python311\Lib\fractions.py", line 363, in forward
    return fallback_operator(complex(a), b)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: unsupported operand type(s) for //: 'complex' and 'complex'

This traceback really confused me.
However, it's easy to fix, just need change these lines:

cpython/Lib/fractions.py

Lines 620 to 621 in 5e6661b

elif isinstance(b, complex):
return fallback_operator(complex(a), b)

To this:

elif isinstance(b, complex):
    try:
        return fallback_operator(complex(a), b)
    except TypeError:
        raise TypeError(
            "unsupported operand type(s) for %r: %r and %r" % (
                fallback_operator.__name__,
                type(a).__qualname__,
                type(b).__qualname__)
        ) from None

So, after that, we have a pretty nice traceback:

Traceback (most recent call last):
  File "C:\Users\KIRILL-1\CLionProjects\cpython\example.py", line 5, in <module>
    b = a // 1j
        ~~^^~~~
  File "C:\Users\KIRILL-1\CLionProjects\cpython\Lib\fractions.py", line 624, in forward
    raise TypeError(
TypeError: unsupported operand type(s) for 'floordiv': 'Fraction' and 'complex'

But, here a one problem - would be nice to have in traceback originally // instead of floordiv. I have no idea how to do it, without create a mapping with names. Theoretically - we can add a special attribute for it in operator, but it should be a another discussion.

Linked PRs

@Eclips4 Eclips4 added the type-bug An unexpected behavior, bug, or error label Mar 20, 2023
@Eclips4
Copy link
Member Author

Eclips4 commented Mar 20, 2023

In fact, this case is also suitable for mod.

@Eclips4 Eclips4 changed the title Confused traceback while floor division happens between Fraction and complex objects Confused traceback while floordiv or mod happens between Fraction and complex objects Mar 20, 2023
@skirpichev
Copy link
Contributor

Maybe it could be fixed on this way...

diff --git a/Lib/fractions.py b/Lib/fractions.py
index c95db0730e..fe3a5d137b 100644
--- a/Lib/fractions.py
+++ b/Lib/fractions.py
@@ -611,16 +611,18 @@ class doesn't subclass a concrete type, there's no
 
         """
         def forward(a, b):
-            if isinstance(b, Fraction):
-                return monomorphic_operator(a, b)
-            elif isinstance(b, int):
-                return monomorphic_operator(a, Fraction(b))
-            elif isinstance(b, float):
-                return fallback_operator(float(a), b)
-            elif isinstance(b, complex):
-                return fallback_operator(complex(a), b)
-            else:
-                return NotImplemented
+            try:
+                if isinstance(b, Fraction):
+                    return monomorphic_operator(a, b)
+                elif isinstance(b, int):
+                    return monomorphic_operator(a, Fraction(b))
+                elif isinstance(b, float):
+                    return fallback_operator(float(a), b)
+                elif isinstance(b, complex):
+                    return fallback_operator(complex(a), b)
+            except TypeError:
+                pass
+            return NotImplemented
         forward.__name__ = '__' + fallback_operator.__name__ + '__'
         forward.__doc__ = monomorphic_operator.__doc__
 

(Ditto for the reverse().)

Here is a traceback:

>>> fractions.Fraction(1,2)//1j
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for //: 'Fraction' and 'complex'

@Eclips4
Copy link
Member Author

Eclips4 commented Mar 20, 2023

Thanks!
Thats looking more pythonic, and this solves the problem, btw, tests are green. I'll send a PR.

@ericvsmith
Copy link
Member

As a rule we don't do type checking like this. Is the original traceback really that confusing?

@Eclips4
Copy link
Member Author

Eclips4 commented Mar 20, 2023

Docs/fractions say nothing about convert Fraction to complex.
For new user, traceback which says that floordiv cannot be applied between two complex, really confusing.
User try to floordiv Fraction and complex, but in traceback he saw a two complex objects.
IMO

@ericvsmith
Copy link
Member

Yes, but the code in the traceback shows that it's converting one argument to complex, so that explains one of them. But maybe I overestimate what people can figure out from the traceback.

@Eclips4
Copy link
Member Author

Eclips4 commented Mar 20, 2023

Assessment of what can be understood by humans within traceback - really hard and subjective thing, so I can't say anything about this at all.
However, PR which related with this issue, doesn't break backward compatibility, just make prettier traceback, I think, we should take in account that.

@terryjreedy
Copy link
Member

@ericvsmith: "As a rule we don't do type checking like this. " We already are doing 'this' as the proposal just wraps an existing isinstance chain in try: except. I definitely like the revised message better.

@ericvsmith
Copy link
Member

@terryjreedy: Thanks. I was trying to get across that in general we don't try to improve messages where duck typing fails.

But you're correct, in this case we are already dispatching based on the type of a parameter, so my argument is weak. I think the better solution here might be to not try to convert the numerator to a complex value, since we know that a_complex // b_complex isn't going to work. But that would require some surgery to make // a special case, different from say a_complex * a_float.

@arhadthedev arhadthedev added the stdlib Python modules in the Lib dir label Mar 22, 2023
@mdickinson
Copy link
Member

I'm not convinced that this is worth the effort; it seems like an unlikely case to even hit in the first place.

serhiy-storchaka pushed a commit that referenced this issue Feb 10, 2024
@Eclips4 Eclips4 closed this as completed Feb 10, 2024
@serhiy-storchaka
Copy link
Member

I would like to backport this change (the final version), but other coredevs were skeptical. Confusing error message is not a large bug.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

7 participants