Skip to content

gh-119594: Improve pow(fraction.Fraction(), b, modulo) error message #119593

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 29, 2024
Merged

gh-119594: Improve pow(fraction.Fraction(), b, modulo) error message #119593

merged 3 commits into from
May 29, 2024

Conversation

wimglenn
Copy link
Contributor

@wimglenn wimglenn commented May 27, 2024 •
edited
Loading

Fraction.__pow__ does not accept the third argument which pow tries to pass to it:

>>> pow(Fraction(1), 1, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Fraction.__pow__() takes 2 positional arguments but 3 were given

Other number types don't do that, they support it when they can and print an error message if they don't want to, e.g.

>>> pow(Decimal("1"), 1, 1)
Decimal('0')
>>> pow(Decimal('1'), 1.1, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: pow() 3rd argument not allowed unless all arguments are integers

I'm not sure what fraction should do, maybe it should just continue not to support it? But accept the argument, and then print a better error message if modulo is not None?

Maybe it is worth adding tests for all the possible coercions too (ref: https://peps.python.org/pep-0208/)

@wimglenn wimglenn changed the title fractions.Fraction __pow__ supports optional modulo argument gh-119594: fractions.Fraction __pow__ to accept optional modulo argument May 27, 2024
@bedevere-app bedevere-app bot mentioned this pull request May 27, 2024
Closed
terryjreedy
terryjreedy requested changes May 27, 2024
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fraction.__pow__ should act same as float.__pow__. This new code does not. pow(3.0, 4, 5) and pow(3.5, 4, 5)` raise "TypeError: pow() 3rd argument not allowed unless all arguments are integers" Put following at top of code:

        if mod is not None:
            raise TypeError("pow() 3rd argument not allowed unless all arguments are integers")

and correct the new test.

@bedevere-app
Copy link

bedevere-app bot commented May 27, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@mdickinson
Copy link
Member

Slight modification to Terry's suggestion: I think we can do:

if mod is not None:
    return NotImplemented

and then the general machinery should construct a reasonable error message for us. And in theory this gives other classes a chance to participate where Fraction declined to do so, but I'm not sure the delegation rules for ternary operations are particularly well-defined.

@wimglenn
Copy link
Contributor Author

wimglenn commented May 27, 2024 •
edited
Loading

I have made the requested changes; please review again.

Should __rpow__ also get the same treatment? Interestingly, pow(1, Fraction(1), 1) already gives the right error message but it looks as though Fraction.__rpow__ does not get invoked at all in that case. I don't quite understand why.

edit: Found a sort-of answer in the docs, where it says

Note that ternary pow() will not try calling __rpow__() (the coercion rules would become too complicated).

The ternary coercion rules in PEP 208 do seem complicated. So according to the status quo, seems a third argument for __rpow__ would be pointless (it won't be used without changes in the C code).

@bedevere-app
Copy link

bedevere-app bot commented May 27, 2024

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from terryjreedy May 27, 2024 19:33
@mdickinson
Copy link
Member

The ternary coercion rules in PEP 208 do seem complicated. So according to the status quo, seems a third argument for __rpow__ would be pointless (it won't be used without changes in the C code).

Agreed, I think. I don't think it would be actively wrong to add the check to __rpow__, but it does look as though it wouldn't achieve anything (and so the status quo wins). Thanks for tracking down the relevant source - that was on my list of things to do.

mdickinson
mdickinson reviewed May 28, 2024
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed May 28, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not __rpow__ be modified too?

@mdickinson
Copy link
Member

mdickinson commented May 28, 2024 •
edited
Loading

Should not __rpow__ be modified too?

The conclusion that @wimglenn came to in the comments above (and that I think I agree with) is that __rpow__ is never called for a ternary pow() call, so modifying __rpow__ would be pointless. I've just been staring at the relevant source code, and it's tangled enough that I can't tell for sure whether that's true or not.

@serhiy-storchaka
Copy link
Member

Ah, sorry, I did not read the discussion before looking at the code in this PR. I experimented and was not able to trigger calling __rpow__. It seems that it is only called in slot_nb_power_binary, which is only used when the third argument is None.

@mdickinson
Copy link
Member

I experimented and was not able to trigger calling __rpow__.

Yep, same here.

mdickinson
mdickinson approved these changes May 29, 2024
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@terryjreedy Okay to merge?

@terryjreedy terryjreedy changed the title gh-119594: fractions.Fraction __pow__ to accept optional modulo argument gh-119594: Improve pow(a,b,modulo) error message if fractions.Fraction and modulo passed May 29, 2024
@terryjreedy terryjreedy changed the title gh-119594: Improve pow(a,b,modulo) error message if fractions.Fraction and modulo passed gh-119594: Improve pow(fraction.Fraction(), b, modulo) error message May 29, 2024
@terryjreedy
Copy link
Member

I am changing title and blurb to reflect user visible effect of change. Will then merge.

@terryjreedy terryjreedy merged commit fcca08e into python:main May 29, 2024
33 checks passed
@wimglenn wimglenn deleted the pow3-fractions branch May 29, 2024 18:05
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@wimglenn @mdickinson @noahbkim
pythongh-119594: Improve pow(fraction.Fraction(), b, modulo) error me…
5667d7e 
…ssage (python#119593)

If one calls pow(fractions.Fraction, x, module) with modulo not None, the error message now says that the types are incompatible rather than saying pow only takes 2 arguments.  Implemented by having fractions.Fraction __pow__ accept optional modulo argument and return NotImplemented if not None.  pow() then raises with appropriate message.
---------

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@wimglenn @mdickinson @estyxx
pythongh-119594: Improve pow(fraction.Fraction(), b, modulo) error me…
434ec19 
…ssage (python#119593)

If one calls pow(fractions.Fraction, x, module) with modulo not None, the error message now says that the types are incompatible rather than saying pow only takes 2 arguments.  Implemented by having fractions.Fraction __pow__ accept optional modulo argument and return NotImplemented if not None.  pow() then raises with appropriate message.
---------

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
scoder added a commit to scoder/quicktions that referenced this pull request Nov 28, 2024
@scoder
Add working test from python/cpython#119593
123fa7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@terryjreedy terryjreedy terryjreedy approved these changes

@mdickinson mdickinson mdickinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wimglenn @mdickinson @serhiy-storchaka @terryjreedy