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

[red-knot] Descriptor protocol #15966

Open
Tracked by #14164
sharkdp opened this issue Feb 5, 2025 · 3 comments
Open
Tracked by #14164

[red-knot] Descriptor protocol #15966

sharkdp opened this issue Feb 5, 2025 · 3 comments
Labels
red-knot Multi-file analysis & type inference

Comments

@sharkdp
Copy link
Contributor

sharkdp commented Feb 5, 2025

We need to understand the descriptor protocol in order to infer proper types for attribute accesses like in this simple example:

from typing import Literal

class Ten:
    def __get__(self, instance: object, owner: type | None = None) -> Literal[10]:
        return 10

    def __set__(self, instance: object, value: Literal[10]) -> None:
        pass

class C:
    ten = Ten()

c = C()

# TODO: this should be `Literal[10]`
reveal_type(c.ten)  # revealed: Unknown | Ten

# TODO: This should `Literal[10]`
reveal_type(C.ten)  # revealed: Unknown | Ten

# These are fine:
c.ten = 10
C.ten = 10

# TODO: Both of these should be errors
c.ten = 11
C.ten = 11

References: Python Documentation: Descriptor Guide

part of: #14164

@sharkdp sharkdp changed the title Implement the descriptor protocol [red-knot] Descriptor protocol Feb 5, 2025
@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Feb 5, 2025
@carljm
Copy link
Contributor

carljm commented Feb 5, 2025

An important case to consider here that isn't shown in the example in summary is where the value argument to __set__ does not have the same type as the return type of __get__, meaning we need to use a different type for type-checking assignments to the attribute than we use for inferring the type of an attribute access.

@sharkdp
Copy link
Contributor Author

sharkdp commented Feb 5, 2025

where the value argument to __set__ does not have the same type as the return type of __get__

Yes, thanks. This is already covered in the provisional tests (#15972).

sharkdp added a commit that referenced this issue Feb 5, 2025
## Summary

This is a first step towards creating a test suite for
[descriptors](https://docs.python.org/3/howto/descriptor.html). It does
not (yet) aim to be exhaustive.

relevant ticket: #15966 

## Test Plan

Compared desired behavior with the runtime behavior and the behavior of
existing type checkers.

---------

Co-authored-by: Mike Perlov <[email protected]>
@mishamsk
Copy link
Contributor

mishamsk commented Feb 9, 2025

@carljm @sharkdp sorry, for delay

Here is my thinking, proposed plan & rough design.

Guiding principles I've used:

  • Design-wise: Avoid special-casing if possible
  • Plan such that each steps adds one new aspect of the protocol support, while addressing the next most popular usage scenario

Design

  • Descriptors should be modeled as Type::Instance, no special Type variant or anything fancy.
    • This rule should apply to everything. Including the most common @property, @classmethod, @staticmethod; attrbiutes of slotted classes and functions (methods)
  • @property, @classmethod, @staticmethod should produce an instance of KnownClass variant(s)
  • Introduce KnownMethod enum, starting with descriptor dunders that will provide
  • Decorators on methods should be processed at inference time and change the binding type to instance (unlike current implementation)
    • This would make it much easier to handle things, like writeable properties
    • Later this would allow supporting user provided functions/decorators that create descriptors
  • If class has slots, all bindings should become data-descriptors - instances of another new KnownClass variant

Obviously there are going to be a lot of minute details in actual implementation, but overall it feels like descriptors do not introduce any special needs really.

Plan

The plan, is to start with known classes and avoid bothering with full inference of descriptor dunder method signatures. Implement the non-data descriptors first, using the most popular builtins. Then expand to set only data descriptor and build-up support for known descriptor classes. Then tackle everything else.

Assuming red_knot is targeting "an average" python user, I'd suggest implementing in the following order based on frequency of use:

  1. Patterns common in non-library-like user code. So code devs write for a FastApi controller, not FastApi internals
    1. readonly property aka non-data descriptor
      1. Step 1: just understanding a member is a descriptor and properly handling in call_bound/member functions
      2. Step 2: diagnostic when non-data descriptor is LHS of assignment
    2. classmethod and staticmethod
      1. This would involve upgrading call_bound to handle classes (second argument in __get__). Otherwise just adding more known classes, should be easy
    3. property with setter
      1. this will be an inference "upgrade" - we can swtich from methods being function literals with decorators, to instances on this step
  2. Popular descriptor generating components from stdlib
    1. functools.cached_property (adding more known classes, should be very easy)
  3. Custom descriptor-generating classes in popular libraries
    1. ??? Not sure, needs research
  4. Less popular / library use-cases
    1. Slotted classes
    2. property (with delete)
    3. maybe functools.singledispatchmethod
  5. Generic inference for arbitrary classes that implement the protocol
    1. Implement full correct search logic in member/call_bound for instances (this), classes & super()
    2. Add __set_name__ support, handle overriden __getattribute__ etc.

PS. I can theoretically augment this with samples for each step, but this can also be done in individual issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

No branches or pull requests

3 participants