-
Notifications
You must be signed in to change notification settings - Fork 260
feat(golang): namespace aware types #1893
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
feat(golang): namespace aware types #1893
Conversation
|
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
6038e1f to
a5c9801
Compare
SoManyHs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't reference lines bc the snapshot diff is too large, but I was seeing:
import (
"github.com/aws-cdk/jsii/jsii"
"child"
"child"
"child"
"jsiicalc"
)
so looks like something in the import generation is not quite right?
| let t: Enum | Interface | GoClass | undefined; | ||
| function buildModuleTypes(parent: Module, types: readonly Type[]): ModuleTypes { | ||
| return types.map( | ||
| (type: Type): ModuleType => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we use a switch statement on the expression type.kind here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with a switch on Type.Kind is that typescript can't infer the type of the Type.
public get types(): ModuleTypes {
return this.typeSpec.map(
(type: Type): ModuleType => {
switch (type.kind) {
case TypeKind.Class:
return new GoClass(this, type);
// ...
default:
throw new Error(
`Type: ${type.name} with kind ${type.kind} is not a supported type`,
);
}
},
);
}Throws a type error Argument of type 'Type' is not assignable to parameter of type 'ClassType'. The isXxxType() methods on the jsii-reflect.Type class are type discriminators so that's why they are used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err it's pretty much a discriminated union, I reckon this could be made to work... perhaps the issue is with how we modeled the class hierarchy here that prevents the compiler from making the correct type reduction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could make it work using return new GoClass(this, type as Class). I generally prefer not to use as if possible.
Fixes compiler errors on generated code with the following: 1. Return placeholder types from JSII class method calls 2. Prevent recursive structs by detecting types nested as properties within themselves and use a pointer instead 3. Map TS `any` types to an empty struct `GoAny` to be provided by the JSII runtime library 4. Fix `map` and slice type signatures in struct properties 5. Fix submodule struct and `package` names 6. Fix broken PascalCasing of return types in class methods when returning a named JSII type Also misc cleanup/enhancements: 1. Call a dummy NoOp method inside JSII class method calls provided by runtime 2. Add import statement for runtime library to modules 3. Consolidate import statements
46c5ca0 to
ccb1261
Compare
Add some doc comments and cleanup. Changes the `Module` interface to an abstract base class that is extended by `Package` and `Submodule` to avoid repeating module build and emit logic. Change dynamic getters for module submodules and types to be built and set in constructor to avoid unecessary recomputation.
ccb1261 to
0db0dbb
Compare
Minimizes changes to interface type for refactoring to class layout based on struct/interface differences.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| /* | ||
| * Format NPM package names as idiomatic Go module name | ||
| */ | ||
| export function goModuleName(name: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should we call this goPackageName?
| } | ||
|
|
||
| /* | ||
| * The module names of this modules dependencies. Used for import statements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: module's
| code.line(); | ||
| } | ||
|
|
||
| public get returnType(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noop: Just noting for posterity that we should include a comment here about how type resolution stuff has to happen outside the constructor.
Adds logic for emitting submodules and namespace aware types.
Creates a
Modulebase class that is extended by bothPackageandSubmodule.Modulerepresents a single.gosource code file that has apackage xxxdeclaration at the top. For the root file, this is the package name. For all submodules, this is the submodule name. All instances ofModulehave a reference to the root package so they are able to search the entire package for a type byfqn.All instances of
GoTypenow have a propertyparentthat is the instance ofModulethat the type is defined in. This is used to pass to any instances ofGoTypeRefthat the type may refer to.GoTypeRefcan resolve the corresponding instance ofGoTypeusing the types parentModuleas described above.The combination of these things means that starting at the root
Package, an AST is defined that mimics the structure of the.jsiiassembly but with all go specific emit logic. This makes it possible to access distant nodes in the tree which is what allows for namespace resolution of foreign type references.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.