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(rosetta): submodule-qualified names produce invalid Go, Java #3928

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented 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


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.

@RomainMuller RomainMuller requested a review from a team January 25, 2023 17:35
@RomainMuller RomainMuller self-assigned this Jan 25, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label 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
@MrArnoldPalmer MrArnoldPalmer added the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2023
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.

Added dnm to give @rix0rrr a chance to look here as my go to rosetta person. LGTM though 🚀

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Jan 30, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

Merging (with squash)...

@mergify mergify bot merged commit 1ff230d into main Jan 30, 2023
@mergify mergify bot deleted the rmuller/go-lost-namespace branch January 30, 2023 16:55
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jan 30, 2023
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.

Rosetta: Incorrect transliteration for namespace/submodule usage in Go
3 participants