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

pyright support via dataclass transforms #795

Open
asford opened this issue Apr 23, 2021 · 27 comments
Open

pyright support via dataclass transforms #795

asford opened this issue Apr 23, 2021 · 27 comments
Labels
Feature Typing Typing/stub/Mypy/PyRight related bugs.

Comments

@asford
Copy link
Contributor

asford commented Apr 23, 2021

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:

microsoft/pyright#1782 - Discussion thread for feature
microsoft/pylance-release#226 - pylance tracking issue.
microsoft/pyright#146 - pyright ur-issue.
microsoft/pyright#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!

@hynek
Copy link
Member

hynek commented Apr 24, 2021

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
Copy link
Member

hynek commented Apr 24, 2021

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

@Tinche
Copy link
Member

Tinche commented Apr 24, 2021

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.

@asford
Copy link
Contributor Author

asford commented Apr 24, 2021

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.

@euresti
Copy link
Contributor

euresti commented Apr 24, 2021

This is interesting. Similar to an idea I had for supporting this in python/mypy#5406 (comment)

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

@euresti
Copy link
Contributor

euresti commented Apr 24, 2021

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

@hynek
Copy link
Member

hynek commented Apr 24, 2021

@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. :)

@asford
Copy link
Contributor Author

asford commented Apr 24, 2021

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

@hynek
Copy link
Member

hynek commented Apr 25, 2021

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. :)

@asford
Copy link
Contributor Author

asford commented Apr 27, 2021

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

@erictraut
Copy link
Contributor

@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?

@hynek
Copy link
Member

hynek commented May 7, 2021

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 hynek pinned this issue May 10, 2021
@hynek hynek changed the title pyright support via dataclass transforms? pyright support via dataclass transforms May 10, 2021
@hynek
Copy link
Member

hynek commented May 18, 2021

@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. 🥺

@erictraut
Copy link
Contributor

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.

@hynek
Copy link
Member

hynek commented May 18, 2021

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.

@erictraut
Copy link
Contributor

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?

@wsanchez wsanchez added Feature Typing Typing/stub/Mypy/PyRight related bugs. labels Aug 23, 2021
@dimaqq
Copy link

dimaqq commented Sep 16, 2021

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

@cazador481
Copy link

cazador481 commented Dec 7, 2021

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.

@erictraut
Copy link
Contributor

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.

@cazador481
Copy link

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.

@asford
Copy link
Contributor Author

asford commented Apr 2, 2022

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

@EmmmaTech
Copy link

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

@supersergiy
Copy link

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

@asford asford mentioned this issue Nov 13, 2022
21 tasks
@hynek hynek unpinned this issue Dec 22, 2022
@Julian
Copy link
Member

Julian commented Jan 26, 2023

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" :).

@Stealthii
Copy link

Stealthii commented Feb 23, 2024

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.

@Darkdragon84
Copy link

Darkdragon84 commented Nov 27, 2024

Not sure when this happened, but it seems that converter is supported by pyright now (I have attrs 24.2.0 and pyright 1.1.389). That is fantastic 🥳

However there is one quirk that screws up a larger project of mine, namely that type variable resolution is partially broken when using a converter:

from typing import Generic, TypeVar

from attrs import field, frozen
from typing_extensions import reveal_type

T = TypeVar("T")


@frozen
class Base(Generic[T]):
    data: set[T] = field()


@frozen
class BaseConverted(Generic[T]):
    data: set[T] = field(converter=set)


reveal_type(Base({1, 2}))  # Type of "Base(1, 2)" is "Base[int]"
reveal_type(Base({1, 2}).data)  # Type of "Base(1, 2).data" is "set[int]"
reveal_type(BaseConverted([1, 2]))  # Type of "BaseConverted([1, 2])" is "BaseConverted[int]"
reveal_type(BaseConverted([1, 2]).data)  # Type of "BaseConverted([1, 2]).data" is "set[T@BaseConverted]"

Interestingly, the type variable for the class itself is correctly resolved in both cases, however for the field it is not: it is still at set[T@BaseConverted] while it should be set[int]. Is this something that can be fixed on the attrs side or should I submit this issue with pyright?

EDIT: mypy actually does this right, but only if the converter is pulled out as a separate standalone function, following mypy's caveat section

@Darkdragon84
Copy link

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.

@erictraut I am actually one of those start-with-mypy-switch-to-pyright users. I got convinced a while ago by this official type checker comparison. Keep up the good work 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Typing Typing/stub/Mypy/PyRight related bugs.
Projects
None yet
Development

No branches or pull requests