Skip to content

gh-79282: Fix inspect.findsource breaking on class frame objects#10209

Open
orlnub123 wants to merge 7 commits intopython:mainfrom
orlnub123:inspect.findsource
Open

gh-79282: Fix inspect.findsource breaking on class frame objects#10209
orlnub123 wants to merge 7 commits intopython:mainfrom
orlnub123:inspect.findsource

Conversation

@orlnub123
Copy link
Copy Markdown
Contributor

@orlnub123 orlnub123 commented Oct 29, 2018

Comment thread Lib/test/inspect_fodder.py Outdated
# line 99
def extra_c():
pass
class C:
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.

class A and class C look very similar to me with a function above them. Any difference between the tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right. When I added extra_a it was to catch any problems with A and B. Reversing the order of them should fix it.

Copy link
Copy Markdown
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification. I feel one more case with a class definition inside a function can be added but I don't know if it's repetitive and can wait for a core dev on the test. I checked out the PR locally and LGTM from me.

Attaching my test as a reference.

diff --git a/Lib/test/inspect_fodder.py b/Lib/test/inspect_fodder.py
index a55e40bf0d..60f46c53e7 100644
--- a/Lib/test/inspect_fodder.py
+++ b/Lib/test/inspect_fodder.py
@@ -101,3 +101,13 @@ class A:
     def func(self):
         pass
     fr = inspect.currentframe()
+
+# line 105
+def extra_d():
+
+    class D:
+        def func(self):
+            pass
+        fr = inspect.currentframe()
+
+    return D()
diff --git a/Lib/test/test_inspect.py b/Lib/test/test_inspect.py
index 90eebec611..9d1c764535 100644
--- a/Lib/test/test_inspect.py
+++ b/Lib/test/test_inspect.py
@@ -432,6 +432,7 @@ class TestRetrievingSourceCode(GetSourceBase):
         functions = inspect.getmembers(mod, inspect.isfunction)
         self.assertEqual(functions, [('eggs', mod.eggs),
                                      ('extra_c', mod.extra_c),
+                                     ('extra_d', mod.extra_d),
                                      ('lobbest', mod.lobbest),
                                      ('spam', mod.spam)])

@@ -583,6 +584,7 @@ class TestGettingSourceOfFrames(GetSourceBase):
         self.assertNotSourceEqual(mod.B.fr, 85, 86)
         self.assertSourceEqual(mod.C.fr, 87, 90)
         self.assertNotSourceEqual(mod.C.fr, 85, 86)
+        self.assertSourceEqual(mod.extra_d().fr, 108, 111)

 class TestDecorators(GetSourceBase):
     fodderModule = mod2

@@ -0,0 +1,2 @@
Fix inspect.findsource incorrectly returning the line number of the first
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.

Just noticed that inspect.findsource is not documented and the tests also use inspect.getsource though it uses findsource internally. Can this be changed to inspect.getsource? Also please use the below with markdown so that the text is linked to the function docs.

Fix :func:`inspect.getsource` incorrectly returning the line number of the first
function above any given class frame objects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That might be a bit unfair since more functions than just getsource use it. getcomments and getsourcelines use it directly while getsource uses getsourcelines, using it indirectly. Would it be OK to mention all of them?

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.

The last two commits ef1e750 and e4e811d used getsource in the NEWS though findsource was fixed since I think findsource is not documented. I think we can wait for the reviewer on this 👍

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.

Just for reference there is an open issue to document functions like findsource : https://bugs.python.org/issue17972

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I'm sorry this patch lingered for so long! I think it's close to ready but have a few comments.

Comment thread Lib/test/test_inspect.py

def test_class_frame(self):
self.assertSourceEqual(mod.A.fr, 100, 103)
self.assertNotSourceEqual(mod.A.fr, 85, 86)
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.

What is the motivation for the assertNotSourceEqual tests? If it's equal to a range, surely you don't have to also test that it's unequal to another range?

Comment on lines +1 to +2
Fix inspect.findsource incorrectly returning the line number of the first
function above any given class frame objects.
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.

inspect.findsource remains undocumented now. I'd suggest simply saying that the patch fixes finding class object frames in the inspect module.

Suggested change
Fix inspect.findsource incorrectly returning the line number of the first
function above any given class frame objects.
Fix bug where the :mod:`inspect` module failed to identify the
correct source code for class object frames.

@erlend-aasland erlend-aasland changed the title bpo-35101: Fix inspect.findsource breaking on class frame objects gh-79282: Fix inspect.findsource breaking on class frame objects Jan 8, 2024
@github-actions
Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core review stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants