Use DirectedCodegen #548

gosar · 2022-05-24T20:10:41Z

Migrating the code generator to use Smithy's new and recommended DirectedCodegen.

This PR is dependent on related change in smithy repo - smithy-lang/smithy#1237 and smithy-lang/smithy#1289 which haven't been released yet. Once they are released, this PR can be merged into main.

Testing by regenerating based on new code

Clients

yarn generate-clients && yarn test:protocols

AWS SDKs, Generic client (under /private/aws-echo-service) and Protocol tests (under /private) regenerates without any diffs.

SSDKs

yarn generate-clients -s && yarn test:server-protocols

Generated SSDK isn't committed to main, but see the differences based on a baseline that was committed to a branch just to enable this comparison pre/post DirectedCodegen - gosar/aws-sdk-js-v3@generated-ssdk-Jun28...gosar:aws-sdk-js-v3:generated-ssdk-Jun28-directed-codegen. The base for this is generated-ssdk-Jun28 branch which is based on smithy-1.21.0 and smithy-typescript main branch. And the diff is against generated-ssdk-Jun28-directed-codegen which is based on smithy/main (unreleased) and smithy-typescript/directed-codegen branch (this PR branch).
As seen in the link, there are 2 types of changes.

Some related to core differences in Smithy, like deprecating sets. I see yarn test:server-protocols is actually broken because of some changed. But this will have to be taken up separately. It does not affect the DirectedCodegen migration.
And also some code location is rearranged within a file. I think the differences are ok, especially for SSDK.

Misc

I made and committed some PRs independently already to reduce the diffs in this PR.
#541
#542
#544
#545
#547
#551
#559
#564

The code has a few TODOs and refactoring that I can be taken up separately. But at this point, the generator via DirectedCodegen is functionally equivalent to before.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

smithy-typescript-ssdk-libs/server-common/src/validation/validators.ts

...c/main/java/software/amazon/smithy/typescript/codegen/integration/TypeScriptIntegration.java

mtdowling · 2022-06-17T19:55:46Z

Looking good!

…ang#564)

gosar · 2022-06-23T21:36:16Z

...degen/src/main/java/software/amazon/smithy/typescript/codegen/DirectedTypeScriptCodegen.java

+        // Write each custom file.
+        for (TypeScriptIntegration integration : directive.context().integrations()) {
+            LOGGER.finer(() -> "Calling writeAdditionalFiles on " + integration.getClass().getCanonicalName());
+            integration.writeAdditionalFiles(
+                    directive.settings(),
+                    directive.model(),
+                    directive.symbolProvider(),
+                    directive.context().writerDelegator()::useFileWriter);
+        }


I think this loop here can be removed by moving integration.writeAdditionalFiles to integration.customize which CodegenDirector will call.

It will be awesome if we can do it in the same customization loop.

Yeah, I think this can be done as a refactoring later.

AllanZhengYP · 2022-06-27T16:29:36Z

...pescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/CodegenVisitor.java

-                shape.accept(this);
-            }
-        }
-        for (Shape shape : TopologicalIndex.of(model).getRecursiveShapes()) {


It seems the generated shapes are not topo sorted. It causes a lot of import statements in the models_0.ts from models_1.ts and vise versa. It will cause the TS compliler consumes more memory than it needs. Can you add the toposort to the new directed codegen visitor? We want models_N.ts file only has import statement to its previous files.

This will also make the generated diff a lot cleaner.

We want models_N.ts file only has import statement to its previous files.

Hmm, I didn't understand this mattered.

Shape generation already does do topo sort.

This reordering is because CodegenDirector does generation for Service before visiting shapes. I thought this rearrangement was not a big deal other than 1 time diff. But now I'm going to evaluate to confirm if it is good to switch the order of those steps in CodegenDirector for all generators - seems reasonable to me.

Changing the order in Smithy - smithy-lang/smithy#1289. Will regenerate SDKs once approved to confirm diffs are gone.

AllanZhengYP · 2022-06-27T16:43:06Z

...degen/src/main/java/software/amazon/smithy/typescript/codegen/DirectedTypeScriptCodegen.java

+        // TODO: do all of these parts below are before/after?
+        SymbolVisitor.writeModelIndex(directive.connectedShapes().values(), directive.symbolProvider(),
+                directive.fileManifest());


Is this a new customization interface to modify the model or settings? If so we need to run it before generation.

No, this is not a new customization. This is just what CodegenVisitor did at here to write the models/index.ts. So this seems in the correct place. I meant to remove that TODO but forget. Removing now.

AllanZhengYP · 2022-06-27T16:43:57Z

...degen/src/main/java/software/amazon/smithy/typescript/codegen/DirectedTypeScriptCodegen.java

+        // Write each custom file.
+        for (TypeScriptIntegration integration : directive.context().integrations()) {
+            LOGGER.finer(() -> "Calling writeAdditionalFiles on " + integration.getClass().getCanonicalName());
+            integration.writeAdditionalFiles(
+                    directive.settings(),
+                    directive.model(),
+                    directive.symbolProvider(),
+                    directive.context().writerDelegator()::useFileWriter);
+        }


It will be awesome if we can do it in the same customization loop.

…ithy-lang#565)

Doesn't generate everything though, but compiles and can run the generator

Generates static files, index.ts, etc. Fixes imports that were `../` instead of `./`

Based on smithy-lang#551

Instead of DeferredWriter passes the WriterDelegator along in GenerationContext so generateProtocolTests can create the file only if any tests need to be written. This is alternative to smithy-lang@09b9956 from smithy-lang#360. It is going back to using writerDelegator like before (and picking the fileName inside the test generation logic, instead of in DirectedTypeScriptCodegen.

The 2 SmithyBuildPlugins have duplication, which can be reduced via refactoring Ran `yarn generate-clients -s && yarn test:server-protocols` in aws-sdk-js-v3 with this commit. Compared the generated ssdks with https://github.com/gosar/aws-sdk-js-v3/tree/f47b48a1eb1187657f489b4e041f979e72e0134d and found there were differences but only some moving around of code within some files.

Given the plugins have moved to DirectedTypeScriptCodegen

The generator does not produce any .tsx files.

AllanZhengYP

Approve the rebase

The logic of `writeAdditionalFiles` was moved into `customize` as a follow up on the discussion here: smithy-lang#548 (comment)

gosar requested review from mtdowling, kstich and adamthom-amzn May 24, 2022 20:10

gosar force-pushed the directed-codegen branch from 2cd40af to 0ebc723 Compare May 24, 2022 22:38

gosar changed the title ~~Directed codegen~~ Use DirectedCodegen May 25, 2022

gosar force-pushed the directed-codegen branch from 0ebc723 to e16e80e Compare May 25, 2022 01:19

gosar mentioned this pull request May 25, 2022

chore(clients): regenerate based on smithy-typescript#548 aws/aws-sdk-js-v3#3634

Closed

gosar force-pushed the directed-codegen branch 2 times, most recently from 9b76732 to de838cc Compare May 27, 2022 23:22

gosar force-pushed the directed-codegen branch 5 times, most recently from 0c166f5 to 69280b6 Compare June 7, 2022 04:32

mtdowling reviewed Jun 17, 2022

View reviewed changes

smithy-typescript-ssdk-libs/server-common/src/validation/validators.ts Outdated Show resolved Hide resolved

mtdowling reviewed Jun 17, 2022

View reviewed changes

...c/main/java/software/amazon/smithy/typescript/codegen/integration/TypeScriptIntegration.java Outdated Show resolved Hide resolved

gosar force-pushed the directed-codegen branch 2 times, most recently from 5ec7130 to 21cd6f7 Compare June 18, 2022 08:59

gosar marked this pull request as ready for review June 18, 2022 09:05

gosar requested a review from a team as a code owner June 18, 2022 09:05

Remove usused code from ProtocolGenerator.GenerationContext (smithy-l…

584f7ed

…ang#564)

gosar commented Jun 23, 2022

View reviewed changes

trivikr approved these changes Jun 24, 2022

View reviewed changes

AllanZhengYP suggested changes Jun 27, 2022

View reviewed changes

gosar force-pushed the directed-codegen branch from 252e34b to a295b49 Compare June 27, 2022 23:05

gosar mentioned this pull request Jun 27, 2022

Generate shapes before generating Service smithy-lang/smithy#1289

Merged

fix: populate content-type header for all events (smithy-lang#567)

acf2ede

AllanZhengYP approved these changes Jun 30, 2022

View reviewed changes

fallback to status code for unmodeled error, cleanup error parser (sm…

7720d5b

…ithy-lang#565)

gosar added 20 commits July 1, 2022 15:53

Initial compiling version using DirectedCodegen

ab856b1

Doesn't generate everything though, but compiles and can run the generator

Generate more files and fixing some imports

0fbfc78

Generates static files, index.ts, etc. Fixes imports that were `../` instead of `./`

Fix self import of Input/Output in commands

d55007e

Remove unnecessary src folder prepend

bbdde79

Based on smithy-lang#551

Fix code formatting failures in checkstyle

b78daa7

Fix spotbugs failures

78ae2fe

Remove TypeScriptIntegration.onShapeWriterUse

75ff597

Remove unnecessary TypeScriptDelegator constructor

6f5d6c9

Cleanup TypeScriptCodegenContext

81cc003

SmithyIntegrations are available from CodegenContext

a90c0f0

Use priority instead of getOrder to sort Integrations

4502fac

Move SSDK specific validations inside generateService

c2e843a

Remove CodegenVistor

569069f

Given the plugins have moved to DirectedTypeScriptCodegen

Remove some unnecessary TODOs and unnecessary code

a9e7ec3

The generator does not produce any .tsx files.

Remove deprecated TypeScriptCodegenPlugin.createSymbolProvider()

7fb4b74

Clarify why createDedicatedInputsAndOutputs is not used

b38aaf1

Clarify TODO for refactoring generateProtocolTests usage

6342409

Fix DirectedTypeScriptCodegen initial commit based on rebase from main

a398fec

gosar force-pushed the directed-codegen branch from a295b49 to a398fec Compare July 1, 2022 23:23

AllanZhengYP approved these changes Jul 1, 2022

View reviewed changes

gosar merged commit be2d6ac into smithy-lang:directed-codegen Jul 1, 2022

gosar mentioned this pull request Aug 31, 2022

Use DirectedCodegen #585

Merged

milesziemer added a commit to milesziemer/smithy-typescript that referenced this pull request Sep 30, 2022

Remove TypeScriptIntegration.writeAdditionalFiles

b6e54bb

The logic of `writeAdditionalFiles` was moved into `customize` as a follow up on the discussion here: smithy-lang#548 (comment)

milesziemer mentioned this pull request Sep 30, 2022

Remove TypeScriptIntegration.writeAdditionalFiles #604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DirectedCodegen #548

Use DirectedCodegen #548

gosar commented May 24, 2022 •

edited

Loading

mtdowling commented Jun 17, 2022

gosar Jun 23, 2022

AllanZhengYP Jun 27, 2022

gosar Jun 27, 2022

AllanZhengYP Jun 27, 2022

AllanZhengYP Jun 27, 2022

gosar Jun 27, 2022

gosar Jun 27, 2022

AllanZhengYP Jun 27, 2022

gosar Jun 27, 2022

AllanZhengYP Jun 27, 2022

AllanZhengYP left a comment

Use DirectedCodegen #548

Use DirectedCodegen #548

Conversation

gosar commented May 24, 2022 • edited Loading

Testing by regenerating based on new code

Clients

SSDKs

Misc

mtdowling commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanZhengYP left a comment

Choose a reason for hiding this comment

gosar commented May 24, 2022 •

edited

Loading