New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin to typecheck attrs-generated classes #4397

Merged
merged 84 commits into from Feb 13, 2018

Conversation

Projects
None yet
6 participants
@euresti
Contributor

euresti commented Dec 20, 2017

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 euresti changed the title from RFC: Attrs plugin to RFC: Plugin to typecheck attrs-generated classes Dec 20, 2017

@euresti

This comment has been minimized.

Contributor

euresti commented Dec 23, 2017

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.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

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,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 23, 2017

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 26, 2017

Contributor

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

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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).

This comment has been minimized.

@euresti

euresti Dec 23, 2017

Contributor

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

This comment has been minimized.

@euresti

euresti Dec 23, 2017

Contributor

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 24, 2017

Contributor

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?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2017

Collaborator

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.

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("_")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

@@ -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 comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 23, 2017

Contributor

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

You don't need these lines.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

This and some other tests are too large. Try splitting it in half (like the previous 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]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 23, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 26, 2017

Contributor

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

This comment has been minimized.

Collaborator

ilevkivskyi commented Dec 23, 2017

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

This comment has been minimized.

Contributor

euresti commented Dec 23, 2017

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

This comment has been minimized.

Collaborator

ilevkivskyi commented Dec 24, 2017

Actually attr has a @dataclass decorator

Ah, OK, I didn't know.

@euresti

This comment has been minimized.

Contributor

euresti commented Dec 26, 2017

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 euresti changed the title from RFC: Plugin to typecheck attrs-generated classes to Plugin to typecheck attrs-generated classes Dec 26, 2017

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

This comment has been minimized.

@euresti

euresti Dec 28, 2017

Contributor

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

This comment has been minimized.

Member

gvanrossum commented Dec 30, 2017

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Jan 3, 2018

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

This comment has been minimized.

Member

hynek commented Jan 3, 2018

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

This comment has been minimized.

Collaborator

ilevkivskyi commented Jan 3, 2018

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

This comment has been minimized.

Contributor

euresti commented Jan 3, 2018

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

@euresti

This comment has been minimized.

Contributor

euresti commented Jan 5, 2018

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

This comment has been minimized.

Member

gvanrossum commented Jan 5, 2018

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

@euresti

This comment has been minimized.

Contributor

euresti commented Jan 6, 2018

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

This comment has been minimized.

Contributor

euresti commented Jan 9, 2018

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.

@euresti

This comment has been minimized.

Contributor

euresti commented Feb 2, 2018

@ambv can you post the code that exhibits this problem (4 above) just so I understand exactly.

@chadrik

This comment has been minimized.

Contributor

chadrik commented Feb 2, 2018

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

This comment has been minimized.

Collaborator

ilevkivskyi commented Feb 3, 2018

@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

This comment has been minimized.

Contributor

euresti commented Feb 3, 2018

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

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Feb 3, 2018

@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

This comment has been minimized.

Contributor

euresti commented Feb 3, 2018

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

euresti added some commits Feb 4, 2018

@euresti

This comment has been minimized.

Contributor

euresti commented Feb 5, 2018

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

@ambv

This comment has been minimized.

Contributor

ambv commented Feb 5, 2018

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 added some commits Feb 5, 2018

@euresti

This comment has been minimized.

Contributor

euresti commented Feb 5, 2018

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 added some commits Feb 5, 2018

@euresti

This comment has been minimized.

Contributor

euresti commented Feb 11, 2018

@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 :-)

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 11, 2018

Collaborator

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

[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'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Feb 11, 2018

Collaborator

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

This comment has been minimized.

Contributor

euresti commented Feb 13, 2018

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

This comment has been minimized.

Member

gvanrossum commented Feb 13, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@euresti euresti deleted the euresti:attrs_plugin branch Feb 13, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment