Improve mypy support for strawberry.field

[patrick91]

I think it is time to make a full copy of the mypy dataclasses plugin: https://github.com/python/mypy/blob/a6c3e0740fb7ddf65a89c1742c9d10c5fe2ed604/mypy/plugins/dataclasses.py

Our plugin reuses it but it doesn't work with strawberry.field, since dataclasses don't know anything about strawberry.field.

Currently we get this error:

import strawberry

def some_resolver() -> str:
    return ""

@strawberry.type
class Example:
    a: str
    b: str = strawberry.field(name="b")

demo_enum.py:9: error: Incompatible types in assignment (expression has type "StrawberryField", variable has type "str")

so we need to tell mypy that this is what we expect :)

[MasterBiryer]

+1

[peniqliotuv]

+1 - this is especially problematic when we have resolvers that explicitly invoke another resolver and expect the result from that resolver to be typed - leading to a lot of # type: ignore comments.

[peniqliotuv]

@patrick91 @BryceBeagle Here's a case I added to tests/mypy/test_fields.yml:

- case: test_type_annotation_field
  main: |
    import strawberry

    @strawberry.type
    class User:
        @strawberry.field
        def age(self) -> int:
          return 42
    reveal_type(User().age())
  out: |
    main:8: note: Revealed type is 'builtins.int'

Output

~/code/strawberry  main !1 ❯ poetry run pytest tests/mypy/test_fields.yml                                                                                                                  Py 3.7.3 Go 1.10.7 10:17:14 AM
=================================================================================================== test session starts ===================================================================================================
platform darwin -- Python 3.7.3, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
django: settings: tests.django.django_settings (from ini)
rootdir: /Users/jerry.tsui/Code/strawberry, configfile: pyproject.toml
plugins: emoji-0.2.0, cov-2.11.1, mypy-plugins-1.6.1, asyncio-0.14.0, benchmark-3.2.3, django-4.1.0, mock-3.5.1, flask-1.2.0
collected 8 items

tests/mypy/test_fields.yml 😃 😃 😃 😰 😃 😃 😃 😃

======================================================================================================== FAILURES =========================================================================================================
_______________________________________________________________________________________________ test_type_annotation_field ________________________________________________________________________________________________
/Users/jerry.tsui/Code/strawberry/tests/mypy/test_fields.yml:85:
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output:
E   Expected:
E     main:13: note: Revealed type is 'builtins.int' (diff)
E   Actual:
E     main:8: note: Revealed type is 'Any'          (diff)
E
E   Alignment of first line difference:
E     E: main:13: note: Revealed type is 'builtins.int'
E     A: main:8: note: Revealed type is 'Any'
E             ^
================================================================================================= short test summary info =================================================================================================
FAILED 😰  tests/mypy/test_fields.yml::test_type_annotation_field -

[peniqliotuv]

@patrick91 does the refactor of StrawberryField version 0.53.0 solve this?

[patrick91]

@patrick91 does the refactor of StrawberryField version 0.53.0 solve this?

I think we chatted about this on discord, but for visibility, we need to update the MyPy plugin, but unfortunately that's blocked by this PR: https://github.com/python/mypy/pull/9925

Hopefully it gets merged and released soon :)

this is also useful: https://github.com/python/mypy/issues/3157

[patrick91]

ok, so I spent a bit more time on this, and it might be possible, but it might still be best to use a plugin for this anyway.

so when we do this:

import strawberry


@strawberry.type
class User:
    @strawberry.field
    def age(self) -> int:
        return 42

we create a StrawberryField, but then dataclasses replaces it with the previous value, so after running @strawberry.type cls.age is the method again which is good as it allows us to call it, but also we kinda lose reference to the StrawberryField instance (but this is probably ok for now as we are going to change that strawberry.type does).

One option here would be to change strawberry.field type signature to say that it doesn't return a StrawberryField, instead we can tell mypy that it returns the decorated method. This might break other usages of strawberry.field (namely x: int = strawberry.field(name="Something")), but it is a start :)

[peniqliotuv]

Thanks for getting back to me @patrick91 ! Would you recommend waiting until python/mypy#9925 is merged (i.e: is Strawberry planning on taking the mypy plugin approach or the strawberry field refactor approach?)

[patrick91]

Thanks for getting back to me @patrick91 ! Would you recommend waiting until python/mypy#9925 is merged (i.e: is Strawberry planning on taking the mypy plugin approach or the strawberry field refactor approach?)

I think the best option would be being able to use the mypy plugin, but I suspect we could already do something to make the experience better (for example we could change the signature of the property/method when the plugin hooks into strawberry.type) but not sure how much work that would be

[peniqliotuv]

Thanks for getting back to me @patrick91 ! Would you recommend waiting until python/mypy#9925 is merged (i.e: is Strawberry planning on taking the mypy plugin approach or the strawberry field refactor approach?)

I think the best option would be being able to use the mypy plugin, but I suspect we could already do something to make the experience better (for example we could change the signature of the property/method when the plugin hooks into strawberry.type) but not sure how much work that would be

I see - given that it's blocking on the Mypy PR getting merged, would an appropriate approach right now maybe be to do a refactor in our own codebase of @strawberry.field(...) to some custom decorator that we can manually cast the return type by python/mypy#3157

[patrick91]

@peniqliotuv that would work I think 😊

[gonzalezzfelipe]

Hey there, I'm having this issue. Now that the https://github.com/python/mypy/pull/9925 is closed, what is the solution?

[patrick91]

@gonzalezzfelipe sorry I missed your comment! what issue are you having exactly? We did a few improvements on typing since we opened this issue. Can you post a reproduction example? :)

[patrick91]

I think this was fixed!

[taylor-cedar]

Where was this fixed? I am still running into this error

[patrick91]

@taylor-cedar can you open a new issue with the full errors and replication? 😊