More improvements to stubgen #7951

JukkaL · 2019-11-14T17:52:51Z

This includes many improvements to stubgen and a related mypy fix.
Here are the most useful ones:

Use a separate process to do introspection of modules so that we can
recover if a module kills the current process on import, for example.
Export all names imported from the current package by default.
Add --export-less stubgen flag to disable this behavior.
Avoid a crash in semantic analysis if there's a bad property definition
(stubgen can generate these).
Fix various issues with bad Python code being generated by stubgen.
Ignore bad signatures in docstrings (this is still very ad-hoc, but it's a
bit more robust now).
Try to find a module using sys.path if we can't import it.
Skip some additional modules that may be runnable since they can cause
trouble when we try to introspect them.

This is again a big PR, but the commit history should be reasonably clean.

We can't always tell whether they are supposed to be part of a public API, so just export all of them. Add the `--export-less` flag to get the old behavior.

This is best effort anyway, and we can't expect docstring to follow any strict conventions.

…alue This also fixes an issue with generated attrs methods.

ilevkivskyi

Thanks, many nice fixes here. I have several comments/suggestions.

ilevkivskyi · 2019-11-15T12:29:52Z

mypy/moduleinspect.py

+    path = getattr(package, '__path__', None)
+    pkg_all = getattr(package, '__all__', None)
+    if pkg_all is not None:
+        pkg_all = list(pkg_all)


Can this cause troubles if __all__ is not valid?

ilevkivskyi · 2019-11-15T12:32:18Z

mypy/moduleinspect.py

+                            subpackages=subpackages)
+
+
+def worker(queue1: 'Queue[str]', queue2: 'Queue[Union[str, ModuleProperties]]') -> None:


This is very minor, but maybe call them task_queue and result_queue?

ilevkivskyi · 2019-11-15T12:35:54Z

mypy/moduleinspect.py

+
+    def _start(self) -> None:
+        self.q1 = Queue()  # type: Queue[str]
+        self.q2 = Queue()  # type: Queue[Union[ModuleProperties, str]]


Same as above, maybe call theses self.tasks and self.results? Also maybe add a short comment that str contains exception message?

ilevkivskyi · 2019-11-15T13:22:59Z

mypy/moduleinspect.py

+            if n == max_iter:
+                raise RuntimeError('Timeout waiting for subprocess')
+            try:
+                return self.q2.get(timeout=0.05)


Maybe make the timeout configurable in constructor?

I don't think that making it configurable is useful.

ilevkivskyi · 2019-11-15T13:27:22Z

mypy/semanal.py

+            else:
+                self.fail('Unexpected definition for property "{}"'.format(first_item.func.name),
+                          item)
+                deleted_items.append(i + 1)


What if the extra item is also decorated (with some unrelated decorator)? Maybe add a test for this?

ilevkivskyi · 2019-11-15T13:30:21Z

mypy/stubgen.py

+    '__gt__',
+    '__ge__',
+    '__hash__',
+    '__iter__',
 }


Not really important but I think all these top-level constants can be made Final.

ilevkivskyi · 2019-11-15T13:32:23Z

mypy/stubgen.py

-                exported_names.update(sub_names)
-                if module:
-                    for name in sub_names:
-                        self.import_tracker.require_name(name)

    def translate_module_name(self, module: str, relative: int) -> Tuple[str, int]:


This technically doesn't need to be a method, but up to you.

ilevkivskyi · 2019-11-15T13:33:16Z

mypy/stubgen.py

@@ -1041,6 +1105,21 @@ def is_recorded_name(self, name: str) -> bool:
        return self.is_top_level() and name in self._toplevel_names


+def find_method_names(defs: List[Statement]) -> Set[str]:
+    # TODO: Traverse into nested definitions
+    # TODO: Overloads


There is an elif for OverloadedFuncDef is this TODO outdated?

ilevkivskyi · 2019-11-15T13:35:18Z

mypy/stubgen.py

@@ -1385,6 +1481,9 @@ def parse_options(args: List[str]) -> Options:
    parser.add_argument('--include-private', action='store_true',
                        help="generate stubs for objects and members considered private "
                             "(single leading underscore and no trailing underscores)")
+    parser.add_argument('--export-less', action='store_true',
+                        help=("don't implicitly export all names imported from other modules "
+                              "in the same package"))


Maybe update the docs with the new flag? (In a separate PR if you prefer.)

A good idea. I'll create a separate PR.

ilevkivskyi · 2019-11-15T13:40:31Z

test-data/unit/stubgen.test

+__all__ = ['C']
+
+@asyncio.coroutine
+def f():


Why don't we include f() in the output?

It's not included in __all__.

(This is something we may want to change, but it may require the generation of an explicit __all__.)

ilevkivskyi · 2019-11-15T13:45:29Z

(I think you noticed, but tests are still failing.)

It doesn't get inherited on Windows.

JukkaL added 23 commits November 14, 2019 17:39

Continue operation even if imported module raises SystemExit

037ce58

Use separate process for module introspection

86e9ad4

Recover from helper process death

9923a7a

Display message when we start processing ASTs

0722c7d

Automatically skip 'setup' modules and other non-library modules

db0a8d5

Add additional vendored package naming convention

ca1923a

Fallback to finding module using sys.path if import fails

7e10f4c

Import all names imported from the current package by default

8e1b5b8

We can't always tell whether they are supposed to be part of a public API, so just export all of them. Add the `--export-less` flag to get the old behavior.

Silently ignore invalid docstring signatures

0728e70

This is best effort anyway, and we can't expect docstring to follow any strict conventions.

Also use Python 2 sys.path to find files

67e2db4

Do not generate invalid argument names from docstrings

2cb948c

Be more aggressive about ignoring various dunders

514ae1d

Avoid generating both an attribute and a property definition

b818592

Avoid None return type for some methods that should always return a v…

6237ec4

…alue This also fixes an issue with generated attrs methods.

Fix indent of named tuples

b4c8f21

Fix some imports from vendorex six.moves

c0dda3b

Fall back to Any for a dynamic named tuple definition

0d6e545

Don't crash on invalid property definition

97f2bf4

Clear decorators if we skip a function

2affb99

Fix @classmethod without corresponding 'def' in C module

29acbed

Catch more bad types and bad arguments names in C stubs

75a094b

Fix stripping of C type names

fb95598

Misc tweaks

17440b4

JukkaL requested a review from ilevkivskyi November 14, 2019 17:52

JukkaL added 6 commits November 14, 2019 18:37

Fixes

7968a6a

Fix lint

73fb604

'None' is a valid type

2cc05d5

Update test case

7e3826c

Fix Python 3.5

e8a1498

Another Python 3.5 fix

9634aa9

Raise exception if subprocess gets stuck

18591d3

ilevkivskyi approved these changes Nov 15, 2019

View reviewed changes

JukkaL added 8 commits November 15, 2019 13:53

Attempt to fix compiled

99c6dad

Attempt to fix compiled

78771f4

Attempt to fix compiled

1a098c6

Fix unused import

d35ec7b

Fix test case on Windows

d892a7a

Pass sys.path to the worker function

4724d00

It doesn't get inherited on Windows.

Respond to feedback

1bc5af7

Skip test case on Windows

fd088b1

JukkaL merged commit bbb192d into master Nov 15, 2019

gvanrossum deleted the stubgen-fixes-final branch August 26, 2020 21:25

		subpackages=subpackages)


		def worker(queue1: 'Queue[str]', queue2: 'Queue[Union[str, ModuleProperties]]') -> None:

Uh oh!

More improvements to stubgen #7951

More improvements to stubgen #7951

Uh oh!

Conversation

JukkaL commented Nov 14, 2019

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Nov 15, 2019

Uh oh!

Uh oh!