-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(python): support inheritance-based interface implementation #3350
base: main
Are you sure you want to change the base?
Conversation
The code generator used to represent jsii interfaces as python `Protocols`, however this mechanism is normally intended to allow structural typing in python. Python `Protocol` types are declared using a custom metaclass, which makes them tricky to inherit from without running into metaclass conflicts (especially in cases where multiple inheritance involving a `Protocol` and a non-`Protocol` base class). The jsii runtime does not perform structural typing, and hence interfaces are better modelled as abstract base classes that only declare abstract members. This, together with a minor modification to the `@jsii.interface` decorator, allows interfaces to be "implemented" by simply adding them to the implementor's inheritance chain, as is "natural" to do in Python in such cases. The "legacy" `@jsii.implements` approach to implementing interfaces is no longer recommended, however it is still supported as it is necessary when dealing with libraries generated by older versions of `jsii-pacmak`.
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
I am leaving this as a draft until I was able to validate that this does not have surprising consequences on a mixed codebase (i.e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much nicer!
...this.interfaces.map((i) => | ||
toTypeName(i).pythonType({ ...context, typeAnnotation: false }), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized this particular part of the change would break if the implemented interface is from a library that was generated with the typing_extensions.Protocol
metaclass.
Not putting this change in means the type checkers will not accept instances of classes where implementations of interfaces are expected (because typing_extensions.Protocol
is what causes structural typing to apply).
I need to find a strategy to be backwards-compatible here, either by conditionally extending interfaces IIF they are not typing_extensions.Protocol
types, or by making type-checkers understand the interface is implemented implicitly in all cases.
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
Okay looks like this is working at this stage. Mypy is not reporting any type checking errors anymore at this stage, and a cursory test appears to work (still got to make something a little more meaningful). |
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
added |
7c847b6
to
27c610d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this hasn't been reviewed in a while, it looks really good. I have only two questions
# Un-set __jsii_class__ as this is an interface, and not a class. | ||
iface.__jsii_class__ = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to not set this at all?
* | ||
* @returns the de-duplicated list of interface types. | ||
*/ | ||
private deduplicatedInterfaces( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice, good catch!
@@ -636,10 +638,7 @@ abstract class BaseMethod implements PythonBase { | |||
} | |||
|
|||
if (decorators.length > 0) { | |||
// "# type: ignore[misc]" needed because mypy does not know how to check decorated declarations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removed because of the PR that introduces pyright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears mypy is now able to check most of these.
Is it possible to pick this up again? The IDE warnings/errors of types not matching in Python is not delightful 😄 |
The code generator used to represent jsii interfaces as python
Protocols
, however this mechanism is normally intended to allowstructural typing in python. Python
Protocol
types are declared usinga custom metaclass, which makes them tricky to inherit from without
running into metaclass conflicts (especially in cases where multiple
inheritance involving a
Protocol
and a non-Protocol
base class).The jsii runtime does not perform structural typing, and hence
interfaces are better modelled as abstract base classes that only
declare abstract members.
This, together with a minor modification to the
@jsii.interface
decorator, allows interfaces to be "implemented" by simply adding them
to the implementor's inheritance chain, as is "natural" to do in Python
in such cases.
The "legacy"
@jsii.implements
approach to implementing interfaces isno longer recommended, however it is still supported as it is necessary
when dealing with libraries generated by older versions of
jsii-pacmak
. A Python warning will be emitted when the decorator isused with interfaces that should instead be inherited.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.