(@aws_cdk): Python: incompatible interface implementations due to differing function argument names

[gshpychka]

In Python prior to 3.8 (and in all the jsii-generated Python code), all function arguments are keyword arguments. In the TS source code, a lot of interface implementations override their interface's functions with different parameter names, which makes them incompatible with the interface in Python.

First of all, it causes typing errors (I'm using pyright). More importantly, it makes these python classes incompatible with their interfaces. To add to this, each change in a positional parameter name on the TS side is a breaking change on the Python side.

For example, @aws_cdk.aws_lambda.Function's grantInvoke method uses grantee as its parameter name, whereas IFunction uses identity. These end up being incompatible in Python.

https://github.com/aws/aws-cdk/blob/b78a1bbf445743d96c8e4f54e7d2e7cac204342a/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L306 https://github.com/aws/aws-cdk/blob/b78a1bbf445743d96c8e4f54e7d2e7cac204342a/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L81

On the jsii side, it would have to add a check for all overrides to be compatible, i.e. to use the same parameter names.

On the CDK side, we'd have to deprecate all of the incompatible parameter overrides and add properly named ones.

https://github.com/aws/jsii/issues/1919 might be related - using Protocols instead of a Metaclass would ensure that this cannot happen.

Environment

• **Framework Version: 1.114
• **Language (Version): Python 3

This is :bug: Bug Report

[gshpychka]

@MrArnoldPalmer do you need any additional info regarding replicating the issue or the underlying causes?

[NGL321]

Hey @gshpychka,

It looks like a commit was made in JSII that might resolve this issue. Could you confirm this? If it did not resolve the issue we will continue to track this, otherwise we can close-out the related issues.

😸

[github-actions[bot]]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[gshpychka]

@NGL321 I don't think I have enough experience to test this, but from my understanding, that commit is still in a draft PR and hasn't been merged. If it's merged, CDK releases would fail, since they don't satisfy the constraints.

[BwL1289]

Commenting for updates.

[BwL1289]

Thanks, I was on my phone.

[TheRealAmazonKendra]

I'm typically closing jsii issues in this repo and tracking them in jsii instead, but this one looks like there's work to be done on both sides so I'm leaving this one open.

[anentropic]

FYI arg names in Protocol methods that start with double underscore won't cause name errors: https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols

seems that would allow implementations to name the arg however they want, though I guess it might complicate documentation and auto generating from the TS source (I am guessing)

the docs above sound like it's specific to "callback protocols" but the convention seems to be generally respected

e.g. changing this code in ISecret (rename arg to __key) got rid of my type error when using DatabaseSecret:

    @jsii.member(jsii_name="secretValueFromJson")
    def secret_value_from_json(self, __key: builtins.str) -> _SecretValue_3dd0ddae:
        '''Interpret the secret as a JSON object and return a field's value from it as a ``SecretValue``.

        :param key: -
        '''
        ...

[pahud]

transferring to jsii

[iliapolo]

For future folks picking this up, there are several approaches for of solving this.

On typescript side

Ideally, jsii libraries wouldn't have been allowed to create such inconsistencies. But now that they have, its impossible to change their source code because renaming positional arguments is a breaking change in python. So, solving this on the typescript side would only be possible by rolling out a new major version of the compiler and implementing https://github.com/aws/jsii-compiler/issues/1320. Affected libraries would then pick up the new compiler, detect and fix these mismatches, and roll out a new major version of their own library. This is very disruptive.

On jsii side

Enforce positional only

This implementation proposes we use a new Python 3.8 capability of marking arguments as positional only, and thus disallowing them to be invoked as keyword arguments. I'm not inclined to do this because many python users prefer invoking with keyword arguments always. It is also a breaking change so we won't be able to roll this out without a new major version.

Argument renaming

We could have pacmak generate argnames that match the interface. This would satisfy type-checking. As for runtime, we will apply a new @jsii.rename_kwargs decorator to populate the correct arguments and invoke the underlying function with them. See https://github.com/aws/jsii/tree/epolon/rename-kwargs for a starting point.

Note that this drops us down a rabbit hole I don't think we should get into, because it would require making the all arguments optional at the signature level, and adding custom code inside function implementations that validate required args. Meh.

Using double-underscore (__)

Props to @anentropic for finding this. I can confirm this indeed works for interface implementations, but i'm still unsure what will happen with abstract classes (do we even need to change those?).

I think this is what we should currently pursue and understand its implications deeper.

---

All in all, I think we should dive deeper into solving this on the jsii side and see if this can indeed be done.

• If no, work on the compiler issue so that existing libraries can stop proliferation of these mismatches, and have an option to enforce this with a new major version.
• If yes, do it and decide whether we'd like to pursue the compiler issue anyway so that it becomes the default behavior for upcoming major versions.

[nilroy]

@iliapolo Sorry to post it here. But looks like aws_cdk.eks.Cluster does not satisfy the aws_cdk.eks.ICluster interface according to pyright. I did not do any in-depth analysis but stumbled upon this issue and looks like they might have the same/similar issue.

os: debian bookworm
cdk -- 2.170.0 (build 060af6c)
Python 3.12.6
node - v22.11.0

[rpmcginty]

@iliapolo thank you for laying out the options in your sep 9 2024 post. Have yall made any decisions on this front? It seems that all indications point to any reasonable or "right" solution requiring a major version bump. Is there any major version release already scheduled on the horizon?