Skip to content

bpo-38026: fix inspect.getattr_static#15676

Merged
methane merged 2 commits into
python:masterfrom
methane:fix/bpo-38026
Sep 5, 2019
Merged

bpo-38026: fix inspect.getattr_static#15676
methane merged 2 commits into
python:masterfrom
methane:fix/bpo-38026

Conversation

@methane
Copy link
Copy Markdown
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
Comment thread Lib/inspect.py
dict_attr = _shadowed_dict(klass)
if (dict_attr is _sentinel or
isinstance(dict_attr, types.MemberDescriptorType)):
type(dict_attr) is types.MemberDescriptorType):
Copy link
Copy Markdown
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
Copy Markdown
Member Author

@methane methane Sep 4, 2019

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
Copy Markdown
Contributor

@srinivasreddy srinivasreddy Sep 4, 2019

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
Copy Markdown
Member Author

@methane methane Sep 4, 2019

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
Copy Markdown
Member

serhiy-storchaka commented Sep 4, 2019

Would be nice to add tests.

@methane
Copy link
Copy Markdown
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
Copy Markdown
Contributor

miss-islington commented Sep 5, 2019

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

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Sep 5, 2019

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

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

+1 more reviewer

@srinivasreddy srinivasreddy srinivasreddy left review comments

Reviewers whose approvals may not affect merge requirements

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