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

jsii requires the wrong type in some cases #3818

Closed
otaviomacedo opened this issue Nov 1, 2022 · 4 comments · Fixed by #3820
Closed

jsii requires the wrong type in some cases #3818

otaviomacedo opened this issue Nov 1, 2022 · 4 comments · Fixed by #3820
Assignees
Labels
bug This issue is a bug. p1

Comments

@otaviomacedo
Copy link
Contributor

otaviomacedo commented Nov 1, 2022

Describe the bug

jsii requires the wrong type in some cases. Examples:

aws/aws-cdk#22688
aws/aws-cdk#22689
aws/aws-cdk#22711

This behavior started with jsii 1.70.0.

Expected Behavior

Require the correct types.

Current Behavior

Example: in CDK 2.48.0+, typeguard in Python is requiring that the Protocol parameter of elasticloadbalancingv2.HealthCheck is of type ecs.Protocol, not elasticloadbalancingv2.Protocol as it was in CDK 2.47.0 and older.

Reproduction Steps

Use one of the CDK bugs posted above to reproduce the issue.

Possible Solution

No response

Additional Information/Context

No response

SDK version used

N/A

Environment details (OS name and version, etc.)

MacOS

@otaviomacedo otaviomacedo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 1, 2022
@RomainMuller RomainMuller added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Nov 2, 2022
@RomainMuller
Copy link
Contributor

Note
Affected users can disable runtime type-checking by running Python in optimized mode (via python3 -O)

@RomainMuller RomainMuller self-assigned this Nov 2, 2022
@RomainMuller
Copy link
Contributor

All parameters that have been reported as problematic are effectively forward declared, and it seems these get incorrectly resolved as part of the runtime (likely due to differences in globals, etc...).

It seems like what we need to do is one of the following (need to check what works):

  • Emit type references on the stub as "real" types (the stub is defined within a function, and all types in the file should exist before the surrounding function can be called, so forward references are not necessary)
  • Emit fully-qualified forward declarations
  • Move type-checking stubs out of the nesting context, so their type annotations are evaluated as part of the same environment as the surrounding declaration (which used to be correct)

@RomainMuller
Copy link
Contributor

RomainMuller commented Nov 2, 2022

Foregoing the forward declaration appears to fix the issue, so I'll give this a shot.

This also means this issue is a code-generator issue, and not a runtime issue, so broken libraries will only be fixed when generated with a fixed version of jsii-pacmak.

RomainMuller added a commit that referenced this issue Nov 2, 2022
Since dae724c, Python type-checking
relies on a nested stub function as a type annotations source. The
parameter signature of that stub was copied verbatim from the
surrounding function, including any forward type references.

However, the forward references can safely be replaced with regular type
references as the module is guaranteed to be fully loaded by the time
the stub is created, and using forward-references there results in
`typeguard` possibly evaluating those in a different context than the
one where the surrounding function was defined. The consequence of this
is that multiple different foward references by the same name may be
incorrectly treated as referring to the same type, despite coming from
different modules.

This is fixed by turning forward type references in the stub with
regular type references (in other words, removing any `"` from the
parameter signature of the stub), which lets the type be resolved from
the local definition context instead of the final runtime context in
which the function is called.

Fixes #3818
@mergify mergify bot closed this as completed in #3820 Nov 2, 2022
mergify bot pushed a commit that referenced this issue Nov 2, 2022
Since dae724c, Python type-checking
relies on a nested stub function as a type annotations source. The
parameter signature of that stub was copied verbatim from the
surrounding function, including any forward type references.

However, the forward references can safely be replaced with regular type
references as the module is guaranteed to be fully loaded by the time
the stub is created, and using forward-references there results in
`typeguard` possibly evaluating those in a different context than the
one where the surrounding function was defined. The consequence of this
is that multiple different foward references by the same name may be
incorrectly treated as referring to the same type, despite coming from
different modules.

This is fixed by turning forward type references in the stub with
regular type references (in other words, removing any `"` from the
parameter signature of the stub), which lets the type be resolved from
the local definition context instead of the final runtime context in
which the function is called.

Fixes #3818

---

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
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants