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

fix(jsii): some submodules are not exported from aws-cdk-lib #3491

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Apr 19, 2022

This bug is due to a crazy interaction between the publication
module, the __all__ array, and imports we might generate to do
type checking in a package that uses submodules.

Context

Here's what you need to know:

  • __all__ is normally used to define the symbols and submodules
    that should be imported when a consumer does from mymod import *.
  • publication is used to hide non-public symbols from library
    consumers. It will move all symbols that are NOT in __all__ to
    a _private submodule at runtime (it's overloading __all__ a
    little here, but this seems generally okay).
  • We generate imports at the top of the file for the modules that are
    referenced in a module, so that we can use them in type annotations.

Example

This is what the currently generated code for the aws_cdk module
(of CDK's v2 library) currently (roughly) looks like:

import publication
from .cloud_assembly_schema import AmiContextQuery as _AmiContextQuery_74bf4b1b

__all__ = ['Annotations', 'App', ..., 'ValidationResults']

publication.publish()

from . import aws_s3
from . import cloud_assembly_schema
...

Given this setup, the following happens:

$ python3
Python 3.9.5 (default, Jun 27 2021, 10:29:41)
>>> import aws_cdk
>>> print(aws_cdk.aws_s3)
<module 'aws_cdk.aws_s3' from '.../aws_cdk/aws_s3/__init__.py'>
>>> print(aws_cdk.cloud_assembly_schema)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'aws_cdk' has no attribute 'cloud_assembly_schema'

Explanation

Why does this happen?

  • __all__ does not contain any submodules
  • When publication.publish() is called, all symbols in the calling
    namespace that are missing from __all__ are hidden.
  • Crucially, this does include cloud_assembly_schema (because we
    imported that submodule to import some symbols for type checking
    purposes) but it doesn't include modules that haven't been imported
    yet.
  • Most modules get imported after publication.publish() is called,
    and so are not subject to hiding anymore. This is why we never noticed
    this problem before.
  • Every module only gets imported once, so even though we generate a
    second from . import cloud_assembly_schema line underneath
    publication.publish(), at that point the line doesn't do anything
    anymore.

Solution

Potential solutions are:

  • Add submodules to __all__.
  • Remove the call to publication.publish().
  • Make a variant of publication that doesn't hide submodules.
  • Don't generate actual imports for the type annotations.

This PR picks the first solution, as it seems simplest. The only
downside would have been that import * would then start importing all
of the CDKv2 library, but that's happening already anyway because of all
the imports we generate ourselves.


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

This bug is due to a crazy interaction between the `publication`
module, the `__all__` array, and imports we might generate to do
type checking in a package that uses submodules.

Context
-------

Here's what you need to know:

- `__all__` is normally used to define the symbols and submodules
  that should be imported when a consumer does `from mymod import *`.
- `publication` is used to hide non-public symbols from library
  consumers. It will move all symbols that are NOT in `__all__` to
  a `_private` submodule at runtime (it's overloading `__all__` a
  little here, but this seems generally okay).
- We generate `imports` at the top of the file for the modules that are
  referenced in a module, so that we can use them in type annotations.

Example
-------

This is what the currently generated code for the `aws_cdk` module
(of CDK's v2 library) currently (roughly) looks like:

```python
import publication
from .cloud_assembly_schema import AmiContextQuery as _AmiContextQuery_74bf4b1b

__all__ = ['Annotations', 'App', ..., 'ValidationResults']

publication.publish()

from . import aws_s3
from . import cloud_assembly_schema
'...'
```

Given this setup, the following happens:

```
$ python3
Python 3.9.5 (default, Jun 27 2021, 10:29:41)
>>> import aws_cdk
>>> print(aws_cdk.aws_s3)
<module 'aws_cdk.aws_s3' from '.../aws_cdk/aws_s3/__init__.py'>
>>> print(aws_cdk.cloud_assembly_schema)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'aws_cdk' has no attribute 'cloud_assembly_schema'
```

Explanation
-----------

Why does this happen?

- `__all__` does not contain any submodules
- When `publication.publish()` is called, all symbols in the calling
  namespace that are missing from `__all__` are hidden.
- Crucially, this *does* include `cloud_assembly_schema` (because we
  imported that submodule to import some symbols for type checking
  purposes) but it doesn't include modules that haven't been imported
  yet.
- Most modules get imported *after* `publication.publish()` is called,
  and so are not subject to hiding anymore. This is why we never noticed
  this problem before.
- Every module only gets imported once, so even though we generate a
  second `from . import cloud_assembly_schema` line underneath
  `publication.publish()`, at that point the line doesn't do anything
  anymore.

Solution
--------

Potential solutions are:

- Add submodules to `__all__`.
- Remove the call to `publication.publish()`.
- Make a variant of `publication` that doesn't hide submodules.
- Don't generate actual imports for the type annotations.

This PR picks the first solution, as it seems simplest. The only
downside would have been that `import *` would then start importing all
of the CDKv2 library, but that's happening already anyway because of all
the `import`s we generate ourselves.
@rix0rrr rix0rrr requested a review from a team April 19, 2022 09:52
@rix0rrr rix0rrr self-assigned this Apr 19, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 19, 2022
@rix0rrr rix0rrr changed the title fix(jsii): submodules used for type checking are hidden fix(jsii): some submodules are not exported from aws-cdk-lib Apr 19, 2022
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks straightforward 👍

@rix0rrr rix0rrr merged commit 47f70a2 into main Apr 19, 2022
@rix0rrr rix0rrr deleted the huijbers/submodule-hidden branch April 19, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants