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

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

Open
gshpychka opened this issue Jul 15, 2021 · 10 comments · May be fixed by #4197
Open
Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort jsii language/python Related to Python bindings p1

Comments

@gshpychka
Copy link

gshpychka commented Jul 15, 2021

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.

#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 Report

@gshpychka gshpychka added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 15, 2021
@gshpychka gshpychka changed the title (@aws_cdk): Python: changing positional parameter names in interface implementations causes a typing error. (@aws_cdk): Python: incompatible interface implementations due to differing function argument names Jul 15, 2021
@gshpychka
Copy link
Author

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

@peterwoodworth peterwoodworth removed the needs-triage This issue or PR still needs to be triaged. label Sep 20, 2021
@NGL321 NGL321 added effort/medium Medium work item – a couple days of effort language/python Related to Python bindings p1 labels Jan 7, 2022
@NGL321
Copy link
Contributor

NGL321 commented Jan 7, 2022

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.

😸

@NGL321 NGL321 added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2022

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.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 9, 2022
@gshpychka
Copy link
Author

@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.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jan 10, 2022
@BwL1289
Copy link

BwL1289 commented Jan 10, 2023

Commenting for updates.

@BwL1289
Copy link

BwL1289 commented Jan 12, 2023

Thanks, I was on my phone.

@TheRealAmazonKendra
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

pahud commented Jun 11, 2024

transferring to jsii

@pahud pahud transferred this issue from aws/aws-cdk Jun 11, 2024
@iliapolo
Copy link
Contributor

iliapolo commented Sep 9, 2024

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 aws/jsii-compiler#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort jsii language/python Related to Python bindings p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants