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

Failure to override an inherited field with a computed field #7250

Closed
1 task done
nilsso opened this issue Aug 25, 2023 · 7 comments · Fixed by #7346
Closed
1 task done

Failure to override an inherited field with a computed field #7250

nilsso opened this issue Aug 25, 2023 · 7 comments · Fixed by #7346
Assignees
Labels
bug V2 Bug related to Pydantic V2 good first issue

Comments

@nilsso
Copy link

nilsso commented Aug 25, 2023

Initial Checks

  • I confirm that I'm using Pydantic V2

Description

It seems that overriding a regular field with a computed field doesn't behave as expected. Instead of a value being returned by accessing the property the property object itself is returned. And as for @cached_property causes an exception within pydantic/_internal/_utils.py:smart_deepcopy.

Example Code

from pydantic import BaseModel, computed_field
from functools import cached_property

class Foo(BaseModel):
    name: str = "Foo"

class Bar(Foo):
    @computed_field
    @property
    def name(self) -> str:
        return "Bar"

print(Foo().name)  # Foo
print(Bar().name)  # <property object at 0x...>

# Upon definition:
# > TypeError: cannot pickle '_thread.RLock' object
class Baz(Foo):
    @computed_field
    @cached_property
    def name(self) -> str:
        return "Baz"

Edit: After some further playing around, removing the default name = "Foo" from Foo it's clear that name within Bar is in some kind of weird hybrid state.

class Foo(BaseModel):
    name: str

class Bar(Foo):
    @computed_field
    @property
    def name(self) -> str:
        print("from property")
        return "Bar"

# Pyright complains:
# 1. Pyright: Argument missing for parameter "name" [reportGeneralTypeIssues]
# But runs and I get the same result
print(Bar().name)  # <property object at 0x...>

# Prints name as inherited from Foo, and without the `"from property"`
print(Bar(name="Bar"))  # Bar

Python, Pydantic & OS Version

pydantic version: 2.1.1
        pydantic-core version: 2.4.0
          pydantic-core build: profile=release pgo=false mimalloc=true
                 install path: /Users/.../.pyenv/versions/3.11.1/lib/python3.11/site-packages/pydantic
               python version: 3.11.1 (main, Jan 14 2023, 16:44:31) [Clang 13.1.6 (clang-1316.0.21.2.5)]
                     platform: macOS-13.4-arm64-arm-64bit
     optional deps. installed: ['email-validator', 'typing-extensions']

Selected Assignee: @hramezani

@nilsso nilsso added bug V2 Bug related to Pydantic V2 unconfirmed Bug not yet confirmed as valid/applicable labels Aug 25, 2023
@samuelcolvin
Copy link
Member

samuelcolvin commented Sep 4, 2023

Looks like a bug. We should either fix it so in this example name is removed from fields, or raise an error.

BTW, there's no way to avoid the pyright complaint since that's just dataclass transforms doing their thing.

PR welcome to fix this.

@samuelcolvin samuelcolvin added good first issue and removed unconfirmed Bug not yet confirmed as valid/applicable labels Sep 4, 2023
@adriangb
Copy link
Member

adriangb commented Sep 5, 2023

Should this even be allowed? In general, overriding an attribute with a property is not allowed, e.g. because it cannot be assigned to. So even if we could allow this, I think it's technically incorrect to allow it. It wouldn't be the first time we do something "not theoretically correct" for the sake of usability, but we should maybe rethink if this is complex to implement as well. @sydney-runkle is going to look into what the implementation would look like.

@adriangb
Copy link
Member

adriangb commented Sep 5, 2023

Here's an example:

class Foo:
    name: str

class Bar(Foo):
    @property
    def name(self) -> str:
        raise NotImplementedError

MyPy says:

error: Cannot override writeable attribute with read-only property  [override]
Found 1 error in 1 file (checked 1 source file)

Pyright says:

error: "name" overrides symbol of same name in class "Foo"
    "property" is incompatible with "str" (reportIncompatibleVariableOverride)

Even within a Pydantic context:

from pydantic import BaseModel, computed_field


class Foo(BaseModel):
    name: str

class Bar(Foo):
    @computed_field
    @property
    def name(self) -> str:
        raise NotImplementedError


def f(a: Foo) -> Foo:
    a.name *= 2
    return a

f(Foo(name='a'))
f(Bar()) # TypeError: unsupported operand type(s) for *=: 'property' and 'int'

Even without the runtime error type checkers that use PEP 681 won't handle this override correctly (which was pointed out in the issue's first comment).

@sydney-runkle
Copy link
Member

sydney-runkle commented Sep 5, 2023

Going off of Samuel's original comment, I've opened the above ^^ PR to raise an error when a computed field overrides an inherited field.

Following up with @adriangb's inquiry, I don't think this behavior should be allowed in pydantic -- it's not allowed otherwise, as our type checkers have made clear.

@tobni
Copy link
Contributor

tobni commented Sep 9, 2023

Following up with @adriangb's inquiry, I don't think this behavior should be allowed in pydantic -- it's not allowed otherwise, as our type checkers have made clear.

Implementing a setter would have revealed that this is valid, at least according to mypy.

from dataclasses import dataclass

@dataclass
class A:
    b: int


class B(A):
    def __init__(self) -> None:
        self._b = 2

    @property
    def b(self) -> int:
        return self._b

    @b.setter
    def b(self, value: int) -> None:
        self._b = value

Type checks with mypy 1.5.1, and behaves like you'd expect. Same goes without @dataclass on A. Interestingly enough, annotating B as a dataclasses reveals another error, though.

... py:12: error: Dataclass attribute may only be overridden by another attribute  [misc]

@sydney-runkle
Copy link
Member

@adriangb, given the increased number of questions about this recently (I've seen 3 in the past week), I think it makes sense to update the docs with an example. Do you agree? I'd be happy to open a PR if so.

@adriangb
Copy link
Member

Yes docs updates are always a good path forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V2 Bug related to Pydantic V2 good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants