pylance-release icon indicating copy to clipboard operation
pylance-release copied to clipboard

`dataclass_transform` incorrectly infer `__init__` kwargs

Open Conchylicultor opened this issue 1 year ago • 5 comments

Environment data

  • Language Server version: Pylance language server 2022.9.10 (pyright aba41864) starting
  • Python version (& distribution if applicable, e.g. Anaconda): 3.8

Code Snippet

Reproduction instruction:

import typing_extensions

@typing_extensions.dataclass_transform(kw_only_default=True)
class A:
  x: int

class B(A):
  y: int

B(y=1)  # pylance expect B(x=, y=), but should instead be `B(y=)`

Explaination: In the above snippet, B is a dataclass-like object (because it inherit from a dataclass_transform class). However A itself should NOT be a dataclass-like object.

Semantically, dataclass_transform should be similar to:

import dataclasses

class A:
  x: int

  def __init_subclass__(cls):
    dataclasses.dataclass(cls)


class B(A):
  y: int


B(y=1)  # A is not a `dataclass`, so `B.__init__` is `(y=)` NOT `(x=, y=)`

Context: https://github.com/google/flax/issues/2416#issuecomment-1239658713

Conchylicultor avatar Sep 08 '22 09:09 Conchylicultor

Our dataclass_transform support comes from Pyright, the type checker that Pylance is based on. This is a duplicate of https://github.com/microsoft/pyright/issues/3907, but we appreciate your thoughts on this issue.

debonte avatar Sep 08 '22 15:09 debonte

I don't think this is a duplicate of https://github.com/microsoft/pyright/issues/3907. The other bug is about whether __init__ method synthesis is skipped based on the presence of an explicit __init__ within the parent class.

In this issue, the __init__ method synthesis is assumed, but there's a question of whether it should include fields from a class that is decorated with @dataclass_transform directly.

When we first authored PEP 681, we said that @dataclass_transform was intended to be used in two contexts:

  1. To decorate a function that, itself, is designed to work as a class decorator and modifies the decorated class to give it dataclass-like properties. The stdlib dataclass is implemented as such a function.
  2. To decorate a class that is intended to be used as a metaclass that implements dataclass-like functionality for any class that is created by that metaclass. This is how pydantic's ModelBase class works.

We later added a third case, which is used in the code sample above:

  1. To decorate a base class that implements dataclass-like functionality for any class that derives from it.

This third case was added late in the review process, and I can see now that there were aspects that we didn't consider when spec'ing it or implementing it. We weren't clear in the spec about whether fields within the decorated base class would be treated as dataclass fields. Pyright's current implementation assumes that they are, which is why x and y are both assumed to be present in the synthesized __init__ method. @Conchylicultor, I can understand why you assumed that the fields within the base class should not be be included here. It's a reasonable assumption.

I think this is something we may need to go back and clarify in the PEP. Once clarified, we can make sure that all type checkers (including pyright) conform to the clarified spec.

erictraut avatar Sep 08 '22 16:09 erictraut

My thinking was that the core issue in both scenarios was where we believe the boundary is between library code (the implementation of the dataclass-like behavior) and user code (the consumer of that behavior).

debonte avatar Sep 08 '22 17:09 debonte

I am not entirely sure I'd expect B(y=) in place of B(x=, y=), if anything B(x=, y=) makes for sense in my current mental model.

That said, I am wondering if dataclass_transform admits programmatically modifying fields? In Flax, inside __init_subclass__ we programmatically add a couple of attributes by modifying .__annotation__, this is currently not detected by pylance. Is this one of:

  1. Not supported by PEP 681
  2. Supported but a current bug in pylance
  3. We are missing something to make pylance happy

cgarciae avatar Sep 14 '22 16:09 cgarciae

@cgarciae, it's option 1 -- not supported by PEP 681. There's currently no way to tell dataclass_transform that you'll be adding arbitrary extra attributes to the affected class.

debonte avatar Sep 14 '22 16:09 debonte

@Conchylicultor, reading this again it's now clear to me that this is different from https://github.com/microsoft/pyright/issues/3907, and also that the expected behavior you described above is clearly correct -- attributes on the class decorated with dataclass_transform (or that class's base/ancestor classes) should not be considered to be fields.

I will clarify this in PEP 681 and we'll get the behavior fixed in Pyright/Pylance as well.

debonte avatar Oct 05 '22 14:10 debonte

This has been fixed in Pyright. The fix will be available in the next prerelease build of Pylance.

debonte avatar Oct 06 '22 18:10 debonte

This issue has been fixed in prerelease version 2022.10.21, which we've just released. You can find the changelog here: CHANGELOG.md

heejaechang avatar Oct 12 '22 22:10 heejaechang

Awesome! Thank you!

Conchylicultor avatar Oct 13 '22 08:10 Conchylicultor