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

Rosetta: Incorrect transliteration for namespace/submodule usage in Go #3551

Closed
jsteinich opened this issue May 19, 2022 · 3 comments · Fixed by #3928
Closed

Rosetta: Incorrect transliteration for namespace/submodule usage in Go #3551

jsteinich opened this issue May 19, 2022 · 3 comments · Fixed by #3928
Assignees
Labels
bug This issue is a bug. module/jsii Issues affecting the `jsii` module. p1

Comments

@jsteinich
Copy link
Contributor

jsteinich commented May 19, 2022

Describe the bug

Translating a reference to a namespaced resource results in #error# being output in the resulting Go code.

Expected Behavior

The namespace should be maintained or separate import statements should be generated.

Current Behavior

The namespace is lost and #error# is generated instead.

Reproduction Steps

import * as aws from "./.gen/providers/aws";
const awsKmsKeyExamplekms = new aws.kms.KmsKey(this, "examplekms", {
  deletionWindowInDays: 7,
  description: "KMS key 1",
});

results in

import aws "github.com/aws-samples/dummy/genprovidersaws"
awsKmsKeyExamplekms := #error#.NewKmsKey(this, jsii.String("examplekms"), map[string]interface{}{
	"deletionWindowInDays": jsii.Number(7),
	"description": jsii.String("KMS key 1"),
})

Possible Solution

#3508 tackled a similar issue, so there maybe a similar solution available; however, I haven't checked the code.

Additional Information/Context

This came up while working on hashicorp/terraform-cdk#1782.
There is a snapshot test that illustrates the issue.

SDK version used

1.55.1, also observed in 1.58.0

Environment details (OS name and version, etc.)

docker.mirror.hashicorp.services/hashicorp/jsii-terraform (based on jsii/superchain)

@jsteinich jsteinich added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 19, 2022
@NGL321 NGL321 added module/jsii Issues affecting the `jsii` module. p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 3, 2022
@RomainMuller RomainMuller self-assigned this Jan 11, 2023
@RomainMuller
Copy link
Contributor

That's a tricky one... the issue here is that the original TypeScript traverses a namespace boundary here (aws.kms.KmsKey goes from the aws namespace into the kms namespace to get KmsKey), which in Go should translate to another import being generated (github.com/aws-samples/dummy/genprovidersaws/kms).

This is actually quite different from #3508 in the sense that we need to modify the list of go packages to import here, but we only "discover" this information after it's too late (we already emitted imports at that stage).

In order to address this, we'd need to either:

  • Change how the code is emitted so we can alter the imports list throughout code generation, and emit it last
  • Pre-process the AST to discover all nested namespaces we need to import at the very beginning

@RomainMuller
Copy link
Contributor

I'm exploring a possible solution. I expect it'll take me probably a few days to get through this (unless I discover I am overthinking it).

RomainMuller added a commit that referenced this issue Jan 25, 2023
Java and Go do not support transitive access to a submodule's items via
a qualified reference and instead require a dedicated import to be
emitted.

This adds code to analyze use of imported symbols to detect when a
namespace traversal occurs, to inject new imports & replace property
access expressions as necessary.

In order to facilitate this work, these extra elements were done:
- Replaced if-tree in the dispatcher with a switch statement, making the
  dispatch a lot faster, and also a lot nice to run through in a
  debugger
- Emit `var` instead of a type name for variable declarations with an
  initializer in C#, to reduce "invalid" code generated in cases where
  the type name needs some qualification that we are missing
- A couple of bug fixes here and there as the tests discovered them

Fixes #3551
RomainMuller added a commit that referenced this issue Jan 25, 2023
Java and Go do not support transitive access to a submodule's items via
a qualified reference and instead require a dedicated import to be
emitted.

This adds code to analyze use of imported symbols to detect when a
namespace traversal occurs, to inject new imports & replace property
access expressions as necessary.

In order to facilitate this work, these extra elements were done:
- Replaced if-tree in the dispatcher with a switch statement, making the
  dispatch a lot faster, and also a lot nice to run through in a
  debugger
- Emit `var` instead of a type name for variable declarations with an
  initializer in C#, to reduce "invalid" code generated in cases where
  the type name needs some qualification that we are missing
- A couple of bug fixes here and there as the tests discovered them

Fixes #3551
@mergify mergify bot closed this as completed in #3928 Jan 30, 2023
mergify bot pushed a commit that referenced this issue Jan 30, 2023
Java and Go do not support transitive access to a submodule's items via a qualified reference and instead require a dedicated import to be emitted.

This adds code to analyze use of imported symbols to detect when a namespace traversal occurs, to inject new imports & replace property access expressions as necessary.

In order to facilitate this work, these extra elements were done:
- Replaced if-tree in the dispatcher with a switch statement, making the dispatch a lot faster, and also a lot nice to run through in a debugger
- Emit `var` instead of a type name for variable declarations with an initializer in C#, to reduce "invalid" code generated in cases where the type name needs some qualification that we are missing
- A couple of bug fixes here and there as the tests discovered them

Fixes #3551

---

> **Note**:
> There are still some awkward issues with the rendered code, but those are significantly more difficult to tackle without profoundly refactoring `jsii-rosetta`... The current work is already chunky enough and I didn't want to make it worse.
> - Some synthetic type references lack qualification (e.g: struct type names in Go & C#)
> - There might still be edge cases where duplicated or unused imports are emitted
> - Import paths (in Go) may not be computed correctly (this might not be too hard to address, though)

---

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

⚠️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. module/jsii Issues affecting the `jsii` module. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants