-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-126019 Fix inspect.getsource for classes created in PyREPL #126032
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
base: main
Are you sure you want to change the base?
Conversation
Lib/inspect.py
Outdated
| # Protect against fetching the wrong source in PyREPL | ||
| if object.__module__ == '__main__': | ||
| raise OSError('source code not available') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm worried about this being a breaking change. __module__ now has more precedent over __file__--maybe people could have been relying on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I share similar concerns. Also why this only affects pyREPL and not any other main module ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why this only affects pyREPL and not any other main module ?
I think because for other __main__ modules, __main__.__file__ points to the correct source code.
In basic REPL, sys.modules["__main__"] doesn't have a __file__ attribute, so it raises OSError('source code not available') when inspected.
But with PyREPL, sys.modules["__main__"].__file__ is _pyrepl.__main__, which is where the source code is being pulled from. So we could instead skip loading from __file__ if we detect PyREPL is in use, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, we could just delete __file__ from __main__, right?
diff --git a/Lib/_pyrepl/main.py b/Lib/_pyrepl/main.py
index a6f824dcc4a..17c55eb9c9f 100644
--- a/Lib/_pyrepl/main.py
+++ b/Lib/_pyrepl/main.py
@@ -35,6 +35,7 @@ def interactive_console(mainmodule=None, quiet=False, pythonstartup=False):
import __main__
namespace = __main__.__dict__
namespace.pop("__pyrepl_interactive_console", None)
+ namespace.pop("__file__", None)
# sys._baserepl() above does this internally, we do it here
startup_path = os.getenv("PYTHONSTARTUP")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That fixes the problem no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that would be much less invasive.
|
I've changed the approach to removing Some changes had to be made to tests that expected Wording of the NEWS entry seems poor to me, would gladly apply any suggested improvements. |
| def test_inspect_getsource(self): | ||
| code = "import inspect\nclass A: pass\n\nprint(inspect.getsource(A))\nexit()\n" | ||
| output, exit_code = self.run_repl(code) | ||
| self.assertEqual(exit_code, 0) | ||
| self.assertIn("OSError('source code not available')", output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that testing whether inspect.getsource works on other objects (functions, methods, traceback, frames) is worthwhile, but I'm not sure if it should be done in this PR. So, I'd be fine with doing it in a follow-up
inspect.getsourcewould fetch sourcecode from_pyrepl.__main__for classes defined in the REPL when using PyREPL becausesys.modules["__main__"]points to that module.This PR makes
inspect.getsourceraiseOSError('source code not available')for classes defined in the REPL by detecting that.__module__is"__main__". Tests pass with this change, but if you know of a corner case that would be broken by it please say so.