Skip to content

Commit

Permalink
fix(python): unable to implement additional interfaces (#2964)
Browse files Browse the repository at this point in the history
When a type extends a jsii class, any additional interfaces implemented
via the `@jsii.implements` annotation were not properly registered, and
overrides were not properly discovered.

This was the combination of a code-generation bug, which caused the type
passed to the `create` kernel method to be that of the jsii class,
instead of the dynamic class (`self.__class__`); and a kernel bug, which
caused those inheritance graphs to completely skip overrides detection.

Fixes #2963



---

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
  • Loading branch information
RomainMuller committed Aug 19, 2021
1 parent a9f0163 commit 4ced4d5
Show file tree
Hide file tree
Showing 6 changed files with 221 additions and 162 deletions.
27 changes: 21 additions & 6 deletions packages/@jsii/python-runtime/src/jsii/_kernel/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,34 @@ def _get_overides(klass: Type, obj: Any) -> List[Override]:
# We need to inspect each item in the MRO, until we get to our Type, at that
# point we'll bail, because those methods are not the overriden methods, but the
# "real" methods.
jsii_classes = [klass] + list(
jsii_name = getattr(klass, "__jsii_type__", "Object")
jsii_classes = [
next(
(
m
for m in type(obj).mro()
if getattr(m, "__jsii_declared_type__", None) == jsii_name
),
Object,
)
] + list(
itertools.chain.from_iterable(
(getattr(m, "__jsii_ifaces__", []) for m in type(obj).mro())
)
)
for mro_klass in type(obj).mro():
if (
mro_klass is klass
and getattr(mro_klass, "__jsii_type__", "Object") is not None
):
if getattr(mro_klass, "__jsii_declared_type__", None) is not None:
# There is a jsii declared type, so we reached a "well known" object,
# and nothing from now on is an override.
break
if mro_klass is Object:
if mro_klass is Object or mro_klass is object:
break

for name, item in mro_klass.__dict__.items():
# Ignore all "special" members (name starting with __)...
if name.startswith("__"):
continue

# We're only interested in things that also exist on the JSII class or
# interfaces, and which are themselves, jsii members.
for jsii_class in jsii_classes:
Expand All @@ -93,6 +106,7 @@ def _get_overides(klass: Type, obj: Any) -> List[Override]:
overrides.append(
Override(method=original.__jsii_name__, cookie=name)
)
break
elif inspect.isdatadescriptor(item) and hasattr(
getattr(original, "fget", None), "__jsii_name__"
):
Expand All @@ -105,6 +119,7 @@ def _get_overides(klass: Type, obj: Any) -> List[Override]:
overrides.append(
Override(property=original.fget.__jsii_name__, cookie=name)
)
break

return overrides

Expand Down
3 changes: 3 additions & 0 deletions packages/@jsii/python-runtime/src/jsii/_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ def __new__(
# will as well anyways.
if jsii_type is not None:
attrs["__jsii_type__"] = jsii_type
# The declared type should NOT be inherited by subclasses. This way we can identify whether
# an MRO entry corresponds to a possible overrides contributor or not.
attrs["__jsii_declared_type__"] = jsii_type

obj = super().__new__(cls, name, bases, attrs)

Expand Down
30 changes: 30 additions & 0 deletions packages/@jsii/python-runtime/tests/test_compliance.py
Original file line number Diff line number Diff line change
Expand Up @@ -1288,3 +1288,33 @@ def repeat(self, word: str) -> str:
entropy = MildEntropy(wall_clock)

assert now == entropy.increase()


def test_class_can_extend_and_implement_from_jsii():
"""
This test is identical to test_iso8601_does_not_deserialize_to_date, except
the WallCloc class extends ClassWithSelf (a well-known jsii type), to
demonstrate it is possible to both extend a jsii type, and implement a
supplemental interface at the same time.
See also https://github.com/aws/jsii/issues/2963
"""

@jsii.implements(IWallClock)
class WallClock(ClassWithSelf):
def __init__(self, now: str):
super().__init__(now)
self.now = now

def iso8601_now(self) -> str:
return self.now

class MildEntropy(Entropy):
def repeat(self, word: str) -> str:
return word

now = datetime.utcnow().isoformat() + "Z"
wall_clock = WallClock(now)
entropy = MildEntropy(wall_clock)

assert now == entropy.increase()
23 changes: 17 additions & 6 deletions packages/jsii-pacmak/lib/targets/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ abstract class BaseMethod implements PythonBase {
private readonly parameters: spec.Parameter[],
private readonly returns: spec.OptionalValue | undefined,
public readonly docs: spec.Docs | undefined,
public readonly isStatic: boolean,
opts: BaseMethodOpts,
) {
this.abstract = !!opts.abstract;
Expand Down Expand Up @@ -710,12 +711,17 @@ abstract class BaseMethod implements PythonBase {
if (this.parent === undefined) {
throw new Error('Parent not known.');
}
jsiiMethodParams.push(
toTypeName(this.parent).pythonType({
...context,
typeAnnotation: false,
}),
);
if (this.isStatic) {
jsiiMethodParams.push(
toTypeName(this.parent).pythonType({
...context,
typeAnnotation: false,
}),
);
} else {
// Using the dynamic class of `self`.
jsiiMethodParams.push(`${implicitParameter}.__class__`);
}
}
jsiiMethodParams.push(implicitParameter);
if (this.jsName !== undefined) {
Expand Down Expand Up @@ -2429,6 +2435,7 @@ class PythonGenerator extends Generator {
parameters,
undefined,
cls.initializer.docs,
false, // Never static
{ liftedProp: this.getliftedProp(cls.initializer), parent: cls },
),
);
Expand All @@ -2448,6 +2455,7 @@ class PythonGenerator extends Generator {
parameters,
method.returns,
method.docs,
true, // Always static
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand Down Expand Up @@ -2487,6 +2495,7 @@ class PythonGenerator extends Generator {
parameters,
method.returns,
method.docs,
!!method.static,
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand All @@ -2503,6 +2512,7 @@ class PythonGenerator extends Generator {
parameters,
method.returns,
method.docs,
!!method.static,
{
abstract: method.abstract,
liftedProp: this.getliftedProp(method),
Expand Down Expand Up @@ -2578,6 +2588,7 @@ class PythonGenerator extends Generator {
parameters,
method.returns,
method.docs,
!!method.static,
{ liftedProp: this.getliftedProp(method), parent: ifc },
),
);
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 4ced4d5

Please sign in to comment.