feat(pacmak): allow `typeguard==3.x` and `typeguard==4.x` #4611

iliapolo · 2024-08-22T05:30:25Z

Currently python projects generated by pacmak define a typeguard dependency range of typeguard~=2.13.3, which prevents users from brining newer major version of typeguard into their projects.

This PR adds support for typeguard==3.x and typguard==4.x, which are the latest versions currently available. We intentionally do not allow an open range because every major version brings breaking changes with it that might need to be addressed.

Notes

We couldn't just drop support for typeguard==2.x because that would be a breaking change.
We could potentially release a new major version of pacmak to make the code more maintainable. My take is that this PR doesn't complicate the code to the extent of justifying a new major version, but lets discuss.

Fixes #4469

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

iliapolo · 2024-08-27T07:52:40Z

packages/@jsii/python-runtime/src/jsii/_runtime.py

@@ -168,6 +168,10 @@ def implements(*interfaces: Type[Any]) -> Callable[[T], T]:
    def deco(cls):
        cls.__jsii_type__ = getattr(cls, "__jsii_type__", None)
        cls.__jsii_ifaces__ = getattr(cls, "__jsii_ifaces__", []) + list(interfaces)
+        cls.__jsii_proxy_class__ = lambda: getattr(cls, "__jsii_proxy_class__", None)


This is required because __jsii_proxy_class__ is added to protocols, and therefore should be implemented in classes.

jsii/packages/jsii-pacmak/lib/targets/python.ts

Lines 1068 to 1070 in 9ecea56

code.line(

'# Adding a "__jsii_proxy_class__(): typing.Type" function to the interface',

);

For pacmak generated classes, __jsii_proxy_class__ is autoamtically added:

jsii/packages/jsii-pacmak/lib/targets/python.ts

Lines 1496 to 1498 in 9ecea56

code.line(

'# Adding a "__jsii_proxy_class__(): typing.Type" function to the abstract class',

);

But in order to support user defined protocol implementations, this needs to be added via the @jsii.implements decorator as well.

Note that prior to 4.x, typeguard didn't actually type check our protocols so this wasn't needed.

iliapolo · 2024-08-27T07:57:41Z

packages/@jsii/python-runtime/tests/test_python.py

@@ -27,17 +27,6 @@ def test_inheritance_maintained(self):

        assert base_names == ["DerivedStruct", "MyFirstStruct"]

-    def test_descriptive_error_when_passing_function(self):


Moved to test_runtime_checking.py, where all other type checking is asserted on.

iliapolo · 2024-08-27T08:30:23Z

packages/jsii-pacmak/lib/targets/python.ts

+    code.closeBlock();
+    code.openBlock('else');
+    code.line(
+      'if isinstance(value, jsii._reference_map.InterfaceDynamicProxy): # pyright: ignore [reportAttributeAccessIssue]',


Typeguard doesn't support type checking against dynamic proxies because it only compares against type declaration:

def check_protocol( value: Any, origin_type: Any, args: tuple[Any, ...], memo: TypeCheckMemo, ) -> None: subject: type[Any] = value if isclass(value) else type(value)

Prior to version 4.x, this wasn't needed because typeguard did not even attempt it. Its protocol checking logic was completely different:

def check_protocol(argname: str, value, expected_type): # TODO: implement proper compatibility checking and support non-runtime protocols if getattr(expected_type, '_is_runtime_protocol', False): if not isinstance(value, expected_type): raise TypeError('type of {} ({}) is not compatible with the {} protocol'. format(argname, type(value).__qualname__, expected_type.__qualname__))

This code never got executed on our protocols because _is_runtime_protocol defaults to False, and is only set to True if the protocol is decorated with @typing.runtime_checkable. Since our protocols aren't decorated with it, the protocol type checking never took place.

I considered adding the @typing.runtime_checkable decorator to gain protocol checking with versions 2.x and 3.x, but decided against it since i'm not sure what the implications would be and it might break existing projects.

So with support for 4.x we are now adding protocol checking, but still avoiding runtime proxies because they are not handled properly by typeguard.

iliapolo · 2024-08-27T08:32:57Z

packages/@jsii/python-runtime/package.json

@@ -28,9 +28,11 @@
    "build": "cp ../../../README.md . && rm -f jsii-*.whl && npm run generate && npm run deps && npm run lint",
    "lint": "ts-node build-tools/venv.ts black .",
    "package": "package-python && package-private",
-    "test": "npm run test:gen && npm run test:run && npm run test:types",
+    "test": "npm run test:gen && npm run test:run:typeguard-2 && npm run test:run:typeguard-3 && npm run test:run:typeguard-4 && npm run test:types",


We will now run three test suites, for each typeguard major version we support. These tests are pretty quick so compared to the current latency of PR validation, they don't add much.

iliapolo · 2024-08-27T08:42:13Z

packages/jsii-pacmak/lib/targets/python.ts

-    code.line('from typeguard import check_type');
+
+    code.line('import typeguard');
+    code.line('from importlib.metadata import version as _metadata_package_version');


This was added in Python 3.8, which is the lowest version of python we support - so its safe to use. Aliasing to avoid possible conflicts with other generated code.

iliapolo · 2024-08-27T08:43:19Z

packages/@jsii/python-runtime/tests/test_runtime_type_checking.py

+
+
+@jsii.implements(jsii_calc.IBellRinger)
+class PythonInvalidBellRinger:


Thanks for 4.x we can now statically typecheck protocols so added a test for it.

iliapolo · 2024-08-27T08:58:07Z

packages/@jsii/python-runtime/tests/test_runtime_type_checking.py

+
+
+@pytest.mark.skipif(TYPEGUARD_MAJOR_VERSION != 3, reason="requires typeguard 3.x")
+class TestRuntimeTypeCheckingTypeGuardV3:


So this is the primary maintenance burden we introduce with supporting multiple major versions. We have 3 classes, each testing a different major version, but containing 99% equal test cases. Note however that assertions are different because typeguard changed both error messages and exception types.

Given that typeguard only actively maintains a single version line (current 4.x) I think our most likely changes to this file are additional test cases that are only supported in 4.x, and therefore do not need backporting to the older test suites. If however we identify a coverage gap that applies to all major versions, we will have to add the test 3 times. I feel this possible overhead doesn't merit a new major version of pacmak, but interested in more thoughts here.

There are probably also ways to increase code sharing between the different 3 test classes that I didn't dive deeper into, but we could also investigate this route if you think its worth while.

iliapolo · 2024-08-27T09:00:04Z

packages/jsii-pacmak/lib/targets/python.ts

+    code.openBlock('else');
+    code.openBlock('if TYPEGUARD_MAJOR_VERSION == 3');
+    code.line(
+      'typeguard.config.collection_check_strategy = typeguard.CollectionCheckStrategy.ALL_ITEMS',


In version 3.x collection_check_strategy can only be configured globally, while in 4.x it can be specified as an argument to check_type.

mergify · 2024-08-27T14:04:08Z

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 · 2024-08-27T14:04:44Z

Merging (with squash)...

…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

increase range

bef73f6

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 22, 2024

iliapolo added 14 commits August 22, 2024 09:17

mid work

4701ee1

mid work

ade49c6

mid work

a0e88b0

mid work

413f5c1

mid work

9bb6969

mid work

b600103

mid work

d94b5b4

mid work

239b974

mid work

a10e213

Merge branch 'main' into epolon/upgrade-typeguard

d60622f

mid work

04c5fe7

mid work

cb65a51

mid work

6b849e8

mid work

5560950

iliapolo changed the title ~~feat: allow any major version of typeguard~~ feat(pacmak): allow typeguard==3.x and typeguard==4.x Aug 27, 2024

iliapolo commented Aug 27, 2024

View reviewed changes

mid work

59afc90

iliapolo commented Aug 27, 2024

View reviewed changes

iliapolo added 2 commits August 27, 2024 12:00

Merge branch 'main' into epolon/upgrade-typeguard

9f1f4cd

mid work

fec8d74

iliapolo marked this pull request as ready for review August 27, 2024 09:26

iliapolo mentioned this pull request Aug 27, 2024

feat: upgrade python runtime version of typeguard to >=3.0.2 #4470

Closed

2 tasks

rix0rrr approved these changes Aug 27, 2024

View reviewed changes

iliapolo added 2 commits August 27, 2024 14:38

mid work

d19f566

Merge branch 'main' into epolon/upgrade-typeguard

59edf5b

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Aug 27, 2024

mergify bot merged commit 32ae5b0 into main Aug 27, 2024
38 checks passed

mergify bot deleted the epolon/upgrade-typeguard branch August 27, 2024 14:04

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Aug 27, 2024

iliapolo mentioned this pull request Aug 29, 2024

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

Merged

iliapolo mentioned this pull request Sep 3, 2024

MyPy throwing type errors when it shouldn't #3931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pacmak): allow `typeguard==3.x` and `typeguard==4.x` #4611

feat(pacmak): allow `typeguard==3.x` and `typeguard==4.x` #4611

iliapolo commented Aug 22, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading

iliapolo Aug 27, 2024

iliapolo Aug 27, 2024

iliapolo Aug 27, 2024

iliapolo Aug 27, 2024

iliapolo Aug 27, 2024

mergify bot commented Aug 27, 2024

mergify bot commented Aug 27, 2024

	code.line(
	'# Adding a "__jsii_proxy_class__(): typing.Type" function to the interface',
	);

		@@ -27,17 +27,6 @@ def test_inheritance_maintained(self):

		assert base_names == ["DerivedStruct", "MyFirstStruct"]

		def test_descriptive_error_when_passing_function(self):



		@jsii.implements(jsii_calc.IBellRinger)
		class PythonInvalidBellRinger:



		@pytest.mark.skipif(TYPEGUARD_MAJOR_VERSION != 3, reason="requires typeguard 3.x")
		class TestRuntimeTypeCheckingTypeGuardV3:

feat(pacmak): allow typeguard==3.x and typeguard==4.x #4611

feat(pacmak): allow typeguard==3.x and typeguard==4.x #4611

Conversation

iliapolo commented Aug 22, 2024 • edited Loading

Notes

iliapolo Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

iliapolo Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

iliapolo Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

iliapolo Aug 27, 2024

Choose a reason for hiding this comment

iliapolo Aug 27, 2024

Choose a reason for hiding this comment

iliapolo Aug 27, 2024

Choose a reason for hiding this comment

iliapolo Aug 27, 2024

Choose a reason for hiding this comment

iliapolo Aug 27, 2024

Choose a reason for hiding this comment

mergify bot commented Aug 27, 2024

mergify bot commented Aug 27, 2024

feat(pacmak): allow `typeguard==3.x` and `typeguard==4.x` #4611

feat(pacmak): allow `typeguard==3.x` and `typeguard==4.x` #4611

iliapolo commented Aug 22, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading

iliapolo Aug 27, 2024 •

edited

Loading