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

Python code failing type checking #4531

Closed
rix0rrr opened this issue Jun 3, 2024 · 9 comments
Closed

Python code failing type checking #4531

rix0rrr opened this issue Jun 3, 2024 · 9 comments
Assignees
Labels
bug This issue is a bug. language/python Related to Python bindings p1

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 3, 2024

Reported in another issue

@dmartin-gh

I am also experiencing the interface issues described above. We're looking to adopt the CDK for our main platform and as a mostly Python/C++ shop the Python CDK seemed like a great choice for us. I didn't make it very far into my first sample app to test things out before mypy started throwing errors.

from aws_cdk.core import Construct, Stack
from aws_cdk import aws_ec2 as ec2

class MyEC2Stack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        ec2.Instance(self, 'my-app',
            instance_type=ec2.InstanceType('m5.4xlarge'),
            machine_image=ec2.MachineImage.generic_linux({
                'us-east-1': 'ami-0affd4508a5d2481b', # CentOS 7 (x86_64) - with Updates HVM
            }),
            vpc=ec2.Vpc(self, "my-app-vpc"))

image
I would love to see this fixed so we can start building out our application stack with confidence that we're passing the right level of constructs around!

Is this still a problem today? I have been trying to reproduce this (with CDK v2, though), and am not getting the MyPy error you report here... if you still happen to have the error, maybe this is because of your specific MyMy version or configuration (in which case, I would like to know what these are so I can reproduce).

Indeed it is still a problem. For example try loading up apigw-http-api-lambda-dynamodb-python-cdk in vscode, then pyright reports for

        # Create the Lambda function to receive the request
        api_hanlder = lambda_.Function(
            ...
        )

        ...

        # Create API Gateway
        apigw_.LambdaRestApi(
           ...
            handler=api_hanlder,  # Pyright error: Argument of type "Function" cannot be assigned to parameter "handler" of type "IFunction"
        )

typecheck error

Argument of type "Function" cannot be assigned to parameter "handler" of type "IFunction" in function "__init__"
  "Function" is incompatible with protocol "IFunction"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"
    "IFunction" is incompatible with "Function"Pylance[reportArgumentType](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportArgumentType)
(variable) api_hanlder: Function

as shown in this screenshot

image

That project uses aws-cdk-lib==2.77.0 however the same issue exists after updating to recent aws-cdk-lib==2.143.1. On my machine it happens with the following configuration

pyright 1.1.364
pylance 2024.5.103
aws-cdk-lib 2.143.1
python 3.11.2
vscode 1.89.1

Originally posted by @mariogalic in #1919 (comment)

@rix0rrr rix0rrr added p1 bug This issue is a bug. labels Jun 3, 2024
@polothy
Copy link
Contributor

polothy commented Jun 3, 2024

Also related to this issue:

I'm also still seeing this issue (#2877) in a JetBrains IDE (Intellij IDEA Ultimate with Python plugin). A comment in the issue near the end says it's fixed, but I see the same Expected type 'str | str | None', got '() -> str | str' instead error in my IDE today using CDK v2.144.0.

These two typing issues combined make Python experience really rough because your IDE thinks you have a lot of invalid types and sometimes its hard to see which typing issue is an actual problem unless you run cdk synth/deploy.

@mikewrighton mikewrighton self-assigned this Jun 3, 2024
@mikewrighton
Copy link
Contributor

I took a look at apigw-http-api-lambda-dynamodb-python-cdk and was able to root cause at least one issue causing these type checker errors.

When you implement an interface in TS, the method names have to match, but the method argument names do not, as long as the argument types are in the correct order. In Python however, when you implement a Protocol (which is how we do interfaces), the method argument names do have to match. So when our TS library has things like IPrincipal.addToPrincipalPolicy(statement: PolicyStatement) and an implementation of PrincipalBase.addToPrincipalPolicy(_statement: PolicyStatement) the former generates an IPrincipal Protocol in python, and the latter generates a class that's incompatible, and we get the PyRight error:

...
        "AnyPrincipal" is incompatible with protocol "IPrincipal"
          "IPrincipal" is incompatible with "AnyPrincipal"
          "IPrincipal" is incompatible with "AnyPrincipal"
          "IPrincipal" is incompatible with "AnyPrincipal"
          "add_to_principal_policy" is an incompatible type

The suggested fix here is to always use the method argument names in the base (Protocol) class, when generating subclasses. In the example above, PrincipalBase.addToPrincipalPolicy() would use the method argument statement instead of what's in the TS code, _statement. I didn't get as far as implementation but hopefully this context is useful.

@adriandelgg
Copy link

I have this issue too in Intellij IDEA Ultimate w/ Python. Super annoying. I don't get the error in VS Code.

@dytyniuk
Copy link

I used typing.cast(aws_iam.IPricipal, principal) as a muffler, but got fed up with it. So, upvote.

@adriandelgg
Copy link

adriandelgg commented Jul 12, 2024

I used typing.cast(aws_iam.IPricipal, principal) as a muffler, but got fed up with it. So, upvote.

Yeah, there shouldn't be a need to do that at all though. It seems to not throw the same type errors in VS Code, but it's definitely a bug with jsii due to how it transforms TypeScript code to other languages. Python is weird in general IMHO.

@gshpychka
Copy link

Possible duplicate of #2927 / #4541

@iliapolo iliapolo assigned iliapolo and unassigned mikewrighton Aug 20, 2024
@iliapolo iliapolo added the language/python Related to Python bindings label Sep 9, 2024
@iliapolo
Copy link
Contributor

iliapolo commented Sep 9, 2024

This issue has become hard to follow because it surfaces many different issues. I am closing it out as all issues have dedicated issues for them. See:

@iliapolo iliapolo closed this as completed Sep 9, 2024
Copy link
Contributor

github-actions bot commented Sep 9, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

@iliapolo iliapolo closed this as not planned Won't fix, can't repro, duplicate, stale Sep 9, 2024
Copy link
Contributor

github-actions bot commented Sep 9, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

dlpzx pushed a commit to data-dot-all/dataall that referenced this issue Oct 11, 2024
### Feature or Bugfix
<!-- please choose -->
- Bugfix

### Detail
- With latest bumps in dependency versions - `requirements.txt` had no
set `typeguard` version installed so installing latst `4.3.0` which
raised errors on `cdk synth` with the following:
- `typeguard.TypeCheckError: aws_cdk.aws_ec2.Vpc is not compatible with
the IVpc protocol because its 'add_client_vpn_endpoint' method has
mandatory keyword-only arguments in its declaration: cidr,
server_certificate_arn`
- This looks like a bug coming from `jsii` similar to some other open
issues like aws/jsii#4531 and the subsequent
issues linked to it

- Fixing `typeguard` to version `4.2.1` resolves the issue for the time
being


### Relates
- <URL or Ticket>

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
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. language/python Related to Python bindings p1
Projects
None yet
Development

No branches or pull requests

7 participants