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

fix(python): user defined __jsii_proxy_class attributes are not preserved #4625

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

iliapolo
Copy link
Contributor

@iliapolo iliapolo commented Aug 29, 2024

In #4611, we added the _jsii_proxy_class__ attributes to the @jsii.interface implementations. This was required in order to comply with typeguard protocol checking. We didn't implement it correctly, accidentally overriding user defined proxy classes.

Note

I have been wrecking my brain trying to understand if this bug has any runtime implications, and I couldn't find any.

How so?

At runtime, from what I could gather, the __jsii_proxy_class__ attribute is only used when we try to instantiate a subclass of an abstract class:

if inspect.isabstract(klass):
klass = klass.__jsii_proxy_class__()
# Create our instance, bypassing __init__ by directly calling __new__, and
# then assign our reference to __jsii_ref__
inst = klass.__new__(klass)

However, for abstract classes, we assign an explicit value to __jsii_proxy_class__:

code.line(
'# Adding a "__jsii_proxy_class__(): typing.Type" function to the abstract class',
);
code.line(
`typing.cast(typing.Any, ${this.pythonName}).__jsii_proxy_class__ = lambda : ${this.proxyClassName}`,
);

Luckily, this happens AFTER the @jsii.implements decorator has finished, thus overriding the mistake in the decorator.
Presumably, this would still be a problem for user defined abstract classes (since they don't have this assignment). However, reference resolving for user defined classes is done via native reference lookup:

# First we need to check our reference map to see if we have any instance that
# already matches this reference.
try:
# TODO: Handle discovery of possible new interfaces on the ObjRef
return self._refs[ref.ref]
except KeyError:
pass

This is also why I couldn't come up with a real life test case, and had to resort to an artificial one.


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

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 29, 2024
@iliapolo iliapolo marked this pull request as ready for review August 29, 2024 05:44
Copy link
Contributor

mergify bot commented Aug 29, 2024

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Aug 29, 2024
Copy link
Contributor

mergify bot commented Aug 29, 2024

Merging (with squash)...

@mergify mergify bot merged commit 54fa1b2 into main Aug 29, 2024
43 checks passed
@mergify mergify bot deleted the epolon/python-preserve-jsii-proxy branch August 29, 2024 09:04
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Aug 29, 2024
mrgrain pushed a commit that referenced this pull request Aug 30, 2024
…served (#4625)

In #4611, we added the `_jsii_proxy_class__` attributes to the `@jsii.interface` implementations. This was required in order to comply with `typeguard` protocol checking. We didn't implement it correctly, accidentally overriding user defined proxy classes.

## Note

I have been wrecking my brain trying to understand if this bug has any runtime implications, and I couldn't find any.

#### How so?

At runtime, from what I could gather, the `__jsii_proxy_class__` attribute is only used when we try to instantiate a subclass of an abstract class:

https://github.com/aws/jsii/blob/dc77d6c7016bcb7531f6e374243410f969ea1fbf/packages/%40jsii/python-runtime/src/jsii/_reference_map.py#L65-L70

However, for abstract classes, we assign an explicit value to `__jsii_proxy_class__`:

https://github.com/aws/jsii/blob/dc77d6c7016bcb7531f6e374243410f969ea1fbf/packages/jsii-pacmak/lib/targets/python.ts#L1496-L1501

Luckily, this happens **AFTER** the `@jsii.implements` decorator has finished, thus overriding the mistake in the decorator.
Presumably, this would still be a problem for user defined abstract classes (since they don't have this assignment). However, reference resolving for user defined classes is done via native reference lookup:

https://github.com/aws/jsii/blob/dc77d6c7016bcb7531f6e374243410f969ea1fbf/packages/%40jsii/python-runtime/src/jsii/_reference_map.py#L48-L54

This is also why I couldn't come up with a real life test case, and had to resort to an artificial one. 

---

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants