attrs icon indicating copy to clipboard operation
attrs copied to clipboard

pyright support via dataclass transforms

Open asford opened this issue 3 years ago • 25 comments

pyright is adding support for attrs classes in static type checking through a source-level extension mechanism called dataclass transforms. This will provide support the strict-subset of attrs class semantics implemented as standard-library dataclasses in pyright, and by extension pylance and VS Code.

Relevant project issues include:

https://github.com/microsoft/pyright/discussions/1782 - Discussion thread for feature https://github.com/microsoft/pylance-release/issues/226 - pylance tracking issue. https://github.com/microsoft/pyright/issues/146 - pyright ur-issue. https://github.com/microsoft/pyright/pull/1773 - Initial PR https://github.com/microsoft/pyright/blob/master/specs/dataclass_transforms.md - Current spec in master

Would y'all be open to a PR implemented the recommendations in the linked specification? Perhaps there's a dialog to be had to ensure that the upcoming next-gen decorator semantics can be supported via this extension mechanism? (I don't have enough of the picture in mind to understand whether this is true.)

edit - Updating cross-refs I'd originally missed and rephrasing description. This connection had already been made!

asford avatar Apr 23 '21 21:04 asford

Yes, Eric was kind enough to reach out to me a few weeks ago and we discussed the topic.

Their integration doesn't cover all of our features but it seems good enough – at least good enough to not deliver false positives. NG APIs should work too.

I was hoping for feedback from @euresti (or any other typing gurus around here, really) before going to town on it. But maybe we can indeed squeeze it into 21.1 – at least provisionally.

hynek avatar Apr 24 '21 06:04 hynek

Also pulling in @Tinche who seems to have already played with it!

hynek avatar Apr 24 '21 09:04 hynek

Seems to work for me. The cost/benefit analysis seems in favor of adding this: it's very easy for us to do and IDE support is very useful, particularly if you're not quite ready to start using mypy.

Tinche avatar Apr 24 '21 15:04 Tinche

I've also verified this on an internal codebase and found it to be generally functional.

Pleasantly, this allows us to implement attrs excellent advice on wrapping the decorator in a reasonably straightforward fashion, however as you mention is is a subset of both attrs' current semantics and the mypy-attrs plugin's current semantics.

I do feel that there's some value in having attrs begin to support this provisionally. It's a huge potential boost to VS Code users and could be a good way to drive more feedback on the pyright spec on additional "non dataclass" semantics like converter.

asford avatar Apr 24 '21 16:04 asford

This is interesting. Similar to an idea I had for supporting this in https://github.com/python/mypy/issues/5406#issuecomment-409233479

I wonder if this could be used in mypy itself. Might be complicated with all the caching mypy does, but ... maybe?

euresti avatar Apr 24 '21 16:04 euresti

How does it work with the overloads? Do you only annotate the real implementation? Can these be put in a .pyi file?

euresti avatar Apr 24 '21 16:04 euresti

@euresti did you get Eric’s e-mail? He sent it to your GitHub address. That would be the best way to get direct answers. :)

hynek avatar Apr 24 '21 19:04 hynek

How does it work with the overloads? Do you only annotate the real implementation? Can these be put in a .pyi file?

Quick link to a gist-playground as well for interested parties:

https://gist.github.com/asford/cd3c8ebbf245df6c3666eec0852c171d

asford avatar Apr 24 '21 19:04 asford

If someone wants to give it a shot, it would be great to squeeze this into the overdue 21.1 release that will come out next week come hell or high water. Seems like enough of y'all have played with it so I'd be delighted if y'all could cooperate on getting it done. :)

hynek avatar Apr 25 '21 10:04 hynek

I threw a very minimal PR together at #796 IFF you would like to fast-track support for this provisionally in 21.1.

asford avatar Apr 27 '21 20:04 asford

@jakebailey just pointed out something that I had missed previously. It appears that attr.ib and attr.field use the parameter name factory. Dataclasses (and the dataclass_transform spec) use the name default_factory. That mismatch means that the following example (which is used in the attrs documentation) generates errors in pyright:

from typing import List
import attr

@attr.s(auto_attribs=True)
class SomeClass:
    a_number: int = 42
    list_of_numbers: List[int] = attr.field(factory=list)

I can think of two solutions here:

  1. attr.field could be amended to accept default_factory rather than (or in addition to) factory
  2. The dataclass_transform spec could accept both default_factory and factory

I don't like option 2 because it would be an attrs-specific accommodation to the spec. What do you think about option 1? Can you think of any other options I'm missing?

erictraut avatar May 07 '21 00:05 erictraut

factory=list is syntactic sugar for default=Factory(list) that has been added after much complaining over the years. Making it long again would almost completely obliterate the point of its very existence.

So in this case it would be really nice if we could go with option 2 (if making it configurable is out of the question). I know that my code bases are full of the "sweet" alternative and I'd hate to change it to the long one.

hynek avatar May 07 '21 03:05 hynek

@erictraut is there anything we can do, to move the factory thing forward? 🐶 Still getting pinged about it and would like to have an answer – ideally a positive one. 🥺

hynek avatar May 18 '21 17:05 hynek

Would you consider adding support for the parameter name default_factory as a parameter to attr.field for consistency with dataclass? You could still support factory for backward compatibility and make it an error if both values are provided. Short of that, I don't see anything that you can do here other than advocating for more attrs-specific accommodations in the dataclass_transform spec. I'm reluctant to add attrs-specific accommodations, but it's an option to consider if there's sufficient demand.

I've been capturing all of the limitations in the spec as we discover them. After we've received feedback for a few months, I plan to go back and do a rev on the spec and the reference implementation.

erictraut avatar May 18 '21 17:05 erictraut

Supporting both in attrs is definitely an option, but there's a huge amount of code bases that use the shorter way (including mine).

The thing with "demand" is that I really don't want to send a mob your way. I saw it play out with the current pydantic/delayed type annotations kerfuffle and I'm still disgusted by how it played out. I guess the lack of power-mongering is a weakness of mine, but I would prefer a solution that's good for everyone.

What if attrs added default_factory and you added factory? 😇


As for your limitations: I think it's worth mentioning field_hooks.

hynek avatar May 18 '21 18:05 hynek

Keep in mind that the more attrs-specific accommodations that I add to the spec, the tougher it will be to standardize it in PEP form. I've already received a fair amount of pushback on the proposal in the typing-sig. If the goal is to standardize this (and I still think that's desirable), then I want to be cautious about adding more "warts" that will be difficult to justify to the typing-sig and python-sig reviewers.

On the other hand, if we decide that we're not going to try to standardize this, then it will remain a pyright-specific mechanism. In that event, we have more flexibility to extend it. However, it's also less likely that attrs users with large existing code bases (like you) will be affected because they presumably started with mypy and are unlikely to switch to another type checker.

If you're willing to add default_factory, we can see if that satisfies the needs of those who use attrs and pylance/pyright for their type checking.

When you say field_hook, are you referring to the field_transformer parameter? Is that something that affects type checking?

erictraut avatar May 18 '21 18:05 erictraut

A random user of both packages here: I'd prefer partial support released sooner to full support released later 🙏🏿

dimaqq avatar Sep 16 '21 04:09 dimaqq

I found that using private attributes. (i.e attributes that start with a _) causes pyright to miss them in the constructor.
Example

@attr.s(auto_attribs=True)
class Class:
  _priv: Int = None

A = Class(priv=1)  # pyright errors saying No parameter named "priv"
B = Class(_priv=1) # no pyright errors.  But is an actual error.

cazador481 avatar Dec 07 '21 15:12 cazador481

Private attributes are not a standard behavior in dataclass, and they're not supported in the "dataclass transform" mechanism. This is one of several known limitations. If you want to use attrs with pyright, you would need to avoid using the private attribute mechanism. Another option is to explicitly specify an alias using a field descriptor.

erictraut avatar Dec 07 '21 17:12 erictraut

Thank you for the link. I didn't dig deep enough in the attrs documentation it seems. Can you point me to an example of using the alias in the field descriptor? I am not finding any examples.

cazador481 avatar Dec 07 '21 17:12 cazador481

As a follow-up for those tracking this issued, support for factory was added to the dataclass transform spec in PEP681.

asford avatar Apr 02 '22 14:04 asford

Pyright doesn't support the validator decorator that attr.field supports. So I can't do this without getting type errors:

import attr

@attr.define
class Thing:
    x: int = attr.field()
    @x.validator  # error: member "validator" is unknown
    def _check_x(self, attribute, value):
        pass

EmmmaTech avatar Oct 09 '22 04:10 EmmmaTech

Are there any plans to add support for attrs converter functionality on the horizon?

supersergiy avatar Nov 10 '22 21:11 supersergiy

This is a random comment (well, it certainly relates to attrs + pyright, and particularly to converter which is why I found this issue) -- to me the most annoying thing I find as a newish user (to type annotations) is definitely not being able to properly separate between public API / user facing types and "internal types" -- and converter is sort of a symptom of the above not being possible. I.e. I use converter in the rare case where I want an internal type on an object to be different from the user-facing type, and where I don't want any user-facing promises at all about what the public type is, let alone want users to manually instantiate them. So ideally I want to be able to separately annotate the type of the generated __init__ parameter from the type of the instance attribute that gets populated by it. But I can imagine having terse syntax for such a thing is somewhere between "not existent yet", "doesn't exist even in theory" or "may not ever be useful enough to a wide audience" :).

Julian avatar Jan 26 '23 14:01 Julian

In a similar vein to @EmreTech's issue, fields that have the default value generated by a method also have pyright treat it as an instance of "str" and not the attributes offered by field():

import attr


@attr.define
class Thing:
    """Thing."""

    a_num: int = attr.field(default=3)
    b_num: int = attr.field()  # Pyright: Fields without default values cannot appear after fields with default values

    @b_num.default  # Pyright: Cannot access member "default" for type "int"   Member "default" is unknown
    def _b_default(self) -> int:
        """Defaults b_num as double of a_num."""
        return self.a_num * 2

This is how documentation suggests to use the default decorator, with the example provided causing the same message to be reported for attrs 23.2.0 and pyright 1.1.351.

Stealthii avatar Feb 23 '24 12:02 Stealthii