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.

Sign up for GitHub

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

Jump to bottom

(Python): @jsii.python.classproperty not working as intended with type checking #3633

Closed
gshpychka opened this issue Jun 29, 2022 · 10 comments · Fixed by #3694
Closed

(Python): @jsii.python.classproperty not working as intended with type checking #3633

gshpychka opened this issue Jun 29, 2022 · 10 comments · Fixed by #3694
Assignees
@RomainMuller
Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort p1

Comments

@gshpychka
Copy link

gshpychka commented Jun 29, 2022 •
edited

Describe the bug

Static properties in the generated Python code are not correctly interpreted by the type checking system. They're recognized as class methods, and require casting.

Consider the following example snippet:

ec2.Vpc(self, "vpc", sugnet_configuration=ec2.Vpc.DEFAULT_SUBNETS)

Expected Behavior

pyright does not report any issues

Current Behavior

Type checker interprets the type of the property as a callable method

error: Argument of type "(cls: Vpc) -> List[SubnetConfiguration]" cannot be assigned to parameter "subnet_configuration" of type "Sequence[SubnetConfiguration] | None" in function "__init__"
    Type "(cls: Vpc) -> List[SubnetConfiguration]" cannot be assigned to type "Sequence[SubnetConfiguration] | None"
      "object" is incompatible with "Sequence[SubnetConfiguration]"
      Type cannot be assigned to type "None"

Reproduction Steps

Use any static property and enable type checking.

Possible Solution

No response

Additional Information/Context

No response

SDK version used

1.61.0

Environment details (OS name and version, etc.)

Linux
@gshpychka gshpychka added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 29, 2022
@peterwoodworth peterwoodworth added effort/medium Medium work item – a couple days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jul 21, 2022
RomainMuller added a commit that referenced this issue Aug 2, 2022
@RomainMuller
fix(python): classproperty not working with type checkers
4c81c76 
In some scenarios, the `@classproperty` decorator would not yield
something that python type checkers would consider as a property. This
attempts to add additional type hints to help type checkers discover
that the returned object is a property descriptor.

Fixes #3633
RomainMuller added a commit that referenced this issue Aug 2, 2022
@RomainMuller
fix(python): classproperty not working with type checkers
90b22dc 
In some scenarios, the `@classproperty` decorator would not yield
something that python type checkers would consider as a property. This
attempts to add additional type hints to help type checkers discover
that the returned object is a property descriptor.

Fixes #3633
@RomainMuller RomainMuller mentioned this issue Aug 2, 2022
Merged
@RomainMuller
Copy link
Contributor

Okay this is weird as I have definitely been validating that and it did not cause any issues with my setup, where it was considered all good (we actually run mypy on our test code, which includes references to static properties, too).

Would you be able to try and replace the jsii/python.py file in your install with the one in #3694, and tell me whether that fixes your type checking issue?

@RomainMuller RomainMuller self-assigned this Aug 2, 2022
@gshpychka
Copy link
Author

@RomainMuller assuming I did this right (replaced the file in my .venv/lib/python3.10/site-packages/jsii with the one from the PR and didn't do anything else), then no, it didn't change anything.

@RomainMuller
Copy link
Contributor

This feels like the type checker you're using might be incorrectly handling property descriptors... can you tell more about what the setup/tooling is?

@gshpychka
Copy link
Author

This is Pyright with default settings. Pyright version is 1.1.264 if that helps.

@RomainMuller
Copy link
Contributor

RomainMuller commented Aug 2, 2022 •
edited

Okay... I've tried running PyRight on the jsii runtime for Python... it definitely found some issues that mypy did not... I'm going to try to dig deeper... it appears to be pretty strict (and I mean that as a compliment) and our property descriptor might not satisfy it entirely...

@RomainMuller
Copy link
Contributor

I found the cause of the issue to be that the jsii/__init__.py does not correctly re-export jsii.python, and so the type-checker ends up assuming it's an Any...

@gshpychka
Copy link
Author

gshpychka commented Aug 2, 2022 •
edited

It'd be awesome if pyright were included in the test suite. It's the type checker used in Pylance, VS Code's Python language server.

@RomainMuller
Copy link
Contributor

@gshpychka yep... I'm actually adding pyright in addition to mypy... Maybe eventually I'll get rid of mypy (but I know some users are using it and I'd rather they not be surprised).

@RomainMuller
Copy link
Contributor

RomainMuller commented Aug 3, 2022 •
edited

I think I might have hit a bug in pyright 🙃 (microsoft/pyright#3781)

@mergify mergify bot closed this as completed in #3694 Aug 11, 2022
mergify bot pushed a commit that referenced this issue Aug 11, 2022
@RomainMuller
fix(python): classproperty not working with type checkers (#3694)
5413720 
The `jsii.python` submodule was not re-exported in `__init__.py`, which
made type resolves not manage to access the. `@classproperty` decorator
and default to treating it as `Any`.

This adds some missing type annotations, runs `pyright` on the runtime
library and tests thereof (as `mypy` did not identify this problem),
and adds the missing export declaration.

Fixes #3633



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RomainMuller RomainMuller

Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(python): classproperty not working with type checkers aws/jsii
3 participants
@RomainMuller @gshpychka @peterwoodworth