Skip to content

Replace inspect usage with common CPython stdlib pattern #327

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 70 additions & 16 deletions importlib_resources/_common.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import contextlib
import functools
import importlib
import inspect
import itertools
import os
import pathlib
import sys
import tempfile
import types
import warnings
Expand Down Expand Up @@ -84,25 +83,80 @@ def _(cand: str) -> types.ModuleType:

@resolve.register
def _(cand: None) -> types.ModuleType:
return resolve(_infer_caller().f_globals['__name__'])
# PYUPDATE: 3.15 - Update depth & explanation for package_to_anchor()'s removal.
# Expected parent stack frames for depth=4:
# 0) resolve()'s singledispatch dispatch
# 1) resolve()'s singledispatch wrapper
# 2) files()
# 3) package_to_anchor()
# 4) <caller>()
Comment on lines +86 to +92
Copy link
Member

Choose a reason for hiding this comment

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

I don't love this comment. It's nice to know, but it distracts from the function's purpose and represents tech debt. Usually, I like to abstract tech debt into separate modules that provide the compatible and future uses. If we end up sticking with the static depth for detecting the caller, I'd like to see the depth itself encapsulated in a _caller_depth() function, whose docstring explains why the number 4 is there, when it might need to change, and maybe a static copy of the current call stack (although I think I'd rather omit that instead ensure there are tests capturing the expectation).
Are there tests that fail if the depth doesn't match the implementation?

return resolve(_get_caller_module_name(depth=4))
Copy link
Member

Choose a reason for hiding this comment

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

I'm quite uneasy hard-coding literals (like depth=4). I understand it represents the expected call stack, but now it needs to be kept in sync with the implementation, entangling those concerns. The current implementation avoided entangling those concerns by tracking a more abstract concept (the first caller in the stack that's not this module). Is there a reason not to instead simply implement a replacement for inspect.stack() that avoids inspect?



def _infer_caller():
"""
Walk the stack and find the frame of the first caller not in this module.
"""
# An expanded version of a CPython stdlib pattern to avoid using the expensive inspect.
def _get_caller_module_name(depth: int = 1, default: str = "__main__") -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the repetitive nature of this function. For example, the literal "__main__" is used multiple times, and the default depth=1 is mentioned three times. And depth+1 has to be implemented twice. I'd like to see it refactored so that these common concerns are implemented in exactly one place.

"""Find the module name of the frame one level beyond the depth given."""

try:
return sys._getframemodulename(depth + 1) or default # type: ignore[attr-defined] # Guarded.
except AttributeError: # For platforms without sys._getframemodulename.
global _get_caller_module_name

def _get_caller_module_name(depth: int = 1, default: str = "__main__") -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I don't care for how __main__ gets snuck in here. It wasn't required before. Why have a fallback now (instead of retaining parity)?

"""Find the module name of the frame one level beyond the depth given."""

try:
return _get_frame(depth + 1).f_globals.get("__name__", default) # type: ignore[union-attr] # Guarded.
except (AttributeError, ValueError): # For platforms without frames.
global _get_caller_module_name

def _get_caller_module_name(
depth: int = 1,
default: str = "__main__",
) -> str:
"""Find the module name of the frame one level beyond the depth given."""

msg = "Cannot get the caller's module's name."
raise RuntimeError(msg)

return _get_caller_module_name(depth, default)

return _get_caller_module_name(depth, default)


# This attempts to support Python implementations that either don't have sys._getframe
# (e.g. Jython) or don't support sys._getframe(x) where x >= 1 (e.g. IronPython).
# We avoid inspect.stack because of how expensive inspect is to import.
Comment on lines +127 to +129
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's worth supporting Jython or IronPython. Maybe best would be to simply rely on inspect in those cases.

def _get_frame(depth: int = 1, /) -> Optional[types.FrameType]:
"""Return the frame object for one of the caller's parent stack frames."""

try:
return sys._getframe(depth + 1)
except (AttributeError, ValueError): # For platforms without full sys._getframe.
global _get_frame

def _get_frame(depth: int = 1, /) -> Optional[types.FrameType]:
"""Return the frame object for one of the caller's parent stack frames."""

try:
raise TypeError
except TypeError:
Comment on lines +141 to +143
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to understand the purpose of this code. It's there to cause the interpreter to generate a stack trace. There should probably be a comment to this effect.

try:
frame = sys.exc_info()[2].tb_frame # type: ignore[union-attr] # Guarded.
for _ in range(depth + 1):
frame = frame.f_back
Copy link
Member

Choose a reason for hiding this comment

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

I don't love that the current implementation goes from 2 levels of indentation to 6. I'll take it on myself to refactor the implementation to be more of a composition of smaller functions.

return frame
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the bare Exception here. What is its purpose?

global _get_frame

def _get_frame(depth: int = 1, /) -> Optional[types.FrameType]:
"""Return the frame object for one of the caller's parent stack frames."""

def is_this_file(frame_info):
return frame_info.filename == stack[0].filename
return None

def is_wrapper(frame_info):
return frame_info.function == 'wrapper'
return _get_frame()

stack = inspect.stack()
not_this_file = itertools.filterfalse(is_this_file, stack)
# also exclude 'wrapper' due to singledispatch in the call stack
callers = itertools.filterfalse(is_wrapper, not_this_file)
return next(callers).frame
return _get_frame()
Copy link
Member

Choose a reason for hiding this comment

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

While the previous implementation was just a few lines, this new implementation has a lot of hacks and internal details. We probably should move it all to its own _inspect module.



def from_package(package: types.ModuleType):
Expand Down