Skip to content

gh-113570: reprlib.repr does not use builtin __repr__ for reshadowed builtins #113577

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

Merged
merged 17 commits into from
Oct 17, 2024

Conversation

georgepittock
Copy link
Contributor

@georgepittock georgepittock commented Dec 30, 2023

Fixes issue: gh-113570 where objects that reshadow certain builtins would call a specific method and could cause errors, e.g.

import reprlib

class array:
    def __repr__(self):
        return "not array.array!"

reprlib.repr(array())
# previously raised an error, now "not array.array"

This uses a mapping of (module, object name): method to map to the right method, as mentioned in the issue.

@ghost
Copy link

ghost commented Dec 30, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Dec 30, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, also take a look at this doctest failure:

 Document: library/reprlib
-------------------------
**********************************************************************
File "library/reprlib.rst", line 481, in default
Failed example:
    import reprlib
    import sys

    class MyRepr(reprlib.Repr):

        def repr_TextIOWrapper(self, obj, level):
            if obj.name in {'<stdin>', '<stdout>', '<stderr>'}:
                return obj.name
            return repr(obj)

    aRepr = MyRepr()
    print(aRepr.repr(sys.stdin))         # prints '<stdin>'
Expected:
    <stdin>
Got:
    <_io.TextIOWr...oding='utf-8'>
1 items passed all tests:
   8 tests in indent
**********************************************************************
1 items had failures:
   1 of   7 in default
15 tests in 2 items.
14 passed and 1 failed.
***Test Failed*** 1 failures.

@georgepittock
Copy link
Contributor Author

To handle the failing doctest some more edge cases need to be handled so cases where someone has defined a custom method still work.

To keep something somewhat similar to what we had there a (4) cases. I am saying there is a predefined method for an object of type a if the class has a method repr_x.

  1. No predefined method
  2. There is a predefined method that is not in our lookup (user defined in a subclass)
  3. There is a predefined method, that is in our lookup and has the same module and type as what we expect
  4. There is a predefined method, that is in our lookup and has a different module and type as what we expect (reprlib (used by pytest) infers builtin types based on class __name__ #113570)

In cases 1 and 4 we will return self.repr_instance(x, level), but in 2 and 3 getattr(self, predefined_method_name)(x, level)

I have added an explicit unit test for case 2 in my latest commit, cases 1, 3 and 4 are already there.

@georgepittock georgepittock changed the title gh-113570: reprlib.repr does not use builtin repr for reshadowed builtins gh-113570: reprlib.repr does not use builtin __repr__ for reshadowed builtins Jan 2, 2024
@georgepittock
Copy link
Contributor Author

@sobolevn are you able to look at the updated changes from your comments?

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Generally, looks ok to me, but there are several nitpicks :)

@georgepittock
Copy link
Contributor Author

Fixed those two comments @sobolevn

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

This is the last comment, thank you! :)

@georgepittock
Copy link
Contributor Author

georgepittock commented Jan 29, 2024

That's sorted @sobolevn !

@georgepittock
Copy link
Contributor Author

Is this all good now @sobolevn ?

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.

Please remove unrelated style 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.

Thank you for your update, @georgepittock. One minor nitpick. The rest LGTM.

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) October 17, 2024 15:51
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 17, 2024
@georgepittock
Copy link
Contributor Author

georgepittock commented Oct 17, 2024

CI failure looks unrelated, merging main into branch so trigger CI again...

@serhiy-storchaka serhiy-storchaka merged commit 04d6dd2 into python:main Oct 17, 2024
34 checks passed
@miss-islington-app
Copy link

Thanks @georgepittock for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 17, 2024
…dowed builtins (pythonGH-113577)

(cherry picked from commit 04d6dd2)

Co-authored-by: George Pittock <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 17, 2024
…dowed builtins (pythonGH-113577)

(cherry picked from commit 04d6dd2)

Co-authored-by: George Pittock <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 17, 2024

GH-125654 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 17, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 17, 2024

GH-125655 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 17, 2024
@georgepittock georgepittock deleted the fix-issue-gh-113570 branch October 17, 2024 16:38
serhiy-storchaka pushed a commit that referenced this pull request Oct 17, 2024
…adowed builtins (GH-113577) (GH-125655)

(cherry picked from commit 04d6dd2)

Co-authored-by: George Pittock <[email protected]>
serhiy-storchaka pushed a commit that referenced this pull request Oct 17, 2024
…adowed builtins (GH-113577) (GH-125654)

(cherry picked from commit 04d6dd2)

Co-authored-by: George Pittock <[email protected]>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants