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

make property generic #5987

Closed
wants to merge 5 commits into from
Closed

make property generic #5987

wants to merge 5 commits into from

Conversation

twoertwein
Copy link
Contributor

Attempt at fixing #4731.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

Hmm, looks fairly disruptive. In practice type checkers end up special casing property to some extent; maybe this isn't something that typeshed should do.

stdlib/builtins.pyi Outdated Show resolved Hide resolved
stdlib/builtins.pyi Outdated Show resolved Hide resolved
twoertwein and others added 2 commits September 1, 2021 09:55
Co-authored-by: Akuli <akuviljanen17@gmail.com>
Co-authored-by: Akuli <akuviljanen17@gmail.com>
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

jax (https://github.com/google/jax.git)
+ jax/core.py:1090: error: Cannot infer type argument 1 of "property"  [misc]
+ jax/core.py:1091: error: Cannot infer type argument 1 of "property"  [misc]
- jax/experimental/maps.py:204: error: Invalid self argument "ResourceEnv" to attribute function "with_extra_loop" with type "Callable[[ResourceEnv, _Loop], Any]"  [misc]
- jax/experimental/maps.py:237: error: Invalid self argument "ResourceEnv" to attribute function "with_mesh" with type "Callable[[ResourceEnv, Mesh], Any]"  [misc]
+ jax/_src/api.py:2546: error: Cannot infer type argument 1 of "property"  [misc]
+ jax/_src/api.py:2547: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)
- jax/_src/numpy/lax_numpy.py:1916: error: Argument 1 to "_wraps" has incompatible type "Optional[Any]"; expected "Callable[..., Any]"  [arg-type]
- jax/experimental/host_callback.py:1705: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

optuna (https://github.com/optuna/optuna.git)
+ optuna/study/_multi_objective.py:20: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/study/_multi_objective.py:21: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:137: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:138: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:153: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:154: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:221: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:222: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:223: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:238: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:239: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/_pareto_front.py:240: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:149: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:150: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:156: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:157: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:221: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:222: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:223: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:230: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:231: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/visualization/matplotlib/_pareto_front.py:232: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/samplers/_nsga2.py:381: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/samplers/_nsga2.py:382: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/samplers/_nsga2.py:384: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/samplers/_nsga2.py:385: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/samplers/_nsga2.py:397: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/samplers/_nsga2.py:398: error: Value of type "Optional[List[float]]" is not indexable
+ optuna/integration/wandb.py:106: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/integration/wandb.py:108: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/integration/wandb.py:114: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/integration/wandb.py:119: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/integration/wandb.py:126: error: Argument 2 to "zip" has incompatible type "Optional[List[float]]"; expected "Iterable[float]"
+ optuna/integration/mlflow.py:164: error: Argument 1 to "_log_metrics" of "MLflowCallback" has incompatible type "Optional[List[float]]"; expected "List[Optional[float]]"
+ optuna/integration/botorch.py:453: error: Argument 1 to "len" has incompatible type "Optional[List[float]]"; expected "Sized"
+ optuna/integration/botorch.py:455: error: Argument 2 to "zip" has incompatible type "Optional[List[float]]"; expected "Iterable[float]"
+ tutorial/20_recipes/002_multi_objective.py:129: error: Value of type "Optional[List[float]]" is not indexable
+ tests/visualization_tests/test_contour.py:36: error: Value of type "Optional[List[float]]" is not indexable
+ tests/visualization_tests/test_contour.py:40: error: Value of type "Optional[List[float]]" is not indexable
+ tests/study_tests/test_study.py:916: error: Argument 1 to "tuple" has incompatible type "Optional[List[float]]"; expected "Iterable[float]"

scrapy (https://github.com/scrapy/scrapy.git)
+ scrapy/utils/httpobj.py:18: error: Argument 1 to "__get__" of "property" has incompatible type "Union[Request, Response]"; expected "Request"
+ scrapy/utils/httpobj.py:18: error: Argument 1 to "__get__" of "property" has incompatible type "Union[Request, Response]"; expected "Response"
+ scrapy/crawler.py:125: error: Cannot infer type argument 1 of "property"
+ scrapy/crawler.py:126: error: "object" has no attribute "_crawlers"

ibis (https://github.com/ibis-project/ibis.git)
+ ibis/expr/api.py:3485: error: Cannot infer type argument 1 of "property"

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/core/apply.py:199: error: Argument 1 to "__get__" of "property" has incompatible type "Union[Series, DataFrame, GroupBy[Any], SeriesGroupBy, DataFrameGroupBy, BaseWindow, Resampler]"; expected "_T"  [arg-type]
+ pandas/core/apply.py:199: error: Item "IndexOpsMixin" of "Union[_T, DataFrame, Any]" has no attribute "transform"  [union-attr]
+ pandas/core/apply.py:711: error: Argument 1 to "__get__" of "property" has incompatible type "Union[DataFrame, Series]"; expected "_T"  [arg-type]
+ pandas/core/apply.py:711: error: Incompatible types in assignment (expression has type "Union[DataFrame, _T, None]", variable has type "Union[DataFrame, Series, None]")  [assignment]
+ pandas/core/indexes/multi.py:1457: error: Argument "fset" to "property" has incompatible type "Callable[[MultiIndex, Any, DefaultNamedArg(Any, 'level'), DefaultNamedArg(bool, 'validate')], Any]"; expected "Optional[Callable[[Index, Any], None]]"  [arg-type]
+ pandas/core/indexes/multi.py:1458: error: Argument "fget" to "property" has incompatible type "Callable[[MultiIndex], FrozenList]"; expected "Optional[Callable[[Index], Any]]"  [arg-type]
+ pandas/plotting/_matplotlib/groupby.py:126: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

manticore (https://github.com/trailofbits/manticore.git)
+ manticore/native/cpu/abstractcpu.py:135: error: Cannot infer type argument 1 of "property"
+ manticore/native/cpu/abstractcpu.py:135: error: "object" has no attribute "parent"
+ manticore/native/cpu/abstractcpu.py:136: error: Cannot infer type argument 1 of "property"
+ manticore/native/cpu/abstractcpu.py:136: error: "object" has no attribute "parent"
+ manticore/native/cpu/abstractcpu.py:137: error: Cannot infer type argument 1 of "property"
+ manticore/native/cpu/abstractcpu.py:137: error: "object" has no attribute "parent"
+ manticore/native/cpu/abstractcpu.py:138: error: Cannot infer type argument 1 of "property"
+ manticore/native/cpu/abstractcpu.py:138: error: "object" has no attribute "parent"
+ manticore/native/cpu/abstractcpu.py:139: error: Cannot infer type argument 1 of "property"
+ manticore/native/cpu/abstractcpu.py:139: error: "object" has no attribute "parent"

edgedb (https://github.com/edgedb/edgedb.git)
+ edb/common/term.py:465:12: error: Need type annotation for "bold"
+ edb/common/term.py:466:13: error: Need type annotation for "faint"
+ edb/common/term.py:467:14: error: Need type annotation for "italic"
+ edb/common/term.py:468:17: error: Need type annotation for "underline"
+ edb/common/term.py:470:13: error: Need type annotation for "blink"
+ edb/common/term.py:471:16: error: Need type annotation for "overline"
+ edb/common/term.py:473:15: error: Need type annotation for "reverse"
+ edb/server/daemon/pidfile.py:37:14: error: Cannot infer type argument 1 of "property"
+ edb/server/daemon/pidfile.py:37:36: error: "object" has no attribute "_locked"
+ edb/server/daemon/daemon.py:72:15: error: Cannot infer type argument 1 of "property"
+ edb/server/daemon/daemon.py:72:37: error: "object" has no attribute "_is_open"

bidict (https://github.com/jab/bidict.git)
+ bidict/_orderedbase.py:192: error: Incompatible types in assignment (expression has type "_Node", variable has type "_SentinelNode")
+ bidict/_orderedbase.py:210: error: Item "None" of "Optional[_Node]" has no attribute "nxt"
+ bidict/_orderedbase.py:211: error: Item "None" of "Optional[_Node]" has no attribute "prv"
+ bidict/_orderedbase.py:229: error: Item "None" of "Optional[_Node]" has no attribute "nxt"
+ bidict/_orderedbidict.py:75: error: Item "None" of "Optional[_Node]" has no attribute "nxt"
+ bidict/_orderedbidict.py:76: error: Item "None" of "Optional[_Node]" has no attribute "prv"
+ bidict/_orderedbidict.py:82: error: Item "None" of "Optional[_Node]" has no attribute "nxt"
+ bidict/_orderedbidict.py:87: error: Item "None" of "Optional[_Node]" has no attribute "prv"

@twoertwein
Copy link
Contributor Author

In practice type checkers end up special casing property to some extent; maybe this isn't something that typeshed should do.

If it is possible to properly annotate property, there are at least two potential benefits

  1. Codebases that inherit from property or write a property-like class know how to annotate it
  2. type checkers will hopefully not need to special case property anymore so that subclasses of property work well with the previously specialized property (that does currently not work well with mypy):
class Property(property):
    pass


class TestA:
    @property
    def x(self) -> int:
        return 1


class TestB(TestA):
    @Property
    def x(self) -> int:
        return 2

reveal_type(TestA.x)
reveal_type(TestB.x)
reveal_type(TestA().x)
reveal_type(TestB().x)

mypy

13: error: Signature of "x" incompatible with supertype "TestA"
17: note: Revealed type is "def (self: test.TestA) -> builtins.int"
18: note: Revealed type is "Any"
19: note: Revealed type is "builtins.int"
20: note: Revealed type is "Any"

pyright

17:13 - info: Type of "TestA.x" is "property"
18:13 - info: Type of "TestB.x" is "Property"
19:13 - info: Type of "TestA().x" is "int"
20:13 - info: Type of "TestB().x" is "int"

@jakebailey
Copy link
Contributor

This idea rings a bell; I think had talked to @erictraut at some point about using generics to eliminate property special casing (but, maybe I'm misremembering).

@erictraut
Copy link
Contributor

Type checkers already implement many special cases for properties. The pyright code base is unfortunately littered with these special cases. I'd love to get rid of them because they're difficult to maintain and often result in inconsistent behaviors and bugs.

Making property generic would go a long way toward eliminating many of these special cases, but it wouldn't eliminate all.

The big challenge with making property generic is that there's five pieces of information that need to be parameterized:

  1. Whether there's a getter
  2. The type that the getter returns
  3. Whether there's a setter
  4. The type that the setter accepts
  5. Whether there's a deleter

I don't see how this can be done in a general way with the current type system.

I also don't see how this change can be made with the existing property without generating a lot of new type errors in existing code bases.

If I could design properties from scratch, I would make the following simplifications:

  • Eliminate the idea of a deleter. No one uses them, and they simply add complexity.
  • Introduce a separate class for read-only properties versus writable properties. The setter method on a read-only property would return a writable property.

These simplifications would allow for two generic classes, the first for read-only properties and the second for writable properties.

from typing import Any, Callable, Generic, TypeVar

_TGet = TypeVar("_TGet")
_TSet = TypeVar("_TSet")

class property(Generic[_TGet]):
    fget: Callable[[Any], _TGet] | None

    def __init__(
        self,
        fget: Callable[[Any], _TGet] | None = ...,
        doc: str | None = ...,
    ) -> None:
        ...

    def setter(
        self, fset: Callable[[Any, _TSet], None]
    ) -> "writable_property[_TGet, _TSet]":
        ...

    def __get__(self, obj: object, type: type | None = ...) -> _TGet:
        ...

class writable_property(Generic[_TGet, _TSet]):
    fget: Callable[[Any], _TGet] | None
    fset: Callable[[Any, _TSet], None] | None

    def __init__(
        self,
        fget: Callable[[Any], _TGet] | None = ...,
        fset: Callable[[Any, _TSet], None] | None = ...,
        doc: str | None = ...,
    ) -> None:
        ...

    def __get__(self, obj: object, type: type | None = ...) -> _TGet:
        ...

    def __set__(self, obj: object, value: _TSet) -> None:
        ...

class Foo:
    @property
    def foo(self) -> int:
        ...

    @foo.setter
    def foo(self, value: int | None):
        ...

    reveal_type(foo)  # writable_property[int, int | None]

@twoertwein
Copy link
Contributor Author

I don't see how this can be done in a general way with the current type system.

That's unfortunate. Have there been proposals to extend the current typing system to be able to type property in future versions of python?

I assume the best solution meanwhile for projects is to avoid using property if it causes false positives due to the special casing and implement their own type-able properties.

@erictraut
Copy link
Contributor

Have there been proposals to extend the current typing system to be able to type property in future versions of python?

Not that I'm aware of. The current type checkers already work around the problem for all common use cases, so I don't think people are feeling much pain here. Do you have a use case where current type checkers are falling short?

@twoertwein
Copy link
Contributor Author

Do you have a use case where current type checkers are falling short?

Pandas uses a custom readonly property that also caches the wrapped value cache_readonly I was trying to type it since it is used in many places where it currently hides the wrapped type annotations. . I tried multiple attempts in this PR but all of them result in many mypy errors which seem due to the special handling of property.

If there is no perfect solution, adding many ignore statements is probably better than using a custom property.

@tyralla
Copy link
Contributor

tyralla commented Nov 4, 2021

@twoertwein: did you check if (most of) the differences reported by mypy primer make sense?

I am asking because I am currently trying to fix python/mypy#4125 in python/mypy#11349, which is relatively easy if one defines a property via the decorator syntax (@property). However, things are more complicated when using the assignment syntax (x = property(getter, ...). If I am correct, this is because mypy uses much special casing for the decorator syntax but relies on typeshed for the assignment syntax. Hence the different reveal results in the following example:

class A:
    @property
    def x(self) -> int:
        ...
    def get_y(self) -> int:
        ...
    y = property(get_y)

a: A
reveal_type(A.x)  # note: Revealed type is "def (self: A) -> builtins.int"
reveal_type(a.x)  # note: Revealed type is "builtins.int"
reveal_type(A.y)  # note: Revealed type is "Any"
reveal_type(a.y)  # note: Revealed type is "Any"

If I modify typeshed's property as you suggested, mypy reveals the following:

reveal_type(A.x)  # note: Revealed type is "def (self: A) -> builtins.int"
reveal_type(a.x)  # note: Revealed type is "builtins.int"
reveal_type(A.y)  # note: Revealed type is "builtins.property[A, builtins.int]"
reveal_type(a.y)  # note: Revealed type is ""builtins.int*"

So, maybe your attempt is a little disruptive for mypy at the moment, as @hauntsaninja suggested. However, eventually, it could help eliminate some of mypy's limitations without introducing too much additional special casing in its source code.

@JelleZijlstra
Copy link
Member

Going to close this as stale for now to keep the list of open PRs manageable. If you're still interested in seeing this through, feel free to reopen or open a new PR.

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

Successfully merging this pull request may close these issues.

7 participants