Add attr.resolve_types

[euresti]

This adds attr.resolve_types which can be used to resolve forward declarations in classes created using __annotations__

Fixes #265

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

• Added tests for changed code.
• New features have been added to our Hypothesis testing strategy.
• Updated documentation for changed code.
• Documentation in .rst files is written using semantic newlines.
• Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
• Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

[codecov]

Codecov Report

Merging #302 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #302   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         731    741   +10     
  Branches      152    154    +2     
=====================================
+ Hits          731    741   +10

Impacted Files	Coverage Δ
src/attr/__init__.py	100% <ø> (ø)	⬆️
src/attr/_make.py	100% <100%> (ø)	⬆️

---

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 051da68...10bea3a. Read the comment docs.

[euresti]

I'm not sure if I need to implement any Hypothesis things. Please advise.
Also feel free to kibbitz on the naming of the new function.
I'm also open to not caching whether we've called the value and just letting the caller decide whether to do so or not.

[hynek]

Code looks good on a first look but is this really what we should do or is that a hack around something else we should be doing? I’m honestly asking because the whole topic is utterly confusing to me 🤯 and the discussions that happened here didn’t really help. :)

[euresti]

Sure! So here's what I understand:

There are 2 ways to get the type annotations. cls.__annotations__ and typing.get_type_hints(cls)

Currently __annotations__ gives you the python object after the :. This may be one of:

• a concrete type, e.g. List[A]
• something with a forward ref inside it, e.g. List['A']
• a string. e.g 'List[A]'

In the future, it will just always give you a string, see https://www.python.org/dev/peps/pep-0563/.

typing.get_type_hints(cls) will take that jumble from __annotations__ and try to return only concrete types. However, in order for that to work it must be able to resolve all the names. If it can't find a single name then it will raise NameError and return nothing. This makes it something that should not be called automatically. Specially since in some cases (e.g. Imports that only exist in if TYPE_CHECKING: blocks) they may never be resolvable. (Also importing typing is slow?)

Now as far as attrs is concerned field.type might as well be an opaque object. attrs doesn't use it for anything so it's just some metadata attached to the field and most users won't care about it at all.

So I think attrs is fine to continue using __annotations__, even after PEP-563 is in force, with 2 exceptions:

1. In the case of auto_attrib=True you have to know if something is typing.ClassVar[...] Right now the code checks str(annot).startswith("typing.ClassVar") which fortunately handles all three annotation styles (typing.ClassVar[int], typing.ClassVar['int'], 'typing.ClassVar[int]'). Though it should probably also check .startswith("ClassVar") to handle `'ClassVar[int]'``. (I should probably file this.)

2. Users who want to resolve field.type to concrete types. This includes third party tools that maybe want to do runtime type checking (Like we use internally at my company). Or something like cattrs that needs types to structure and unstructure objects.

So for those in 2, this code would be useful.

And just to double check, @Tinche would this code help you at all?

[hynek]

Before I dive any deeper, how are data classes coping with this, @ericvsmith?

[ericvsmith]

It's an open issue: ericvsmith/dataclasses#92

The only place where dataclasses cares about the value of the annotation is to detect if it's a ClassVar or an InitVar. Other than that, if an annotation is a string or an actual type, I don't care. This is equivalent to point 1 above (#302 (comment)).

As far as point 2 is concerned, I don't think this is a dataclass or attrs issue. My suggestion is to ignore it and let the user deal with it, just as they will have to for normal classes.

In an earlier discussion about it (that of course I can't find), I think it was Guido who suggested that if we find a string attribute, just scan the string for ClassVar (and now InitVar, too). This might have been an in-person conversation, which might be why I can't find it. The question remains: is that good enough? What if someone actually has a local type or alias called ClassVar?

I'm deliberately avoiding importing typing because it is so expensive. Maybe PEP 560 will help in that regard, but I doubt it. If you look at the implementation (https://github.com/ericvsmith/dataclasses/blob/10fb487befe96e5e5c25952bcb609dbe6f7aaaa1/dataclasses.py#L384), you'll see the trick I use to only check for a ClassVar if typing has already been imported, thus avoiding the import myself. This is the thing that breaks with string annotations.

I'm sort of waiting for PEP 563 to be accepted or not before I do anything. If it's accepted, maybe there will be better tools for this.

Thanks for pointing me to this issue.

[hynek]

Thanks for your input! IOW: you just YOLO it for now and check a bunch of string.

As a fun aside, we have a gross ClassVar thingie too (for the same reason): https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L186 if it makes you feel better. :)

[ericvsmith]

For now I completely ignore the issue and never treat a string as a ClassVar. The YOLO string comparison (as ugly as it is) is a hack I haven't written yet.

Your hack does make me feel better! Whatever we end up doing, I'm hoping we can use the same approach.

[ambv]

@ericvsmith, PEP 563 is accepted, it's being implemented now, I'll finish this around Christmas time. The only thing left is really f-strings. Unparsing those is not super easy ;-)

@euresti, I would much prefer if you didn't reimplement get_type_hints(). I don't want to deal with differences in resolution later and there's going to be differences when we're dealing with nested scopes, etc. You raise a fair point about the entire call to get_type_hints() failing if a single forward ref cannot be resolved. We should handle this by adding a custom error handler to the call (sorta kinda like errors='ignore' or 'surrogateescape', etc. in text decoding).

PEP 563 requires updates to get_type_hints(), too. So, if @hynek could wait a week or two for a late Christmas present with all this landed, I would appreciate that.

[hynek]

Would anyone feel bad if we pushed this to 18.1? I would like to get the fixes of 17.4 out and this seems to be still a bit in the air.

[ericvsmith]

@ambv: I'm not sure what you're trying to do with f-string parsing, but there might be some helpers in the compiler and ast that can help. I wouldn't mind exposing some of them, sort of like string.Formatter.parse() does for "".format() strings.

[euresti]

Delaying is fine, and I'd love to see what happens to get_type_hints.

[hynek]

How are we gonna proceed here? 😇

[euresti]

I'm actually unsure how useful this is. Perhaps this would serve better as separate project, one in which you want runtime types. Maybe part of cattrs or something new. I'm fine with closing this without merge.

[hynek]

I’m gonna have to rely on your, @chadrik and @ambv’s judgement. So voice y’all opinions, I really don’t know. :)

[ambv]

PEP 563 is implemented, typing.get_type_hints() resolves all types for runtime use. This is the only thing you should be using, otherwise strange things will happen if your users use from __future__ import annotations. If there's anything that you don't like about typing.get_type_hints(), make an issue on BPO and assign to me (lukasz.langa).

[euresti]

Based on all the new developments (mypy support, typing.get_type_hints() etc) I'm ok closing this unmerged. Basically all this code does is call get_type_hints and set the type arguments in the class attributes. Basically I don't think this should be a feature of attrs anymore.

[hynek]

Alright, thanks again for all the work you put into it!

[euresti]

Ok this is ready. The only part I'm not super happy about is that my tests pass globals() and locals(). This is only necessary because the classes are defined in local scope. If the classes were defined in module scope (which is probably how most classes are defined) you don't need to pass the arguments. I could move the class declaration outside but then I can't parametrize the slots flag. Also it means the class is defined somewhere else. Oh but this is pytest. I don't have to put the test in a class. Don't know how you feel about that.

[hynek]

Mostly docs & questions – code looks good to my untrained eye. :)

[hynek]

cough :D

[euresti]

What should it be?

[hynek]

20.1.0 – we're doing CalVer. The next version can always be found in the package's __init__.py (sans dev suffix).

[hynek]

Very random thought: it would totally be useful if it worked as a class decorator too, no?

Like in:

@attr.resolve_types
@attr.dataclass
class C:
   X: Foo

[euresti]

Interesting. That will work unless you've got a ForwardRef

e.g. This works

@attr.dataclass
class Foo:
   ...

@attr.resolve_types
@attr.dataclass
class C:
   X: Foo

But this won't:

@attr.resolve_types
@attr.dataclass
class C:
   X: Foo

@attr.dataclass
class Foo:
   ...

But not-too hard to implement.

[hynek]

I'll fix the version myself, thanks!