Next step. The classes can be nested not only in other classes, but in functions, which can be nested in classes or functions, etc.

1. Remove theisinstance(child, ast.ClassDef) check in the loop.
2. Add visitors for FuncDef and AsyncFuncDef. They should just push/pop the name and'<locals>' on the stack and iterate child nodes.
3. Add corresponding tests. Something like:

deffunc123():classclass234:passreturnclass234classclass345:deffunc456(self):classclass567:passreturnclass567

etc (and similar for async functions).

Thanks, I will work on them. It seems there is no test for similar to your examples since my PR raises OSError for your examples meanwhile master works. I guess my code breaks these cases which was handled by the previous regex based approach.

deffunc123():classclass234:deffunc(self):passreturnclass234classclass345:deffunc456(self):classclass567:passreturnclass567if__name__=="__main__":importinspectprint(inspect.getsource(func123()))print(inspect.getsource(class345().func456()))

$ python3 foo_1.py    class class234:        def func(self):            pass        class class567:            pass$ ../cpython/python.exe foo_1.pyTraceback (most recent call last):  File "foo_1.py", line 15, in <module>    print(inspect.getsource(func123()))  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 993, in getsource    lines, lnum = getsourcelines(object)  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 975, in getsourcelines    lines, lnum = findsource(object)  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 832, in findsource    raise OSError('could not find class definition')OSError: could not find class definition

I will fix the rest of the PR comments too in next commit.

One side effect of the change is thatinspect.getcomments also works correctly. It caused a failure in test_pydoc athttps://github.com/python/cpython/blob/master/Lib/test/test_pydoc.py#L479 .

pydoc'srender_doc adds some more content usinginspect.getdoc or comments of a class usinginspect.getcomments if either of them is present at

importpydocdeffunc123():# 124classclass234:passreturnclass234classclass234:passif__name__=="__main__":importinspectprint(inspect.getcomments(func123()))print(pydoc.render_doc(func123()))

➜  backups python3 foo_3.pyNonePython Library Documentation: class class234 in module __main__class class234(builtins.object) |  Data descriptors defined here: | |  __dict__ |      dictionary for instance variables (if defined) | |  __weakref__ |      list of weak references to the object (if defined)

➜  backups ../cpython/python.exe foo_3.py# 124Python Library Documentation: class class234 in module __main__class class234(builtins.object) |  # 124 | |  Data descriptors defined here: | |  __dict__ |      dictionary for instance variables (if defined) | |  __weakref__ |      list of weak references to the object (if defined)

Seems there is an altered environment failure with my test I will look into it. I think I am running the asyncdef version of the test correctly athttps://github.com/python/cpython/pull/10307/files#diff-0258c7716015b67c231e0d2a60ebbd31R711. Please add in your thoughts.

Thanks!

Looking at the failure and other tests I think I need to callasyncio.set_event_loop_policy(None) in the end as a teardown action.

Current test

./python.exe -m test --fail-env-changed test_inspectRun tests sequentially0:00:00 load avg: 2.47 [1/1] test_inspectWarning -- asyncio.events._event_loop_policy was modified by test_inspect  Before: None  After:  <asyncio.unix_events._UnixDefaultEventLoopPolicy object at 0x1109e7050>test_inspect failed (env changed)== Tests result: ENV CHANGED ==1 test altered the execution environment:    test_inspect

set event loop policy in the end

deftest_nested_class_definition_inside_async_function(self):importasyncioself.assertSourceEqual(asyncio.run(mod2.func226()),227,229)self.assertSourceEqual(mod2.cls227,233,237)self.assertSourceEqual(asyncio.run(mod2.cls227().func234()),235,236)asyncio.set_event_loop_policy(None)

./python.exe -m test --fail-env-changed test_inspectRun tests sequentially0:00:00 load avg: 2.33 [1/1] test_inspect== Tests result: SUCCESS ==1 test OK.Total duration: 2 sec 386 msTests result: SUCCESS

@serhiy-storchaka The AST module takes some noticeable hit for very large files like machine generated python code with a lot of classes. The only optimization I tried was to usedeque instead oflist forstack but for this benchmark there was no noticeable effect which I assume is that deque is helpful for highly nested code whereappend andpop are performant and not needed for small amount ofappend andpop.

I just want to add it here as a known tradeoff and maybe I am benchmarking on a highly pathological case with 60k classes with 160k lines. The largest I came across in the wild was generating Python code classes from XML schema usinggenerateDS that generated 35k line file with around 4k classes. I prefer correctness over wrong results given the trade off but if there are any optimizations I am missing then I can add them and@1st1 might also have thoughts on the approach.

For general files like usinginspect.getsource on it's own module that is 3000 lines there was no significant impact.

# with patch$ ./python.exe -m timeit -s 'import inspect' 'inspect.getsource(inspect.getsource)'1000 loops, best of 5: 276 usec per loop# on 3.7$  python3 -m timeit -s 'import inspect' 'inspect.getsource(inspect.getsource)'1000 loops, best of 5: 279 usec per loop

I generated a large file with around 20000 classes with each 3 layer of nesting. On an optimized build the timings were as below :

withopen('foo_perf.py','a')asf:f.write('import inspect')foriinrange(20000):r='''class cls{0}:    class cls{0}:        class cls{0}:            def func{0}(self):                pass'''.format(i)f.write(r)f.write('print(inspect.getsource(cls1999.cls1999))')

On master (Incorrect result but performant)

time ./python.exe -m timeit -s 'import foo_perf' 'import inspect' 'inspect.getsource(foo_perf.cls999.cls999)'class cls1999:    class cls1999:        class cls1999:            def func1999(self):                pass50 loops, best of 5: 6.89 msec per loop./python.exe -m timeit -s 'import foo_perf' 'import inspect'   3.82s user 0.17s system 98% cpu 4.037 total

With patch (Correct result but very slow)

time ./python.exe -m timeit -s 'import foo_perf' 'import inspect' 'inspect.getsource(foo_perf.cls999.cls999)'    class cls1999:        class cls1999:            def func1999(self):                pass1 loop, best of 5: 914 msec per loop./python.exe -m timeit -s 'import foo_perf' 'import inspect'   7.94s user 0.48s system 94% cpu 8.864 total

I don't think a deque has advantages over a list here. We push and pop from one end.

Stopping when the first candidate is found will make findingcls1999 in your example 10x faster. But it will not help in case ofcls19999.

Yes, parsing to AST is much slower than using a regex, and uses much more memory, but I don't think this is very large issue. In common cases the performance should be good enough.

@1st1 It would be great if you can review this

Looks good to me in general. Here're a few questions:

1. For the example from bpo:

   ifsys.platform=='win32':classArena(object):        ...else:classArena(object):        ...

   Which class will be returned? Serhiy mentioned that the current implementation returns the first one. Did you fix this implementation to do the same?

2. This PR also adds returning class decorators as part of the source code instead of just returning the class definition.

   This might be a backwards compatibility issue. I'd prefer the behaviour to stay absolutely the same. We can add another function, or can consider adding a keyword-only flag togetsource to make it return decorators.

Thanks@1st1 for the review.

Which class will be returned? Serhiy mentioned that the current implementation returns the first one. Did you fix this implementation to do the same?

I have modified the PR to behave like the current implementation to return the first class definition inside conditional clauses. Added a test athttps://github.com/python/cpython/pull/10307/files#diff-0258c7716015b67c231e0d2a60ebbd31R697

This might be a backwards compatibility issue. I'd prefer the behaviour to stay absolutely the same. We can add another function, or can consider adding a keyword-only flag to getsource to make it return decorators.

I guess@serhiy-storchaka brought this up since functions return decorators as part ofinspect.getsource. I have modified the PR and NEWS entry to make sure decorators are not returned like the original implementation. I will open a separate enhancement request on bpo for this.

I guess@serhiy-storchaka brought this up since functions return decorators as part of inspect.getsource. I have modified the PR and NEWS entry to make sure decorators are not returned like the original implementation. I will open a separate enhancement request on bpo for this.

It's an interesting discrepancy. For the sake of making the API consistent we might consider breaking it slightly for classes and start returning decorators too in 3.8. I honestly don't think it's going to be a huge deal. I'm curious what@serhiy-storchaka thinks about that though.

I think it is good to start returning decorators too.

Agreed with@serhiy-storchaka that this is a useful enhancement and makes the API consistent with functions. Since the decorator related code is reverted@1st1 please let me know on your decision so that I can add it back if needed.

Thanks

Adding decorators to classes too was an enhancement proposed byhttps://bugs.python.org/issue15856 . I have currently reverted the decorator related code but I think it would be good to add it back to return decorators for consistency.

@1st1 It will be helpful if you can review the changes. I have changed the code to return decorators too to resolvehttps://bugs.python.org/issue15856 and also it makes it consistent with source code for functions. Not sure if this can be merged after beta release given the decorator enhancement but I would be very happy if this makes it part of 3.8 beta 1.

Thanks

@serhiy-storchaka I have rebased the branch with latest master since it was old. It will be helpful to have your review and re-approval so that I can merge this.

@tirkarthi: Please replace# withGH- in the commit message next time. Thanks!

Thanks Serhiy, I have merged the PR. Unfortunately, I had an issue with GitHub UI that my squashed message in the textbox didn't pass through and it just added the intermediate commit messages even after edit :(

FWIW the performance regression due to this PR madeinspect.getsource unusable for us in an existing use case in a large code base. Just noting it here rather than filing an issue since it seems like this was already considered here and it was decided that performance ofinspect.getsource is not important. And I'm not sure it's really fixable anyway without reintroducing the bugs fixed by this PR, and it's too late for that. Just making a note for historical value to clarify that this scale of performance regression will likely bite someone.