Skip to content

bpo-34871: inspect: Don't pollute sys.modules#9696

Merged
miss-islington merged 3 commits into
python:masterfrom
methane:fix-inspect
Oct 4, 2018
Merged

bpo-34871: inspect: Don't pollute sys.modules#9696
miss-islington merged 3 commits into
python:masterfrom
methane:fix-inspect

Conversation

@methane

@methane methane commented Oct 4, 2018

Copy link
Copy Markdown
Member

@methane methane added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Oct 4, 2018
@methane methane requested a review from 1st1 October 4, 2018 10:37

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like the idea to copy sys.modules. It's an expensive operation, and this code is executed to evaluate every parameter with a non-trivial default for a function with __text_signature__.

Looking at the documentation of Argument Clinic, I see that this code is supposed to handle simple defaults like mod.attr.

So why don't we rewrite wrap_value to do just that, e.g.:

    def wrap_value(s):
        try:
            value = eval(s, module_dict)
        except NameError:
            match = re.match(r'\s*(?P<mod>\w+)\.(?P<attr>\w+)\s*', s)
            if not match:
                raise RuntimeError

            mod = sys.modules.get(match.group('mod'))
            if mod is None:
                raise RuntimeError
            try:
                value = getattr(mod, match.group('attr'))
            except AttributeError:
                raise RuntimeError

This code is equivalent to the previous code and doesn't use eval() at all for evaluating non-trivial defaults.

@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@methane

methane commented Oct 4, 2018

Copy link
Copy Markdown
Member Author

I don't like the idea to copy sys.modules. It's an expensive operation, and this code is executed to evaluate every parameter with a non-trivial default for a function with __text_signature__.

sys.modules is a plain dict. Copying a dict will be faster than regular expression.

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sys.modules is a plain dict. Copying a dict will be faster than regular expression.

Depends on how many modules you have in your application. But your solution to copy it once works for me.

@miss-islington

Copy link
Copy Markdown
Contributor

@methane: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 6f85b82 into python:master Oct 4, 2018
@miss-islington

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 4, 2018
https://bugs.python.org/issue34871
(cherry picked from commit 6f85b82)

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
@bedevere-bot

Copy link
Copy Markdown

GH-9701 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 4, 2018
https://bugs.python.org/issue34871
(cherry picked from commit 6f85b82)

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
@bedevere-bot

Copy link
Copy Markdown

GH-9702 is a backport of this pull request to the 3.6 branch.

1st1 pushed a commit that referenced this pull request Oct 4, 2018
https://bugs.python.org/issue34871
(cherry picked from commit 6f85b82)

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
1st1 pushed a commit that referenced this pull request Oct 4, 2018
https://bugs.python.org/issue34871
(cherry picked from commit 6f85b82)

Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
yan12125 pushed a commit to archlinuxcn/repo that referenced this pull request Oct 5, 2018
@methane methane deleted the fix-inspect branch November 29, 2018 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants