pyre-check icon indicating copy to clipboard operation
pyre-check copied to clipboard

Pyre is unable to correctly type JSII generated classes.

Open zhzhang opened this issue 4 years ago • 3 comments

Repo to help reproduce: https://github.com/zhzhang/jsii-example

I make the following interfaces in Typescript:

export interface Foo {
  readonly name: string;
}

export interface Bar {
  readonly foo: Foo;
}

I then compile these to python with npm run build && npm run package. This produces some build artifacts (.tar.gz, .whl) in the ./dist/python directory that can be pip installed.

The modules generated by JSII do have type hints, here's a snippet from the generated file:

class Bar:
    def __init__(self, *, foo: "Foo") -> None:
        """
        :param foo:
        """
        if isinstance(foo, dict):
            foo = Foo(**foo)
        self._values = {
            "foo": foo,
        }

    @builtins.property
    def foo(self) -> "Foo":
        return self._values.get("foo")

However, when I go to type check the included example.py, the types detected are

py/example.py:4:0 Revealed type [-1]: Revealed type for `foo` is `Foo`.
py/example.py:6:0 Revealed type [-1]: Revealed type for `bar` is `Bar`.
py/example.py:7:0 Revealed type [-1]: Revealed type for `bar.foo` is `typing.Any`.
py/example.py:8:0 Revealed type [-1]: Revealed type for `bar.foo.name` is `unknown`.

The output I'm expecting is

py/example.py:4:0 Revealed type [-1]: Revealed type for `foo` is `Foo`.
py/example.py:6:0 Revealed type [-1]: Revealed type for `bar` is `Bar`.
py/example.py:7:0 Revealed type [-1]: Revealed type for `bar.foo` is `Foo`.
py/example.py:8:0 Revealed type [-1]: Revealed type for `bar.foo.name` is `str`.

Curious why this is happening! I'm putting this as a pyre issues rather than a JSII issues since mypy seems to be able to pick up the correct types.

zhzhang avatar Apr 27 '21 21:04 zhzhang

Just a guess but maybe it's the builtins.property part; perhaps pyre only supports @property.

JelleZijlstra avatar Apr 27 '21 22:04 JelleZijlstra

Thanks for the report! Yes there are actually 2 issues here:

  • As @JelleZijlstra mentioned, Pyre was not really good when it comes to handling accesses to builtins modules. I can teach it to understand @builtins.property quickly but there's still some work involved in making sure it does not complain about missing attributes property in builtins module. Any chance you can just do @property instead?
  • Pyre does not attempt to do any inference on global variables and attributes (there are some exceptions in simple cases but the statement holds in general). You can see this issue with strict mode turned on (i.e. pyre --strict check). I'd recommend explicitly annotating the _values attribute here:
class Bar:
  def __init__(self, *, foo: Foo) -> None:
    ...
    self._values: Dict[str, Foo] = ...

grievejia avatar Apr 27 '21 23:04 grievejia

Unfortunately I cannot apply either of these fixes, even though I'd love to! These modules are automatically generated by JSII and I don't think editing them post generation is a good idea. Would this be something Pyre team is willing to fix? I can open an issue with JSII at the same time so that they might improve their generated code as well, though their hands are tied a little when it comes to which language features they can support since they support a ton of different target languages and want consistent behavior as far as is possible.

zhzhang avatar Apr 27 '21 23:04 zhzhang