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

Support ClassVar string annotations #361

Closed
euresti opened this issue Mar 20, 2018 · 9 comments
Closed

Support ClassVar string annotations #361

euresti opened this issue Mar 20, 2018 · 9 comments
Milestone

Comments

@euresti
Copy link
Contributor

euresti commented Mar 20, 2018

The following doesn't work in Python 3.7

from __future__ import annotations
import attr
from typing import ClassVar

@attr.dataclass
class A:
    x: ClassVar[int]

a = A()

Because ClassVar[int] will be 'ClassVar[int] in A.__annotations__ and so return str(annot).startswith("typing.ClassVar") returns False.

You can repro without 3.7 using this:

import attr
from typing import ClassVar

@attr.dataclass
class A:
    x: 'ClassVar[int]'

a = A()

Note: It's weird I'm not giving it a value but if I do then attrs will think it's a default value and the code won't error out.

Oh and the following works fine:

import attr
import typing

@attr.dataclass
class A:
    x: 'typing.ClassVar[int]'

a = A()
@hynek hynek added this to the 18.1 milestone Mar 20, 2018
@hynek
Copy link
Member

hynek commented Mar 20, 2018

Oh god more string comparisons. 🙈

@hynek
Copy link
Member

hynek commented Apr 9, 2018

This is the last blocker for the next release. Any ideas how to solve this save expanding the comparison to just ClassVar and call it a day?

@euresti
Copy link
Contributor Author

euresti commented Apr 9, 2018

I believe these are our only viable options:

  1. eval the first part (up to the first [) of every string annotation.
  • If it raises, move on, if you get something check if it's a ClassVar.
  • Pros: Catches every possible invocation, from typing import ClassVar as MyClassVar
  • Cons: I imagine this is prohibitively slow but it could be tested.
  1. Just check for .startswith('ClassVar') also and move on.
  • Pros: Fast.
  • Cons: This will miss as imports. And will also ignore situations in which you didn't import ClassVar correctly or at all. e.g.
@attr.s(auto_attrib=True)
class a:
    a: 'ClassVar'

(Though any good linter would catch that)

@hynek
Copy link
Member

hynek commented Apr 10, 2018

Can I haz a non-terrible third option? :'(

@ambv
Copy link
Contributor

ambv commented Apr 11, 2018

The non-gross option is to import typing and use get_type_hints(). I thought you're already doing that for resolving regular fields but apparently not.

How about this: if user code is using annotations and that annotation is a string, then we import typing and use get_type_hints() like grown-ups? In all likelihood if the code was already using annotations, it was importing typing somewhere anyway so there's no danger of slowing anybody down. Plus, since user code is already using annotations, we know it's Python 3 so typing is there?

Let me create a pull request to that effect.

@ambv
Copy link
Contributor

ambv commented Apr 11, 2018

OK, so I tried the above and it seems it's a nuclear option since it forces all annotations to be evaluated. This is what I wanted to avoid with from __future__ import annotations.

Making it more intelligent would be possible with typing.ForwardRef and its _evaluate() method but those are only available on 3.7+ (PEP 560).

I looked what dataclasses do in this case and they also don't work with from __future__ import annotations which I need to fix before Python 3.7 beta4.

So my pull request is going with Euresti's Option 2. This is going to be enough for 99.9% users.

ambv added a commit to ambv/attrs that referenced this issue Apr 11, 2018
Also updated the docstring to reflect why exactly we're doing what we're doing.
The previous comment was incorrect (`typing` is already imported in
applications using annotations).

I added `t.ClassVar` as well which is the only third alternative import I found
in use for typing-related classes.

Fixes python-attrs#361
@hynek
Copy link
Member

hynek commented Apr 11, 2018

I look forward to hear from the other 0.1% :D

But yeah, it’s the only realistic option I’m afraid.

@ambv
Copy link
Contributor

ambv commented Apr 11, 2018

Well, we can do the correct thing and slow things down for 99.9% while making that 0.1% not even notice.

Note that our avoidance of evaluation with from __future__ import annotations makes this (almost) as fast as not having annotations in the first place. So this is actually an improvement over 3.6 and older.

Admittedly, ClassVar is an ugly edge case.

@hynek
Copy link
Member

hynek commented Apr 11, 2018

I’m not arguing, just lamenting.

hynek pushed a commit that referenced this issue Apr 23, 2018
* Support `from typing import ClassVar`

Also updated the docstring to reflect why exactly we're doing what we're doing.
The previous comment was incorrect (`typing` is already imported in
applications using annotations).

I added `t.ClassVar` as well which is the only third alternative import I found
in use for typing-related classes.

Fixes #361

* Tests, docstrings, et al.
hynek added a commit that referenced this issue Jan 22, 2019
hynek added a commit that referenced this issue Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants