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.

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

bpo-38026: fix inspect.getattr_static #15676

Merged
merged 2 commits into from
Sep 5, 2019
Merged

bpo-38026: fix inspect.getattr_static #15676

merged 2 commits into from
Sep 5, 2019

Conversation

methane
Copy link
Member

@methane methane commented Sep 4, 2019 •
edited by bedevere-bot
Loading

It should avoid dynamic lookup including isinstance.

This is a regression caused by #5351.

https://bugs.python.org/issue38026

@methane
bpo-38026: fix inspect.getattr_static
1dc1cd0 
It should avoid dynamic lookup including `isinstance`.

This is regression caused by pythonGH-5351.
@methane
add news entry
2eeddb9
@methane methane added type-bug An unexpected behavior, bug, or error needs backport to 3.8 labels Sep 4, 2019
srinivasreddy
srinivasreddy reviewed Sep 4, 2019
View reviewed changes
Lib/inspect.py
@@ -1580,7 +1580,7 @@ def getattr_static(obj, attr, default=_sentinel):
klass = type(obj)
dict_attr = _shadowed_dict(klass)
if (dict_attr is _sentinel or
isinstance(dict_attr, types.MemberDescriptorType)):
type(dict_attr) is types.MemberDescriptorType):
Copy link
Contributor

@srinivasreddy srinivasreddy Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think It helps to comment on why we use type here.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is required, every line in this function should have same comment. It will be too noisy...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a link to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I merged because I didn't read the comment / docstring so I didn't notice dynamic lookup should be avoided. It's totally my mistake.

But note that It is not because I didn't notice isinstance cause dynamic lookup.
If we want to avoid this type of regression by adding comment, we need to add
# to avoid dynamic lookup comment on every line. It's nonsense...

@serhiy-storchaka
Copy link
Member

Would be nice to add tests.

@methane
Copy link
Member Author

methane commented Sep 4, 2019

Would be nice to add tests.

Of course, but I am not metaprogramming expert so I don't know how to override __dict__ and hook lookup on it yet.
I want to just revert what I merged.

@methane methane merged commit 8f9cc87 into python:master Sep 5, 2019
@methane methane deleted the fix/bpo-38026 branch September 5, 2019 04:07
@miss-islington
Copy link
Contributor

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 5, 2019
@methane @miss-islington
bpo-38026: fix inspect.getattr_static (pythonGH-15676)
78b7750 
It should avoid dynamic lookup including `isinstance`.

This is a regression caused by pythonGH-5351.
(cherry picked from commit 8f9cc87)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
miss-islington added a commit that referenced this pull request Sep 5, 2019
@miss-islington @methane
bpo-38026: fix inspect.getattr_static (GH-15676)
bdcbb83 
It should avoid dynamic lookup including `isinstance`.

This is a regression caused by GH-5351.
(cherry picked from commit 8f9cc87)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@methane @lisroach
bpo-38026: fix inspect.getattr_static (pythonGH-15676)
f15d8ff 
It should avoid dynamic lookup including `isinstance`.

This is a regression caused by pythonGH-5351.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@methane @DinoV
bpo-38026: fix inspect.getattr_static (pythonGH-15676)
b430725 
It should avoid dynamic lookup including `isinstance`.

This is a regression caused by pythonGH-5351.
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@methane @websurfer5
bpo-38026: fix inspect.getattr_static (pythonGH-15676)
d5d1946 
It should avoid dynamic lookup including `isinstance`.

This is a regression caused by pythonGH-5351.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srinivasreddy srinivasreddy srinivasreddy left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@methane @serhiy-storchaka @miss-islington @bedevere-bot @srinivasreddy @the-knights-who-say-ni