Plugin to typecheck attrs-generated classes #4397

euresti · 2017-12-20T06:31:41Z

See http://www.attrs.org/en/stable/how-does-it-work.html for information on how attrs works.

The plugin walks the class declaration (including superclasses) looking for "attributes" then depending on how the decorator was called, makes modification to the classes as follows:

init=True adds an __init__ method.
cmp=True adds all of the necessary __cmp__ methods.
frozen=True turns all attributes into properties to make the class read only.
Remove any @x.default and @y.validator decorators which are only part of class creation.

Fixes #2088

euresti · 2017-12-23T00:22:25Z

Ok this supports quite a bit of features now.

Things that aren't working correctly:
a) subclasses that overwrite attributes from parents.
b) Setting a default using a decorator on a method. (i.e. @y.default)
c) The @dataclass decorator. ;)

I think everything else is working fairly well. I kind of wish everything wasn't so hard coded on the method names. Many users of attrs (us included) create helper functions to add metadata and other stuff. I'll probably have to make the code a bit more adaptable to that.

ilevkivskyi

Thank you for working on this and sorry for a delay. Here are some comments.

ilevkivskyi · 2017-12-22T23:56:55Z

mypy/plugin.py

+    cast
+
+from mypy import messages
+from mypy.nodes import Expression, StrExpr, IntExpr, UnaryExpr, Context, \


Our style is to use (...) for multiline imports instead of \, see for example right below from mypy.types import etc.

ilevkivskyi · 2017-12-23T00:00:17Z

mypy/plugin.py

+    arg_names = [arg.variable.name() for arg in args]
+    arg_kinds = [arg.kind for arg in args]
+    assert None not in arg_types
+    signature = CallableType(cast(List[Type], arg_types), arg_kinds, arg_names,


Is it possible to avoid the cast, why it is necessary? We try to avoid using casts.

The named tuple code does the same thing. arg_types is technically a List[Optional[Type]] but note the assert None not in arg_types right above to make sure it's right.

I think the cast is not required with that assert. Have you actually tried removing it?

Yeah I get:

mypy/plugin.py:580: error: Argument 1 to "CallableType" has incompatible type "List[Optional[Type]]"; expected "List[Type]"

When running mypy mypy/plugin.py --strict

ilevkivskyi · 2017-12-23T00:03:15Z

mypy/plugin.py

+                          default: Optional[bool]) -> Optional[bool]:
+        for arg_name, arg_value in zip(call.arg_names, call.args):
+            if arg_name == name:
+                # TODO: Handle None being returned here.


Don't forget to fix TODO items before the PR is merged.

ilevkivskyi · 2017-12-23T00:05:28Z

mypy/plugin.py

+        return
+
+    # To support nested classes we use fullname(). But fullname is <filename>.<variable name>
+    # and named_type() expects only the name.


I don't understand this comment. Why do you need the self type here? It is normally bound at the call site (i.e. type variables of a generic type are fixed).

Oh I see. AnyType(TypeOfAny.unannotated) is what i need here! Thanks!

Oh wait. I need it for
def __lt__(self, other: SelfType)

OK, then please use it only in methods that need it. Also please double-check that this reflects the actual runtime semantics/docs.

Actually, I found out that if i do ctx.api.accept(func) on the FuncDef I create it does a lot of things for me (setting the type of the self arg, being one of them). Is that a reasonable thing to do?

Is that a reasonable thing to do?

I am not sure it is really needed, but if you find a case where this is necessary then go ahead.

ilevkivskyi · 2017-12-23T00:07:27Z

mypy/plugin.py

+            for stmt in info.defn.defs.body:
+                if isinstance(stmt, AssignmentStmt) and isinstance(stmt.lvalues[0], NameExpr):
+                    lhs = stmt.lvalues[0]
+                    name = lhs.name.lstrip("_")


Why this is necessary? Does attr has specific treatment of underscores? If yes, then please add a comment.

ilevkivskyi · 2017-12-23T00:13:58Z

mypy/test/data.py

@@ -101,6 +101,13 @@ def parse_test_cases(
                    src_path = join(os.path.dirname(path), arg)
                    with open(src_path) as f:
                        files.append((join(base_path, 'typing.pyi'), f.read()))
+                elif p[i].id == 'add-module':


This whole block is not necessary, just put the "pseudo stub" for attrs in mypy/test-data/unit/lib-stub.

Ok cool. I didn't know if that was ok since it's not a stdlib.

ilevkivskyi · 2017-12-23T00:14:57Z

test-data/unit/fixtures/attr.pyi

@@ -0,0 +1,14 @@
+from typing import TypeVar, overload, Callable, Any


Move this file to mypy/test-data/unit/lib-stub as I explained above.

ilevkivskyi · 2017-12-23T00:17:32Z

test-data/unit/check-attr.test

+UnTyped(1, 2) == UnTyped(2, 3)
+UnTyped(1, 2) >= UnTyped(2, 3)
+[builtins fixtures/attr_builtins.pyi]
+[add-module fixtures/attr.pyi]


You don't need these lines.

ilevkivskyi · 2017-12-23T00:20:53Z

test-data/unit/check-attr.test

+[builtins fixtures/attr_builtins.pyi]
+[add-module fixtures/attr.pyi]
+
+[case testTypedAttrS]


This and some other tests are too large. Try splitting it in half (like the previous test).

ilevkivskyi · 2017-12-23T00:26:18Z

test-data/unit/check-attr.test

+reveal_type(C)  # E: Revealed type is 'def (y: Any) -> __main__.C'
+reveal_type(C.D)  # E: Revealed type is 'def (x: builtins.int) -> __main__.C.D'
+[builtins fixtures/attr_builtins.pyi]
+[add-module fixtures/attr.pyi]


Could you please add more tests? Check not only that definitions are processed correctly, but how they interact with various other concepts. For example, try to have good coverage at least for:

type aliases (both inside and outside of a class)

type variables

decorated generic classes

type inference

forward references (both inside and outside of class)

importing decorated classes

instance and class methods in decorated types

The only tests I couldn't write because I wasn't sure what you meant were "type inference" But I think I covered everything else. Let me know if you want more tests.

ilevkivskyi · 2017-12-23T00:29:45Z

The @dataclass decorator. ;)

Support for data classes is a separate topic/PR. Also, they will probably be supported directly like named tuples (not via plugin).

euresti · 2017-12-23T00:36:12Z

Actually attr has a @dataclass decorator, defined as

dataclass = partial(attrs, auto_attribs=True)

That's why there was a smiley.

Thanks for the review. I'll probably play with things again after the break.

ilevkivskyi · 2017-12-24T22:50:22Z

Actually attr has a @dataclass decorator

Ah, OK, I didn't know.

euresti · 2017-12-26T22:37:46Z

Ok I believe this is now feature complete. It's pretty long because the attrs feature set is pretty large. Let me know what other changes you want.

euresti · 2017-12-28T18:19:06Z

mypy/plugin.py

+
+
+# The names of the different functions that create classes or arguments.
+attr_class_makers = {


I could use some advice/opinion here. It's fairly common when using attrs to write your own wrappers around the functions to make coding easier. However the plugins only work on specific names so I tried to leave some way for users to add their own "class makers" and "attrib makers" to the plugin. But that's probably not the right way to do this.

gvanrossum · 2017-12-30T01:28:27Z

I think it would be good if @ambv looked at it. IIRC he is a big attrs user (and co-author of PEP 484).

…

On Dec 26, 2017 3:37 PM, "David Euresti" ***@***.***> wrote: Ok I believe this is now feature complete. It's pretty long because the attrs feature set is pretty large. Let me know what other changes you want. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4397 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACwrMk24KiF9UznjZSUVtIg0gMCy0lD-ks5tEXU8gaJpZM4RH9QS> .

ilevkivskyi · 2018-01-03T16:42:57Z

I think it would be good if @ambv looked at it. IIRC he is a big attrs user (and co-author of PEP 484).

This is a good idea. I already mentioned that it would be great if someone familiar with attrs will look at this PR (in particular at the tests, to check that attrs "semantics" is right) I was also thinking about @hynek or @Tinche.

Anyway, I will also make another pass of review soon.

hynek · 2018-01-03T16:57:11Z

This stuff is way over my head :) but one thing I’ve noticed: convert has been deprecated in favour of converter (to mirror validator).

ilevkivskyi · 2018-01-03T17:01:04Z

but one thing I’ve noticed: convert has been deprecated in favour of converter

Thanks! This is exactly the kind of feedback I wanted to see. Also later (when there will be more tests), you can comment if the error messages given by mypy are reasonable.

euresti · 2018-01-03T21:43:21Z

I can add support for converter.
And looking at 17.4.0 I'm also gonna have to change the MRO handling code.

euresti · 2018-01-05T06:17:21Z

Ok I've added support for converter and fixed the MRO issue.

I'm a little unhappy because in order to support subclassing correctly I had to add something to the TypeInfo instance. Basically I needed to add the list of Attributes somewhere so that a) I don't have to traverse the class bodies again for subclasses and b) I can modify the body as I traverse it e.g. to remove the @x.default decorators that don't make sense during static type checking.

Another way would be to add a Dict[TypeInfo, Attributes] to the DefaultPlugin and then all the attributes would be in that dictionary.

I'm also starting to think that the code is pretty large to all live in one method and am wondering if I should put in a separate file and even maybe a separate plugin from DefaultPlugin. I would like it to live in mypy though (rather than in attrs) because it'll be less likely to break.

gvanrossum · 2018-01-05T19:11:11Z

(I'm hoping Ivan will be able to review this in the next few weeks and guide it to completion.)

euresti · 2018-01-06T15:44:52Z

Ok I'm now mostly happy with the code. I got rid of the magic numbers, the MRO works correctly, and the tests pass. Feel free to review when you're ready. Also this is my first big mypy project so I expect I did lots of things wrong.

And a musing for v2:

The only other thing we might want is a way to allow users to say: this function returns an attrib or this function basically creates an attr class. I was thinking it could be a decorator. Something like:

@attr.register_attrib_func()
def type_checked(...)
    # Add my special validators.
    return attr.ib(...)

@attr.register_attrs_func(auto_attribs=True)
def myattr_maker(...)
    # Add my special validators.
    return attr.s(...)

At runtime the decorator does nothing, but in mypy we'd see the decorator and add the function to the list of functions we process. Additionally the decorator could change the default values for the methods (e.g. auto_attribs) etc.

However since that requires commits in attrs, typeshed and mypy I'd figure that's best left to v2. :)

euresti · 2018-01-09T18:37:09Z

Sigh. Just noticed that my caching of Attributes in the plugin breaks incremental mode + subclassing. I might have to move the caching back into TypeInfo so that I can serialize them in the mypy_cache.

chadrik · 2018-02-02T20:06:45Z

Regarding tests, I've added pretty thorough tests to my attrs PR for pyi stubs which cover both the attrs stubs and mypy plugin. I'm not sure where this should ultimately live: mypy, typeshed, attrs, or some combination thereof?

ilevkivskyi · 2018-02-03T00:45:41Z

@euresti There is no specific timeline IIUC, but I think we could get this in (including typeshed PR) in next few days. This is almost ready.

As for the incremental mode, maybe we can just allow plugins to add keys to a special metadata field of TypeInfo. Probably you can add it in already serialized form (like JSON), so it will be damped to cache as is, and then loaded as is. My understanding is that you just need to preserve flags for certain Vars in the class symbol table, their types are already (de-)serialized automatically. For example, if you have a class like this:

@attr.s(frozen=True)
class C:
    x: int = attr.ib(init=False)
    y: str

Then you can just write (pseudocode, hopefully you get the idea):

type_info.metadata['attrs'] = {'fileds': {'x': [], 'y': ['init']}, 'flags': ['frozen']}

Taking into account the flags (both global and per-field) and the types that you can find in class symbol table, you should be able to reconstruct all the necessary info from a deserialized base class.

euresti · 2018-02-03T14:46:46Z

@ambv I think I know your issue. It's a mypy issue: #3135

ilevkivskyi · 2018-02-03T16:28:33Z

@euresti

I think I know your issue. It's a mypy issue

Since decorators are important for attrs, maybe you can write a limited fix (in a separate PR)? Namely, just check the decorators that are function calls. They should be visited in semantic analysis and type check, you can see how it is done for a regular CallExpr. This would be a partial fix, but it will already catch the errors like @ambv discovered.

euresti · 2018-02-03T17:06:33Z

Definitely on my mind. Wanna get the serialization working first though.

euresti · 2018-02-05T00:20:08Z

Dang that was hard. But it works! Comments welcome!

ambv · 2018-02-05T11:26:56Z

Cool! This looks much better. I won't block you on this basis alone but we might want to split attr_class_maker_callback further in a future refactor. Paraphrasing Linus Torvalds, "if you need more than 3 levels of indentation, you’re screwed anyway, and should fix your program". Another rule of thumb that applies here: if you need comments to name separate sections in your function, those should really be separate functions :-)

As I said though, I won't bother you with doing this right now. Just something to keep in mind for future work.

euresti · 2018-02-05T19:31:17Z

Ok I refactored a bit more and cleaned up some stuff related to the converter method. Will leave this alone for a bit until I hear something. Thanks!

euresti · 2018-02-11T14:36:10Z

@ilevkivskyi @ambv Any more comments on this PR? I've already started using it on our code base and it's been pretty great.

ilevkivskyi

This is most probably my last review on this. I didn't look carefully through the code, but I already reviewed it few times and my understanding is that the latest changes are purely "logistical", i.e. writing some code as helper functions.

Thanks for this great work, this will be a useful addition for mypy users (and sorry for being pedantic sometimes :-)

ilevkivskyi · 2018-02-11T21:43:48Z

mypy/attrs_plugin.py

@@ -0,0 +1,498 @@
+"""Plugin for supporting the attrs library (http://www.attrs.org)"""


I would put this in a separate folder, not directly in mypy. I think mypy/plugins/attrs.py would be better.

ilevkivskyi · 2018-02-11T22:07:18Z

test-data/unit/check-incremental.test

+[out1]
+main:6: error: Revealed type is 'def (x: builtins.int) -> __main__.B'
+[out2]
+main:6: error: Revealed type is 'def (x: builtins.int) -> __main__.B'


Please add more tests for incremental mode. Here are some ideas:

Check situations with more attributes

Checks that access to attributes after deserialization results in a correct type and "kind" (i.e. being frozen etc.)

Check that signatures of deserialized dunder methods are correct.

Check situations where base attr class in module a.py is not changed in the second run, but its subclass in module b.py is changed so that this produces a type error.

Also vice-versa (error to no error)

Add test with more than two files

Add test with more than two incremental runs (for example: no error -> error -> no error in a subclass)

Here are two additional notes about incremental tests:

If a file results in errors, it is never actually deserialized from cache, this includes reveal_type. So although you can keep current tests (to check serialization, which happens always), please avoid reveal_type in future incremental tests, instead use an assignment to check that the type is as expected.

To add changes between runs, use this pattern:

[file a.py] import b ... [file a.py.2] import b ... [file b.py] ...

Important note: you don't need to add .2, (and .3 in one test) to all files, only to the files that are actually changed. It is better to actually not do this (to avoid touching unchanged file, and therefore considering its cache stale).

Don't add too many tests, just one case for each item I listed above.

euresti · 2018-02-13T06:48:00Z

Thanks for the review. I've added a couple more tests. I hope I got all your points.

ilevkivskyi

There is still some roughness, but I think we should move forward. Maybe we can polish implementation details later. If no-one is against, then I will merge this soon.

gvanrossum · 2018-02-13T15:00:53Z

+1 on merging!

python/mypy#4397 to be present in a release

euresti changed the title ~~RFC: Attrs plugin~~ RFC: Plugin to typecheck attrs-generated classes Dec 20, 2017

euresti force-pushed the attrs_plugin branch from 2c92b81 to c38d3ac Compare December 21, 2017 19:19

ilevkivskyi reviewed Dec 23, 2017

View reviewed changes

euresti changed the title ~~RFC: Plugin to typecheck attrs-generated classes~~ Plugin to typecheck attrs-generated classes Dec 26, 2017

euresti commented Dec 28, 2017

View reviewed changes

gvanrossum assigned gvanrossum and ilevkivskyi and unassigned gvanrossum Jan 5, 2018

euresti added 8 commits January 10, 2018 22:52

WIP: attrs_plugin_real

005547a

Handle untyped attrs

ecd0164

Handle auto_attribs and refactor

8f4dbd7

WIP: attrs_plugin

2b55334

WIP: attrs_plugin

83b008c

WIP: attrs_plugin

3bc5c11

WIP: attrs_plugin

aa55fe5

WIP: attrs_plugin

5f897b7

Refactor code, split into helper methods

3fa9e4f

euresti added 2 commits February 4, 2018 13:06

Work around circular import

cad44bb

Support incremental mode with attrs

0240092

euresti added 2 commits February 5, 2018 10:49

A little more refactoring

c37147d

Handle converter in one place

cb795cd

euresti added 4 commits February 5, 2018 11:40

Merge branch 'master' into attrs_plugin

b2b538c

Sync typeshed

d1e3e0b

Merge branch 'master' into attrs_plugin

6ef4a63

Fix test

feab654

euresti force-pushed the attrs_plugin branch from 23d5e04 to feab654 Compare February 11, 2018 00:51

ilevkivskyi reviewed Feb 11, 2018

View reviewed changes

euresti added 2 commits February 11, 2018 15:10

Move attrs_plugin to plugins/attrs

641230d

Add more incremental tests

bf49a87

ilevkivskyi approved these changes Feb 13, 2018

View reviewed changes

ilevkivskyi merged commit 91f2d36 into python:master Feb 13, 2018

euresti deleted the attrs_plugin branch February 13, 2018 15:42

yedpodtrzitko mentioned this pull request Feb 13, 2018

add mypy.plugins into setup.py packages #4570

Merged

glyph added a commit to glyph/klein that referenced this pull request Feb 20, 2018

can't wait for python/mypy#4583 and

f737b8f

python/mypy#4397 to be present in a release

chrahunt mentioned this pull request Nov 21, 2019

Add "no-new-attributes" behavior to our models pypa/pip#7313

Closed

		@@ -0,0 +1,14 @@
		from typing import TypeVar, overload, Callable, Any



		# The names of the different functions that create classes or arguments.
		attr_class_makers = {

		@@ -0,0 +1,498 @@
		"""Plugin for supporting the attrs library (http://www.attrs.org)"""

Plugin to typecheck attrs-generated classes #4397

Plugin to typecheck attrs-generated classes #4397

Conversation

euresti commented Dec 20, 2017 • edited Loading

euresti commented Dec 23, 2017

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Dec 23, 2017

euresti commented Dec 23, 2017

ilevkivskyi commented Dec 24, 2017

euresti commented Dec 26, 2017

Choose a reason for hiding this comment

gvanrossum commented Dec 30, 2017 via email

ilevkivskyi commented Jan 3, 2018

hynek commented Jan 3, 2018

ilevkivskyi commented Jan 3, 2018

euresti commented Jan 3, 2018

euresti commented Jan 5, 2018

gvanrossum commented Jan 5, 2018

euresti commented Jan 6, 2018

euresti commented Jan 9, 2018

chadrik commented Feb 2, 2018

ilevkivskyi commented Feb 3, 2018

euresti commented Feb 3, 2018

ilevkivskyi commented Feb 3, 2018

euresti commented Feb 3, 2018

euresti commented Feb 5, 2018

ambv commented Feb 5, 2018

euresti commented Feb 5, 2018

euresti commented Feb 11, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

euresti commented Feb 13, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

gvanrossum commented Feb 13, 2018 via email

euresti commented Dec 20, 2017 •

edited

Loading

ilevkivskyi Feb 11, 2018 •

edited

Loading