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

Deferred type annotations are evaluated in the wrong execution context #593

Closed
Vlad-Shcherbina opened this issue Oct 24, 2019 · 17 comments · Fixed by #760
Closed

Deferred type annotations are evaluated in the wrong execution context #593

Vlad-Shcherbina opened this issue Oct 24, 2019 · 17 comments · Fixed by #760
Labels

Comments

@Vlad-Shcherbina
Copy link

To reproduce, run the following program:

import attr
from typing import List, get_type_hints

@attr.s
class C:
    x = attr.ib(type='List[int]')

print(get_type_hints(C.__init__))

Expected result

{'return': <class 'NoneType'>, 'x': typing.List[int]}

Actual result

Traceback (most recent call last):
  File "example.py", line 8, in <module>
    print(get_type_hints(C.__init__))
  File "C:\Python37\lib\typing.py", line 1001, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "C:\Python37\lib\typing.py", line 260, in _eval_type
    return t._evaluate(globalns, localns)
  File "C:\Python37\lib\typing.py", line 464, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'List' is not defined

This annotation should be evaluated in the context of the module where it's defined, where List is in globals.

Version info

attrs 19.3.0
Python 3.7.3 (v3.7.3:ef4ec6ed12, Mar 25 2019, 22:22:05) [MSC v.1916 64 bit (AMD64)] on win32
@hynek
Copy link
Member

hynek commented Oct 24, 2019

I'm not sure that's something we can achieve TBH.

@wsanchez wsanchez added the Bug label Oct 24, 2019
@ivanprado
Copy link

I'm having the same problem. Is there any workaround in which the original __init__.__globals__ can be accessed somehow?

Also, is there any way to know if a __init__ method was created by attrs? (i.e. is_attrs_init(C.__innit_))

@euresti
Copy link
Contributor

euresti commented Feb 10, 2020

PS. You can workaround it with:

 get_type_hints(C.__init__, sys.modules[C.__module__].__dict__)

or

get_type_hints(C.__init__, globalns=globals())

@ivanprado
Copy link

Thanks @euresti for the proposals 👍 . Unfortunately, I think they won't work for me:

First proposal:

 get_type_hints(C.__init__, sys.modules[C.__module__].__dict__)

I need to obtain the hints only from the __init__ method so unless I can obtain the class from the method __init__ this solution won't work for me. Do you know if there is any way to obtain the class from the __init__ method? I mean, is possible to implement the method get_cls_from_attrs_init such that:

assert get_cls_from_attrs_init(C.__init__) == C

Second proposal

get_type_hints(C.__init__, globalns=globals())

The problem with this proposal is that the globals obtained from globals() are from a different module than C.__init__ module in my case, so it is not working.

Thank you!

@euresti
Copy link
Contributor

euresti commented Feb 10, 2020

You don't need the class. You only need the module:

def hints(method):
     return get_type_hints(method, sys.modules[method.__module__].__dict__)

@ivanprado
Copy link

Thanks, @euresti 👍 . That is very helpful, serves for most of my cases, thanks. But it is not working for the case of classes defined within functions. The following fails with NameError: name 'B' is not defined:

import sys
from typing import get_type_hints
import attr

def hints(method):
    return get_type_hints(method, sys.modules[method.__module__].__dict__)

def resolve():
    @attr.s(auto_attribs=True)
    class A:
        b: 'B'

    @attr.s(auto_attribs=True)
    class B:
        pass

    return hints(A.__init__)

resolve()

Any idea if there is any workaround for that?

By the other hand, I have discovered that this issue will get worse in future python 4.0, where posponed evaluation of annotations will be the default.

The following fails with NameError: name 'B' is not defined on Python 3.7. But it works if from __future__ import annotations is commented. And then it will fail by default in the upcoming Python 4.0.

from __future__ import annotations

from typing import get_type_hints

import attr

class B:
    pass

@attr.s(auto_attribs=True)
class A:
    b: B

get_type_hints(A.__init__)

Hope this is hepful.

ivanprado added a commit to scrapinghub/andi that referenced this issue Feb 11, 2020
It doesn't cover all the possible cases because there are limitations in attrs itself. See python-attrs/attrs#593

Enabled py35 in Travis
@euresti
Copy link
Contributor

euresti commented Feb 11, 2020

Local scopes are very hard to remember. Do you need to pass around __init__ methods? One should not be calling those anyway. Maybe your code can accept the B for these cases. (Full disclosure I have no idea what your code is doing)

this issue will get worse in future python 4.0, where posponed evaluation of annotations will be the default.

That's because in python 4 this

@attr.s(auto_attribs=True)
class A:
    b: B

is actually

@attr.s(auto_attribs=True)
class A:
    b: "B"

@kmike
Copy link

kmike commented Feb 11, 2020

Local scopes are very hard to remember. Do you need to pass around init methods? One should not be calling those anyway. Maybe your code can accept the B for these cases. (Full disclosure I have no idea what your code is doing)

@euresti some context: a library inspects signature of functions (__init__ method in this case, for classes), and does dependency injection based on type annotations.

So if we have __init__(self, a: A), we'd create an instance of A and pass it to __init__.

For this it needs to be able to retrieve typing information reliably, at runtime; this breaks for attr.s classes.

@euresti
Copy link
Contributor

euresti commented Apr 25, 2020

So there are 2 places where these values are stored. cls.__annotations__ of in field.type. If you plan to use get_type_hints then you could do something like this to "resolve" the ForwardRefs

def foo():
   @dataclass
   class Foo:
      x: "Optional[Foo]"

   print(Foo.__annotations__)
   Foo.__annotations__ = get_type_hints(Foo, localns=locals())

   return Foo

X = foo()
print(get_type_hints(X))

Otherwise you have to do something like this:

   hints = get_type_hints(cls, localns=locals())
   for field in attr.fields(cls):
      if field.name in hints:
         # field is a FrozenInstance. The only way to update it is using object.__setattr__
         object.__setattr__(field, "type", hints[field.name])

In the end we just decided the simplest solution was to enforce that all classes are created at module scope and then get_type_hints(cls) is guaranteed to work.

@euresti
Copy link
Contributor

euresti commented Apr 25, 2020

To clarify using your example:

import sys
from typing import get_type_hints
import attr

def hints(method, localns):
    return get_type_hints(method, sys.modules[method.__module__].__dict__, localns=localns)

def resolve():
    @attr.s(auto_attribs=True)
    class A:
        b: 'B'

    @attr.s(auto_attribs=True)
    class B:
        pass

    return hints(A.__init__, localns=locals())

print(resolve())

You could even use inspect.currentframe().f_back in your hints method to get the globals and locals at the call site.

@euresti
Copy link
Contributor

euresti commented Jul 13, 2020

I learned today that dataclasses fixed this issue (get_type_hints(C.__init__)) by passing the correct globals into the exec call.

python/cpython#9518

Basically on class creation they save the class module dict, globals = sys.modules[cls.__module__].__dict__ and then pass it in to exec. I don't know if that would work for attrs though.

@hynek
Copy link
Member

hynek commented Jul 14, 2020

I don't know if that would work for attrs though.

Is that a gut feeling or do you see blockers?

@euresti
Copy link
Contributor

euresti commented Jul 14, 2020

I haven't looked deep at the attrs code. I notice that dataclasses just uses exec whereas attrs uses compile + eval. Basically since the code is not identical I couldn't say that it would 100% work to do the same as dataclasses.

@hynek
Copy link
Member

hynek commented Jul 14, 2020

Hmhm. FWIW, we used to use compile/exec until…this happened #87 (comment).

@frenzymadness
Copy link
Contributor

It would be nice to move this forward because we are already in Python 3.10 alpha 5 and our possibilities to change something big are limited in beta releases.

@hynek
Copy link
Member

hynek commented Feb 17, 2021

It would be nice to move this forward because we are already in Python 3.10 alpha 5 and our possibilities to change something big are limited in beta releases.

Euresti is on it but wouldn’t be it just fine for you to disable the relevant tests on Python 3.10 for now?

@frenzymadness
Copy link
Contributor

Euresti is on it but wouldn’t be it just fine for you to disable the relevant tests on Python 3.10 for now?

We definitely can do that but this early testing of the next Python release gives us a possibility to find regressions or bad implementation in the Python itself so it's preferred to adapt to the new version early.

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 a pull request may close this issue.

7 participants