Skip to content
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

Add PEP484 stubs #238

Merged
merged 74 commits into from
Jul 12, 2018
Merged

Add PEP484 stubs #238

merged 74 commits into from
Jul 12, 2018

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Aug 27, 2017

Here's a first take at PEP484-compatible pyi stubs. The discussion in #215 has been mostly focused on runtime type checking, so I wanted to make sure that the plan discussed there will work with static analysis as well.

Here's a quick test of the stubs that I put together to run against mypy:

from typing import cast
import attr

@attr.s
class C(object):
    x : int = attr.ib(convert=int)
    y: str = attr.ib()

c = C('1', 'foo')  # E: Too many arguments for "C"
c.x = 2
c.y = 'bar'
c > C()  # E: Unsupported left operand type for > ("C")
str(c)

# errors below are correctly detected:
c.x = 'foo'  # E: Incompatible types in assignment (expression has type "str", variable has type "int")
c.y = 1  # E: Incompatible types in assignment (expression has type "int", variable has type "str")

D = attr.make_class('D', {'x': cast(attr._CountingAttr, attr.ib())})
d = D(2)
# errors below are not correctly detected (mypy knows nothing about D other than that it's a type)
d.x ='foo'

mypy knows nothing about the dunder methods added by attrs, so cmp methods and __init__ fail. This will have to be solved with a mypy plugin, which I'll look into next. Same goes for attr.make_class though that's likely to be more difficult.

One important thing to note: in order to ensure that y: str = attr.ib() does not trigger a mypy error, the return type of attr.ib is Any instead of _CountingAttr. A side effect of this is that it complicates passing the result of attr.ib to functions that expect _CountingAttr instances. There are a handful of strategies to solve this:

  1. use typing.cast: ugly and verbose
  2. create, or choose an existing, alias for attr.ib that will return _CountingAttr, to use with functions like make_class.
  3. add another argument (make=True?) whose presence invokes an @overload of attr.ib that returns _CountingAttr
  4. slight variation on 2. create a public alias for _CountingAttr and copy the keyword defaults from attr.ib to _CountingAttr.__init__. In other words, if you use static type checking and are working with a function that expects _CountingAttr instances, use _CountingAttr() directly instead of attr.ib()

I favor the 3rd solution. After some consideration I favor 4.

If you're interested in accepting this, the next things to figure out are:

  • how to make tests for this, and run them on travis
  • how best to install the stubs (should they be moved to python-typeshed?)

Let me know what you think.

edit added option 4.

@hynek
Copy link
Member

hynek commented Aug 28, 2017

So, we need a mypy-Plugin in any case (please coordinate with @Tinche if you want to help, so we don't end up with duplicate work here!), [c|sh]ouldn't that plugin also handle the y: str = attr.ib() problem or am I missing something here?

@Tinche
Copy link
Member

Tinche commented Aug 28, 2017

I'm not really sure how to handle the stubs; does mypy scan the PYTHONPATH for them? I seem to remember it doesn't. Putting them into typeshed is not a good solution imho, what about versioning? Maybe someone should go ask them what to do exactly.

But stubs aren't really the big issue here. We basically need a mypy plugin that would run between the 2nd pass semantic analysis and the typecheck pass that would inject attrs generated stuff into attrs classes, and I think mypy doesn't have this type of plugin yet.

@chadrik
Copy link
Contributor Author

chadrik commented Aug 28, 2017

So, we need a mypy-Plugin in any case (please coordinate with @Tinche if you want to help, so we don't end up with duplicate work here!), [c|sh]ouldn't that plugin also handle the y: str = attr.ib() problem or am I missing something here?

I think that the plugin should focus on exposing the dunder methods to mypy, and not concern itself with the return value of attr.ib. What I'm proposing here for attr.ib is not only sufficient as-is but may be preferable to trying to solve via a mypy plugin. Here's why:

The core problem from a static type-checking POV is that in our example we want C.x to be a _CountingAttr and C(1).x to be an int. This is anathema to static type checking, which wants a variable to always have one type. For example, this is an error in mypy:

x = 1
x = 'foo'  # error

The typing.ClassVar construct mentioned in a related issue adds greater restriction: it says that a class attribute cannot be modified from an instance:

class C:
    x : ClassVar[int] = 1

C.x = 2  # ok
C().x = 2 # error

So that's not what we want either.

Could we create a mypy plugin that allows this dual-type behavior between classes and instances? Maybe, I don't know the API well enough to say for sure, but my instincts tell me that it will be far more difficult than adding the attr.s dunder support, precisely because the notion of rebinding types bucks against how static type checking works in general.

As @Tinche pointed out, the current mypy plugin system does not expose everything needed to solve our issues for attrs: attempting to solve the attr.ib problem might require exposing many more hooks to the plugin system, and if this requires some kind of specific support for rebinding of types, the mypy folks might not approve it.

I believe that the desire to make attr.ib "just work" with existing code in all cases is a perspective that users of static type checking won't share. I've been working towards having complete static type checking for a medium sized code base and it requires changing the way that you code in many cases. Given my exposure to the static way of thinking, the prospect of changing existing code to differentiate between _CountingAttr objects and their typed attributes is a perfectly natural and sufficient solution.

Here's an example showing those two use cases :

import attr

@attr.s
class C(object):
    x : int = attr.ib(convert=int)
    y: str = attr.ib()

D = attr.make_class('D', {'x': attr.CountingAttr()})

A user who is experienced with static type checking will actually find it unintuitive if the attribute types are allowed to change. Yes, we're lying to the static type checker (e.g. by saying x is an int), and certain operations are off limits if you want the analysis to succeed (e.g. C.x.metadata without also using typing.cast) but I've seen quite a bit of both of these approaches in the official stdlib stubs.

All these things considered, I think we should focus on writing a mypy plugin that exposes the dunder methods added by @attr.s as well as attr.make_class.


I'm not really sure how to handle the stubs; does mypy scan the PYTHONPATH for them? I seem to remember it doesn't.

Mypy has its own search path defined by MYPYPATH, whereas PyCharm searches the PYTHONPATH. Both tools will prefer .pyi over .py.

Putting them into typeshed is not a good solution imho, what about versioning? Maybe someone should go ask them what to do exactly.

I agree about the disadvantages of typeshed. There is an epic typing issue discussing installation of third-party stubs, and it's still not resolved.

Maybe the simplest solution is just to move the annotations into the code itself? This also has a major advantage in PyCharm: when working with modules that provide stubs, the "go to definition" behavior will take you to the definition within the .pyi file, which doesn't contain docs or any code.

edit: The only disadvantage I can think of to moving the annotations into the code is that it will add typing as a requirement for python 2.7 (already present in stdlib in 3.4+).

But stubs aren't really the big issue here. We basically need a mypy plugin that would run between the 2nd pass semantic analysis and the typecheck pass that would inject attrs generated stuff into attrs classes, and I think mypy doesn't have this type of plugin yet.

I agree that for complete support we'll need such a plugin, but adding typing to attr's public API will provide immediate benefits in tools like PyCharm, where perfection is not required. This is a step in the right direction and sets us up to solve the harder issue of dunder methods.

@codecov
Copy link

codecov bot commented Aug 30, 2017

Codecov Report

Merging #238 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #238   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         856    830   -26     
  Branches      183    174    -9     
=====================================
- Hits          856    830   -26
Impacted Files Coverage Δ
src/attr/exceptions.py 100% <0%> (ø) ⬆️
src/attr/__init__.py 100% <0%> (ø) ⬆️
src/attr/validators.py 100% <0%> (ø) ⬆️
src/attr/_compat.py 100% <0%> (ø) ⬆️
src/attr/_make.py 100% <0%> (ø) ⬆️
src/attr/_funcs.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d71759...2b19c36. Read the comment docs.

@chadrik chadrik mentioned this pull request Sep 17, 2017
@chadrik
Copy link
Contributor Author

chadrik commented Sep 18, 2017

I read all of the epic typing issue on third-party pyi stubs, as well as the newly drafted PEP on the subject, and here's the synopsis:

  • For package maintainers wishing to ship stub files containing all of their type information, it is prefered that the *.pyi stubs are alongside the corresponding *.py files.

  • setuptools will soon grow a new typed argument:

    setup(
        ...
        setup_requires=["typing"],
        typed="stubs",
        ...
    )
    
  • mypy and other type checkers will work out-of-the-box with compliant packages

That means that installing the pyi files alongside the py files is correct.

@Tinche @hynek, do you all have any concerns about merging this PR prior to making a mypy plugin? Any other concerns that you'd like addressed?

I still have to fix a few remaining typing issues and deal with documentation and testing, I just want to make sure I have the green light first.

thanks.

@hynek
Copy link
Member

hynek commented Sep 24, 2017

Correct me if I'm wrong, but it seems to me that merging this is kind of pointless without a mypy plugin because it fixes only a part of the failures? Unless I miss something, I’d put this PR on hold until we can actually use mypy (better sooner than later as far as I'm concerned…).

@chadrik
Copy link
Contributor Author

chadrik commented Sep 25, 2017

Correct me if I'm wrong, but it seems to me that merging this is kind of pointless without a mypy plugin because it fixes only a part of the failures?

This is probably true for mypy, since it would typically be used as part of a CI process where erroneous failures are problematic, but this PR would immediately benefit users of IDEs with PEP 484 support, like PyCharm.

Below is an animation showing how the stubs improve PyCharm's completions. What's great is that this works for users who aren't yet on python 3.6 and can't take advantage of variable annotations. It also gives completions for utility functions like attr.fields. This is something that I'd like to have right away, and I'm sure others will agree.

pycharm completion

@hynek
Copy link
Member

hynek commented Sep 25, 2017

I see, so the question becomes: [how] can we meaningfully test them? Ideally both for consistency and completeness?

@chadrik
Copy link
Contributor Author

chadrik commented Sep 25, 2017

The mypy project contains some pytest utilities that shouldn't be to hard to port/utilize, but mpy will have to be added to the list of test requirements.

Our test runner will look something like this
Tests files look like this

The test runner code can only be run in python 3.5+ but the code within the .test files can be written using 2 or 3. I see two approaches to running them:

  • Create tests/test_stubs.py and use conftest.collect_ignore to ensure that it only runs in python 3.5+
  • Create a separate entry point script and run it using tox

@chadrik
Copy link
Contributor Author

chadrik commented Sep 28, 2017

Update: I've got the test running added, but the stubs don't check properly in mypy due to a bug: python/mypy#4027

Let me know if you have any comments on how the tests are run.

it does not make sense to test the stubs in multiple python *runtime* environments (e.g. python 3.5, 3.6, pypy3) because the results of static analysis wrt attrs is not dependent on the runtime.  Moreover, mypy is not installing correctly in pypy3 which has nothing to do with attrs.
@chadrik chadrik mentioned this pull request Nov 4, 2017
It is crucial to ensure that make_class() works with attr.ib(), as a result we no longer have any functions that care about _CountingAttr.
this allows for an abbreviated idiom: `x: List[int] = Factory(list)`
In preparation for removing them from the pyi_stubs branch.
This is a final test of the current stubs before moving the stub tests to a new branch.
Replace with a simple mypy pass/fail test
@chadrik
Copy link
Contributor Author

chadrik commented Jul 11, 2018

@hynek ready for your review!

Copy link
Contributor

@euresti euresti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

import attr


# Type argument

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,240 @@
from typing import (

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave the review of the typing part to @chadrik and only comment on the packaging part.

Which is currently broken. :) And it shows one problem we’ll always have: we have to compete with typeshed's stubs. :|

I suspected it might not work, so to prove it, I installed mypy and deleted attrs’ stubs from it…I wish there were a better way?

Anyways, you need to add include_package_data=True, to the setup.py call so the typed.py gets actually installed.


Also please remove the .idea from .gitignore and add it to your ~/.gitignore_global we try to steer clear of editor-specific ignores.


Finally, let’s move the typing env higher up in tox’s envlist (I don’t care about Travis). It runs fast, so it’s quick feedback before the slow part begins.

Thanks!!!

@hynek
Copy link
Member

hynek commented Jul 11, 2018

(As for the typeshed competition problem: does anyone have an idea how to ensure we’re using our own stubs? Maybe define some private sentinel object that typeshed doesn’t have and check against it in typing_example.py?)

@ethanhs
Copy link
Member

ethanhs commented Jul 11, 2018

Hm, PEP 561 (and the mypy implementation) dictates that installed packages should take precedence over typeshed. Is that not happening?

@hynek
Copy link
Member

hynek commented Jul 11, 2018

It is. However this PR’s packaging has a bug and we can’t tell because it falls back to typeshed.

@chadrik
Copy link
Contributor Author

chadrik commented Jul 11, 2018

Hi all, I addressed your notes.

does anyone have an idea how to ensure we’re using our own stubs?

Why don't we remove them from typeshed? I don't see any good reason to keep them in both, and there are a lot of reasons not to.

@ethanhs
Copy link
Member

ethanhs commented Jul 11, 2018

Why don't we remove them from typeshed?

The only reason not to AFAIK is that if the pyre/pytype folks (who haven't yet implemented/are not implementing PEP 561 respectively) may want them in typeshed. That could make things more complicated, but I'd suggest opening an issue and asking.

E: forgot critical "may" want them

@hynek
Copy link
Member

hynek commented Jul 11, 2018

Wouldn’t that be a regression for users of older versions of attrs? 🤔

@chadrik
Copy link
Contributor Author

chadrik commented Jul 11, 2018

Wouldn’t that be a regression for users of older versions of attrs?

We're talking about the bloodiest of bleeding edge here. I just don't think it's worth taking on the work -- and additional issues -- of maintaining both for the tiny niche of users who are using the typeshed stubs but not able to update to the latest version of attrs, or who are using pyre or pytype and are not able to wait for PEP 561 support. Keep in mind that the typeshed stubs are limited to the bleeding-edge anyway, as they can only support a single version of attrs at a time.

@hynek hynek merged commit de104e0 into python-attrs:master Jul 12, 2018
@hynek
Copy link
Member

hynek commented Jul 12, 2018

Almost eleven months, 164 comments, a lot hence and forth, but we’ve made it! Thanks @chadrik and everyone else! Now we can work on details. :)

hynek added a commit that referenced this pull request Jul 12, 2018
@chadrik
Copy link
Contributor Author

chadrik commented Jul 12, 2018

Congrats everyone!

@euresti actually did a lot of the hardest work, I just pushed along the process.

@ethanhs Let me know when you get started on your pytest plugin. I have some thoughts on what an ideal test framework would look like, and would be happy to pitch in.

My next project is going to be lobbying the mypy devs to accept a PR to find plugins on the PYTHONPATH, so we can move ours into this repo.

@chadrik
Copy link
Contributor Author

chadrik commented Jul 12, 2018

Also, I've parked my old changes containing pytest and doctest plugins here: https://github.com/chadrik/attrs/tree/stubs_tests

@ethanhs
Copy link
Member

ethanhs commented Jul 12, 2018

Congrats!

@chadrik I likely will work on the plugin this weekend. I've opened https://github.com/ethanhs/pytest-stub-check/issues/1 and would welcome your input.

hynek added a commit that referenced this pull request Jul 14, 2018
hynek added a commit that referenced this pull request Jul 28, 2018
* Add narrative docs for type annotations

* Better wording

* Use better code-block types

* Add newsfragment for #238 that refers to these docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants