Use __getattr__ to mark partial stub packages by ilevkivskyi · Pull Request #5231 · python/mypy

ilevkivskyi · 2018-06-18T13:14:20Z

There is a problem with annotating large frameworks -- they are large. Therefore it is hard to produce good stubs in a single pass. A possible better workflow would be to allow indicating that a given (sub-)package is incomplete.

I propose to use __getattr__ for the role of such indicator. A motivation is that currently adding a __getattr__ to package/__init__.pyi already makes from package import mod work, but import package.mod still fails (plus simplicity of implementation). Here are the rules that I propose:

One can declare a (sub-)package as incomplete by adding a __getattr__ to its __init__.pyi
If the return type of this function is types.ModuleType or Any, we assume that all imports from this (sub-)package succeed.
Incomplete package can contain a complete subpackage:

# file a/__init__.pyi
from types import ModuleType
def __getattr__(attr: str) -> ModuleType: ...

# file a/b/__init__.pyi
# empty (i.e. complete package)

# file main.py
import a.d  # OK
import a.b.c  # Error module not found

Note: these rules apply only to stubs (i.e. .pyi files). I add several tests to illustrate this behaviour.
This PR shouldn't give any significant performance penalty because the added parsing/loading only happens when an error would be reported (for our internal workflow the penalty will be zero because of the flags we use).

This PR will allow gradually adding stub modules to a large framework package, without generating loads of false positives for user code.

Note: PEP 561 introduces the notion of a partial stub package, implemented in #5227. I think however this is a bit different use case that I don't want to mix with this one for two reasons:

Partial packages in PEP 561 are mainly focused on interaction between stubs and inline/runtime packages.
The proposed feature may be also used in typeshed, not only for installed stub packages.

emmatyping

The implementation looks good to me! Just some minor typos to fix.

emmatyping · 2018-06-18T19:54:56Z

    is_stub = False
    # Is this loaded from the cache and thus missing the actual body of the file?
    is_cache_skeleton = False
+    # Is this representns an __init__.pyi stub with a module __getattr__


Is this representns -> Does this represent

emmatyping · 2018-06-18T19:57:50Z

+                 # If `temporary` is True, this State is beeing created to just
+                 # quickly parse/load the tree, without an intention to further
+                 # process it. With this flag, any changes to external state as well
+                 # as error reprting should be avoided.


reprting -> reporting

emmatyping · 2018-06-18T20:08:13Z

+                ret = func.type.ret_type
+                if isinstance(ret, UnboundType) and not ret.args:
+                    sym = self.sem.lookup_qualified(ret.name, func, suppress_errors=True)
+                    # We only interpred package as partial if __getattr__ return type


We only interpred -> We only interpret a

Also, if -> if the

emmatyping · 2018-06-18T20:10:12Z

+x = b.f()
+[file a/__init__.pyi]
+from typing import Any
+def __getattr__(attr: str) -> Any: pass


Don't we usually use an ellipsis for stubs/tests?

gvanrossum

Could you also add some tests that verify module-specific settings of ignore_missing_imports work correctly?

gvanrossum · 2018-06-18T20:35:46Z

                 caller_line: int = 0,
                 ancestor_for: 'Optional[State]' = None,
                 root_source: bool = False,
+                 # If `temporary` is True, this State is beeing created to just


beeing -> being

gvanrossum · 2018-06-18T20:37:35Z

                path, follow_imports = find_module_and_diagnose(
                    manager, id, self.options, caller_state, caller_line,
-                    ancestor_for, root_source)
+                    ancestor_for, root_source, temporary)


I'd use quick=temporary here since it's an optional flag. (Though maybe more of the args should use keyword=value then?

Though maybe more of the args should use keyword=value then?

I would do this for quick only, because it is a bit "more special" than other.

gvanrossum · 2018-06-18T20:39:26Z

      caller_line: the line number of the import
      ancestor_for: the child module this is an ancestor of, if applicable
      root_source: whether this source was specified on the command line
+      quick: skip any error diagnosis and reporting


It's kind of odd to have a flag that suppresses the and_diagnose part of the function's name. :-) Also quick reminds (me) of --quick-and-dirty but it has nothing to do with that. Also might want to clarify that ModuleNotFound is still raised. (Hm, the docstring should probably mention that that is raised in the first place?)

OK, I will rename it to skip_diagnose to be more explicit.

gvanrossum · 2018-06-18T20:42:43Z

            follow_imports = 'normal'
-
+        if quick:
+            return (path, follow_imports)


This is a little odd -- it's really just skipping the next if-elif block (up to line 2309 below). Would it be clearer to write it as

if quick: pass elif follow_imports == 'silent': ... elif ...

? Alternatively

if not quick: if follow_imports == 'silent': ... elif ...

gvanrossum · 2018-06-18T20:47:24Z

+    """
+    while '.' in id:
+        parent, _ = id.rsplit('.', 1)
+        parent_mod = None


Maybe move this into the except block (L2345)? I was looking whether parent_mod is always defined (because it's not set in the except clause) and this felt too far back up. A different way to structure this would be parent_mod = manager.modules.get(parent) followed by if parent_mod is None.

gvanrossum · 2018-06-18T20:49:55Z

+                pass
+            else:
+                parent_mod = parent_st.tree
+        if parent_mod:


is not None

gvanrossum · 2018-06-18T20:50:31Z

                'names': self.names.serialize(self._fullname),
                'is_stub': self.is_stub,
                'path': self.path,
+                'is_partial_stub_package': self.is_partial_stub_package


Trailing comma.

gvanrossum · 2018-06-18T20:53:09Z

+                ret = func.type.ret_type
+                if isinstance(ret, UnboundType) and not ret.args:
+                    sym = self.sem.lookup_qualified(ret.name, func, suppress_errors=True)
+                    # We only interpred package as partial if __getattr__ return type


Also, if -> if the

gvanrossum · 2018-06-18T20:55:36Z

+def __getattr__(attr: str) -> int: pass
+[out]
+main:1: error: Cannot find module named 'a.b'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)


Would it be too much to ask to add a note pointing to the presence of __getattr__ with the wrong return type?

I can create a follow-up issue after we are done with this PR, unfortunately it will require some efforts to implement (and most likely a new flag on MypyFile, that I would rather avoid)

I opened #5276 for this.

gvanrossum · 2018-06-18T20:57:40Z

+[builtins fixtures/module.pyi]
+[out]
+main:1: error: Cannot find module named 'a.b.c'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)


FWIW when it's about a subpackage, the part of the note about setting MYPYPATH does not apply. I'm in general not a fan of this flag -- does it really help users that much? CC: @JukkaL

FWIW when it's about a subpackage, the part of the note about setting MYPYPATH does not apply.

This is not something I changed, so I would make it in a separate PR. But I actually also don't like suggesting to set MYPYPATH, I think this adds more confusion than help.

OK, I filed #5241.

ilevkivskyi · 2018-06-19T11:54:57Z

@gvanrossum

Could you also add some tests that verify module-specific settings of ignore_missing_imports work correctly?

I am not sure what do you mean here. Could you please show an example of what you would like to test?

gvanrossum

Could you also add some tests that verify module-specific settings of ignore_missing_imports work correctly?

I am not sure what do you mean here. Could you please show an example of what you would like to test?

I was hoping for a test that has a mypy.ini like this:

[mypy]
[mypy-a]
ignore_missing_imports = True

And then in a.py have an import that would cause an error without the flag, and in b.py (or main.py) the same import should still error. But it looks like such a test would be complicated to craft and there are no tests of that feature currently, so I don't think it makes sense to require it.

gvanrossum · 2018-06-19T17:24:44Z

+[builtins fixtures/module.pyi]
+[out]
+main:1: error: Cannot find module named 'a.b.c'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)


OK, I filed #5241.

gvanrossum · 2018-06-19T17:29:19Z

    """Find a module by name, respecting follow_imports and producing diagnostics.

+    If the module is not found, then the ModuleNotFound exception is raised.
    Args:


Conventionally there's a blank line before Args:.

gvanrossum · 2018-06-19T17:30:27Z

+def __getattr__(attr: str) -> int: pass
+[out]
+main:1: error: Cannot find module named 'a.b'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)


ilevkivskyi · 2018-06-25T18:06:50Z

@gvanrossum I think I implemented all the requested changes. Is there something here that needs to be done?

gvanrossum

Thanks, looks good!

The two ancestors json.JSONMixin and werkzeug.wrappers.Request of cmk.gui.http.Request break mypy checking. For example access to not existing members is not detected anymore. This commit fixes the issue with the json.JSONMixin class. The problem here is that the typesheds distributed with mypy, that are normally available in our virtualenv (lib/python3.7/site-packages/mypy/typeshed/third_party/2and3/werkzeug/wrappers.pyi) miss the typesheds for werkzeug.wrappers.json. I tried hard to add partial typesheds for this single file to tests-py3/typeshed, but did not find a working solution. If you have a good solution, please let me know. I tried to make use of the __getattr__ mechanic added in python/mypy#5231 and the PEP 561 "partial" feature. The only working solution I came up with was copying the original werkzeug typesheds and extending them. Sorry :/. At least it's better than copying the werkzeug.wrappers.json implementation to our code ;). Change-Id: Ie2723ab286188faa67d4738c226462a162552421

Ivan Levkivskyi added 7 commits June 18, 2018 02:39

Add basic support

ee57713

Better support

c72a65c

Add some comments and docstrings

1debe2f

Be more strict with return type

aa4a9d4

Fix lint

1ac99a9

Tighted subpackages

e6d458e

Add one more test

e3185d8

emmatyping reviewed Jun 18, 2018

View reviewed changes

gvanrossum requested changes Jun 18, 2018

View reviewed changes

Ivan Levkivskyi added 2 commits June 19, 2018 12:35

Address CR first part

1fb3b1d

Address CR: second part

c0bf548

gvanrossum reviewed Jun 19, 2018

View reviewed changes

Ivan Levkivskyi added 3 commits June 25, 2018 17:53

Fix self-check; add one test case

700391e

Merge remote-tracking branch 'upstream/master' into getattr-init

fdd870d

Fix docstring formatting; update tests

daede0f

ilevkivskyi mentioned this pull request Jun 25, 2018

Show a more detailed error when __getattr__ in __init__.py has wrong signature #5276

Open

gvanrossum approved these changes Jun 25, 2018

View reviewed changes

ilevkivskyi merged commit 4c3f800 into python:master Jun 25, 2018

ilevkivskyi deleted the getattr-init branch June 25, 2018 19:14

Conversation

ilevkivskyi commented Jun 18, 2018

Uh oh!

emmatyping 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!

gvanrossum 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!

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 Jun 19, 2018

Uh oh!

gvanrossum 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!

ilevkivskyi commented Jun 25, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants