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

Allow type checkers to infer trait types #788

Closed
wants to merge 6 commits into from

Conversation

vidartf
Copy link
Contributor

@vidartf vidartf commented Oct 20, 2022

We are currently encoding typing information for traits without integrating with the Python static typing system. This change is for allowing traitlets users to get static typing "for free" by using our trait types.

Ideally, the typing system would allow for default types, such that TraitType == TraitType[t.Any, t.Any], and TraitType[U] == TraitType[U, U], but for now we have to be verbose for this to work.

This is currently in a draft state, as some of the typings are still not accurately reflecting all the information we have. Potential improvements:

  • The Union type should correctly surface its types as the typings union of those of its components
  • Make the Optional part depend on allow_none (is it possible?).
  • Improve types of class based traits
  • Can we make the coercing variants not need to re-inherit TraitType ?
  • Make Enum and UseEnum types better
  • etc.

cc @maartenbreddels

vidartf and others added 4 commits October 20, 2022 15:02
Typings can still be improved, and more tests would be needed, but this demonstrates the concept.
@blink1073
Copy link
Contributor

Nice! You may want to play with the Application object in your mypy tests, to see if we can remove the explicit typings there.

@maartenbreddels
Copy link
Contributor

maartenbreddels commented Oct 21, 2022

I've been looking into if we can make a distinction between the instance and class access:

class IntField:
    @typing.overload
    def __get__(self, instance: None=None, owner:object=...) -> "IntField":
        ...

    @typing.overload
    def __get__(self, instance: object, owner:object=...) -> int:
        ...

    def __get__(self, instance:typing.Any=None, owner:object=None) -> typing.Union["IntField", int]:
        if instance is None:
            return self
        print(instance, owner)
        return 42

    def __set__(self, instance:object, value: int) -> None:
        print("setting", value)

    def lala(self):
        print("lala")

    def on_value(self, handler: typing.Callable[[int], None]) -> None:
        pass

class Lala:
    x = IntField()

Lala.x.lala()
lala = Lala()
print(lala.x)
# lala.x = "dsa"  # gives type error :+1+
lala.x = 42

This seems to work with traitlets as well:

diff --git a/traitlets/traitlets.py b/traitlets/traitlets.py
index 0d2649e..2259289 100644
--- a/traitlets/traitlets.py
+++ b/traitlets/traitlets.py
@@ -677,7 +677,15 @@ class TraitType(BaseDescriptor, t.Generic[G, S]):
         else:
             return value  # type: ignore
 
-    def __get__(self, obj: "HasTraits", cls: t.Any = None) -> t.Optional[G]:
+    @t.overload
+    def __get__(self, obj: None=None, cls:t.Any=...) -> "TraitType[G, S]":
+        ...
+
+    @t.overload
+    def __get__(self, obj: "HasTraits", cls:t.Any=...) -> G:
+        ...
+
+    def __get__(self, obj: t.Any=None, cls: t.Any=None) -> t.Union[G, "TraitType[G, S]"]:
         """Get the value of the trait by self.name for the instance.
 
         Default values are instantiated when :meth:`HasTraits.__new__`
@@ -688,7 +696,7 @@ class TraitType(BaseDescriptor, t.Generic[G, S]):
         if obj is None:
             return self
         else:
-            return self.get(obj, cls)
+            return t.cast(G, self.get(obj, cls))
 
     def set(self, obj, value):
         new_value = self._validate(obj, value)

@vidartf
Copy link
Contributor Author

vidartf commented Oct 25, 2022

@maartenbreddels I thought mypy already did that for class access (it isn't using __get__ type for class access, is it?).

@rmorshea
Copy link
Contributor

@vidartf, it does not. MyPy checks for polymorphic behavior on __get__ to determine class vs instance access return type. Here's a sample demonstrating this.

@maartenbreddels
Copy link
Contributor

it isn't using __get__ type for class access, is it?

it is, therefore the if obj is None: check

Thanks @rmorshea for the gist. Do you have an idea how to approach the allow_none -> Optional issue? Would that be possible with overload or should dataclass_transform be used for that?

@rmorshea
Copy link
Contributor

rmorshea commented Oct 25, 2022

This appears to work as expected. You can overload the __new__ method (just while type checking) to narrow the type. Unfortunately I don't see a clear way to avoid copy-pasting this to each TraitType because there's no way to generalize and add args to type variables (e.g. def __new__(cls: type[C], kind: T) -> C[T]: ...). Looks like there's a open issue for higher-kinded type variables.

@vidartf
Copy link
Contributor Author

vidartf commented Oct 27, 2022

Thanks for the input. I thought I had checked it, but I must have dreamed it. Now added a test for the typing implicitly via using .tag()

@@ -855,7 +857,7 @@ def set_metadata(self, key, value):
warn("Deprecated in traitlets 4.1, " + msg, DeprecationWarning, stacklevel=2)
self.metadata[key] = value

def tag(self, **metadata):
def tag(self, **metadata) -> "TraitType[G, S]":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with the typing_extension package we can do this:

Suggested change
def tag(self, **metadata) -> "TraitType[G, S]":
def tag(self, **metadata) -> typing_extension.Self":

@blink1073
Copy link
Contributor

The failures are due to davidfritzsche/pytest-mypy-testing#35

@maartenbreddels
Copy link
Contributor

This appears to work as expected. You can overload the __new__ method

Whow, thanks, you never stop learning with typing tricks! I think it's acceptable, the number of traits types isn't huge, and it will give us really good type checks with pylance and mypy!

@blink1073
Copy link
Contributor

We can pin to pytest<7.2 for now to unblock this PR.

@blink1073
Copy link
Contributor

I opened vidartf#1 exploring the idea @rmorshea had, with a bit of tweaking to get it to work with other arguments like help

@blink1073
Copy link
Contributor

It turns out we will still need the explicit types on the Application object to allow subclasses to set name = "foo" for example, which is not a huge problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants