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

Review User API Type Hints #794

Open
Archmonger opened this issue Jul 29, 2022 · 33 comments · Fixed by #827
Open

Review User API Type Hints #794

Archmonger opened this issue Jul 29, 2022 · 33 comments · Fixed by #827
Labels
priority-3-low May be resolved one any timeline. type-refactor About improving code structure.

Comments

@Archmonger
Copy link
Contributor

Archmonger commented Jul 29, 2022

Current Situation

ReactPy can generate type hinting errors generated (from Pylance).

For example, the fact that reactpy.component can ingest a ComponentType, but can only return a Component. Or the fact that use_state can be set to None initially and changed later.

Proposed Actions

Review user API type hints and fix where needed.

@Archmonger Archmonger added flag-good-first-issue A well defined and self-contained task. priority-3-low May be resolved one any timeline. type-refactor About improving code structure. labels Jul 29, 2022
@Archmonger Archmonger added this to the 2.0 milestone Jul 29, 2022
@rmorshea
Copy link
Collaborator

rmorshea commented Jul 30, 2022

I think idom.component is typed correctly. It's input function is a render function which may return ComponentType | VdomDict | None and its output is a constructor for a Component object. The distinction between Component and ComponentType is that the latter is a protocol, an abstract interface. Component on the other hand is a concrete implementation of ComponentType. ContextProvider is another example of a ComponentType.

For use_state, if its value is initially None but may also be some other type (e.g. a str) then you must explicitly declare this upfront.

initial_value: str | None = None
value, set_value = use_state(initial_value)

# Or cast initial value

from typing import cast
value, set_value = use_state(cast(str | None, None))

# Or declare state, set_state upfront

value: str | None
set_value: Callable[[str | None], None]
value, set_value = use_state(None)

Admittedly, while these solutions work, I think they all feel a bit awkward. Here are two alternatives:

# Use a named tuple
state: State[str | None] = use_state(None)
value, set_value = state
state.value
state.set_value

# Or do some weird typing magic to make this possible
value, set_value = use_state[str | None](None)

I haven't seen the latter pattern used very many places so it might surprise people, but it looks nice.

@rmorshea
Copy link
Collaborator

On thinking further, use_state is complicated by the fact that Python 3.9 broke generic NamedTuples which we'd need to get this working. The best we'd be able to do is a generic tuple till this is fixed in Python 3.11.

@rmorshea
Copy link
Collaborator

Mypy 0981 includes support for generic NamedTuples.

@rmorshea
Copy link
Collaborator

rmorshea commented Sep 27, 2022

Also, presently the @component hides the type annotations of the underlying rendering function. This is because @component adds the special key parameter that is not consumed by the rendering function. Typing this correctly is blocked on concatenating keyword-only arguments with ParamSpec

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 7, 2022

Currently getting type hint errors on the component decorator, if str() is directly returned within a component. Or if a component is directly returned within a component.

@rmorshea rmorshea mentioned this issue Oct 7, 2022
3 tasks
@Archmonger
Copy link
Contributor Author

VDOM constructor type hints currently don't like None.

@rmorshea
Copy link
Collaborator

rmorshea commented Oct 19, 2022

In the case a None child is passed?

@Archmonger
Copy link
Contributor Author

Yeah, for example a conditional to return a component or None within a div tag.

@Archmonger
Copy link
Contributor Author

It's also still very annoying that I can't directly return a component within a component without type hint errors.

@rmorshea
Copy link
Collaborator

a conditional to return a component or None within a div tag.

Can you share an example. I'm not quite sure I understand what you're describing.

I can't directly return a component within a component

Can you share the error you're getting? The type hint for the allowable return type for component functions should allow for this.

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 19, 2022

Argument of type "None" cannot be assigned to parameter "attributes_and_children" of type "VdomAttributesAndChildren" in function "__call__"
  Type "None" cannot be assigned to type "VdomAttributesAndChildren"
    Type "None" cannot be assigned to type "Mapping[str, Any]"
    "__iter__" is not present
    "key" is not present
    "type" is not present
    "render" is not present
    "should_render" is not present
    Type "None" cannot be assigned to type "VdomDict"
  ...Pylance[reportGeneralTypeIssues](https://github.com/microsoft/pylance-release/blob/main/DIAGNOSTIC_SEVERITY_RULES.md#diagnostic-severity-rules)
@component
def component_1():
   return html.div(
      None
   )

Argument of type "(*args: Unknown, **kwargs: Unknown) -> (Component | Unknown)" cannot be assigned to parameter "function" of type "(...) -> (ComponentType | VdomDict | None)" in function "component"
  Type "(*args: Unknown, **kwargs: Unknown) -> (Component | Unknown)" cannot be assigned to type "(...) -> (ComponentType | VdomDict | None)"
    Function return type "Component | Unknown" is incompatible with type "ComponentType | VdomDict | None"
      Type "Component | Unknown" cannot be assigned to type "ComponentType | VdomDict | None"
        Type "Component" cannot be assigned to type "ComponentType | VdomDict | None"
          "Component" is incompatible with protocol "ComponentType"
          "Component" is incompatible with "VdomDict"
          Type cannot be assigned to type "None"Pylance[reportGeneralTypeIssues](https://github.com/microsoft/pylance-release/blob/main/DIAGNOSTIC_SEVERITY_RULES.md#diagnostic-severity-rules)
@component
def component_2():
   return html.div()

@component
def component_1():
   return component_2()

@rmorshea
Copy link
Collaborator

For the first case, it looks like the behavior of React is to simply exclude children which are None. Code changes will need to happen in two places:

For the second, I don't observe any problems when I lint with MyPy. What does your VSCode configuration look like? I can't seem to figure out how to reproduce the errors your seeing.

@Archmonger
Copy link
Contributor Author

Archmonger commented Oct 19, 2022

Yes those errors are pylance specific, using "basic" type checking.

I'll post the required vscode settings after work.

@rmorshea rmorshea mentioned this issue Oct 20, 2022
3 tasks
@Archmonger
Copy link
Contributor Author

In the vscode settings.json

{
    "python.analysis.typeCheckingMode": "basic",
    "python.languageServer": "Pylance",
}

@Archmonger
Copy link
Contributor Author

Did you ever replicate the issue with embedded component type hints?

@rmorshea
Copy link
Collaborator

rmorshea commented Nov 2, 2022

Whoops I linked to a comment here thinking it wouldn't close the issue.

@rmorshea rmorshea reopened this Nov 2, 2022
@Archmonger
Copy link
Contributor Author

Object of type ComponentType is not callable is a common type hint issue I get when trying to generically define a component.

For example

@dataclass
class Example():
    my_component: ComponentType

...

Example.my_component(params)

@rmorshea
Copy link
Collaborator

rmorshea commented Dec 1, 2022

Technically this is true. The component object itself is not actually callable. You should annotate this as Callable[... ComponentType]. We can create a new type alias for this since it seems generally useful. Maybe ComponentFunc?

@rmorshea
Copy link
Collaborator

rmorshea commented Dec 1, 2022

Actually there's already an annotation for this called ComponentConstructor

@Archmonger
Copy link
Contributor Author

There is a type hint mismatch causing some annoyance on my side

The @component decorator returns (...) -> Component while ComponentConstructor returns (...) -> ComponentType, making them incompatible in the eyes of Pylance.

@rmorshea
Copy link
Collaborator

rmorshea commented Dec 5, 2022

Component is a concrete implementation of the abstract ComponentType. MyPy seems able to handle this so probably an issue with Pyright.

@Archmonger
Copy link
Contributor Author

In order for PyRight to handle this, Component would need to inherit from ComponentType.

@rmorshea
Copy link
Collaborator

rmorshea commented Dec 5, 2022

Traditionally, subtyping is based on inheritance hierarchies, but ComponentType is a protocol which uses something called structural subtyping. That is, if A is a protocol, then B can be said to be a sub-type of A if B has all, compatibly annotated, attributes and methods of A.

@Archmonger
Copy link
Contributor Author

I think the generic tuple on use_state might be more frustrating to use than the original implementation.

@Archmonger
Copy link
Contributor Author

Yeah... Far too many annoying to fix type hint errors with the new use_state.

@rmorshea
Copy link
Collaborator

Can you elaborate on what those issues are?

@Archmonger
Copy link
Contributor Author

Archmonger commented Jan 27, 2023

Seems like the main MyPy issue is _Type is not correctly inferred for state and set_state.

Basically every usage of state/set_state result errors similar to
Argument 1 has incompatible type "bool"; expected "Union[_Type, Callable[[_Type], _Type]]"
or
Argument 2 to "Mutation" has incompatible type "_Type"; expected "bool"

You can see this in django_idom.hooks.

@rmorshea
Copy link
Collaborator

I've been noticing some weird behavior with my MyPy cache. Does the error persist if you clear the cache first?

@Archmonger
Copy link
Contributor Author

Clearing cache fixed the issues.

How strange... Maybe we can see if sqlite_cache resolves this issue?

@rmorshea
Copy link
Collaborator

Yeah, I have no idea what's going on. I've been meaning to try and write up a bug report.

@Archmonger
Copy link
Contributor Author

Archmonger commented Jan 30, 2023

The issue kept returning in my environment.

Changing sqlite_cache and cache_fine_grained did not resolve the issue.

However, it seems like changing incremental to false does resolve the issue.

@rmorshea
Copy link
Collaborator

That's good to know, I've been manually deleting the cache on every run.

@rmorshea rmorshea removed this from the Luxury milestone Feb 21, 2023
@rmorshea rmorshea removed the flag-good-first-issue A well defined and self-contained task. label Jun 12, 2023
@Archmonger
Copy link
Contributor Author

Archmonger commented Sep 25, 2023

Note to self:

The type hints around reactpy.html elements is too strict. Frankly, we may as well just accept Any for children and have the end-user handle incompatibilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low May be resolved one any timeline. type-refactor About improving code structure.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants