(feat): Add support for recursive shapes in Go #608

rishav-karanjit · 2024-09-23T05:07:10Z

This PR includes:

Support for the recursive shape
Formatting the code
Adding/updating go.mod in test models
New test model (recursive shape)

* Put container shape conv to different function * Write functions to support recursive shape * Update aggregate model for test only * Some refactoring * Add logic to support recursive function * Rename recursiveShapeHelper -> toNativeContainerShapeHelper * Fix changes made for recursive shape * Some more fixes * Add my plan --REVERT_LATER * Fix * Add go mod to dafny runtime go * Add go mod to Standard library * Support recursive shapes in to dafny * Add toDafnyContainerShapeHelper * Separate shapes into functions in To dafny * Comment contraints validation code for now * Put recursive shape inside aggregate * Remove adding go mod to deps * Add } * Update shape visitor and its helper * Update DafnyLocalServiceTypeConversionProtocol * Add a } * Remove unwanted print * Remove all unwanted func() * Function name should be based on member shape not on shape * Add check for member shape and shape * Update validation generator for Recursive shape * Fix validation * Add list * Revert "Add list" This reverts commit 2036bbf. * Add recursive shape * Add recussive shape * revert back * Add assertionRequired boolean * Add knownSmithyType * Remove redundant thingy * Add pointer to the output type if needed * Update shape for recursive shape * In native break all shapes to a function * Fix map and list input type * Fix union pointer and structure assertion * Remove redundancy * remove the package name * Function all shapes in to native * fix boolean for shape conv in func: * Update for resource shape * Update smithy name resolver * remove redundant "}" * fixes to reference of service type * Output type for reference * Add return * Add type for service * Revert change to service * Handle service type output * long and double to support pointer and non pointer type * Fix Go Pointable * Put all shapes to func * remove redundant code * Format * Remove go mod * Merge fix * Fix returns * Bring back dots * Add go mod * Update make file * Fix wrappedIndex * Add Go mod * New codegen uses sdk id and service name * New codegen uses sdk id * Remove go mod * add go mod * add go mod * update get smithy type in AWS * Add generated shim because dafny generated name is camel case but not smithy generated * Shim is generated in service shape namespace * revert later * Revert "revert later" This reverts commit 7ae73f0. * fixes * Revert aggregate * fixes * Update recursive shape * Add go mod * fixes: * fixes * fix import

ShubhamChaturvedi7

Reviewed the files under TestModels. Few top level comments:

Always provide a description unless a PR is like very small. Your current reviewer might have context, but down the line people might read this PR to figure out the intention.
Break PRs down into logical units. Easier to thoroughly review 2 small PRs than 1 huge PR.
If changing Dafny name which affects other languages - call this out in TODO (unless you are prototyping / unblokcing someone but then you don't need a PR)
Keep optimization / linting / formatting tasks in a different PR or call them out in description.

TestModels/CodegenPatches/Model/SimpleCodegenpatchesTypes.dfy

TestModels/CodegenPatches/Model/SimpleCodegenpatchesTypesWrapped.dfy

TestModels/LocalService/src/WrappedIndex.dfy

TestModels/RecursiveShape/Makefile

ShubhamChaturvedi7 · 2024-09-25T20:13:06Z

TestModels/RecursiveShape/Model/RecursiveShape.smithy

+    value: StructureWithRecursion
+}
+
+structure StructureWithRecursion {


Good Model :)

TestModels/RecursiveShape/removeDotFromExtern.sh

TestModels/RecursiveShape/src/Index.dfy

TestModels/RecursiveShape/src/SimpleRecursiveShapeImpl.dfy

TestModels/RecursiveShape/test/SimpleRecursiveShapeImplTest.dfy

TestModels/aws-sdks/ddb/Model/model.json

.../java/software/amazon/polymorph/smithygo/awssdk/DafnyAwsSdkClientTypeConversionProtocol.java

ShubhamChaturvedi7

Look around the corner and see if any of the comments are applicable to other places as well, not just where it has been provided.

ShubhamChaturvedi7 · 2024-10-03T22:14:59Z

TestModels/RecursiveShape/Makefile

+#TODO: Think about extern processing in new test models
+ENABLE_EXTERN_PROCESSING=1


Drop this since it's a new model and we don't need to rely on the sed processing. We don't want to set a precedence.

We still need sed processing because make polymorph_dafny will generate module {:extern "simple.recursiveshape.internaldafny.types" } SimpleRecursiveShapeTypes. Thats why there is a TODO.

ShubhamChaturvedi7 · 2024-10-03T22:15:24Z

TestModels/RecursiveShape/Makefile

+# Constants for languages that drop extern names (Python, Go)
+# TODO: Before merging decide if we need this variables
+TYPES_FILE_PATH=Model/SimpleRecursiveShapeTypes.dfy
+TYPES_FILE_WITH_EXTERN_STRING="module {:extern \"simple.recursiveshape.internaldafny.types\" } SimpleRecursiveShapeTypes"
+TYPES_FILE_WITHOUT_EXTERN_STRING="module SimpleRecursiveShapeTypes"
+
+INDEX_FILE_PATH=src/Index.dfy
+INDEX_FILE_WITH_EXTERN_STRING="module {:extern \"simple.recursiveshape.internaldafny\"} SimpleRecursiveShape refines AbstractSimpleRecursiveShapeService {"
+INDEX_FILE_WITHOUT_EXTERN_STRING="module SimpleRecursiveShape refines AbstractSimpleRecursiveShapeService {"
+
+WRAPPED_INDEX_FILE_PATH=src/WrappedSimpleRecursiveShapeImpl.dfy
+WRAPPED_INDEX_FILE_WITH_EXTERN_STRING="module {:extern \"simple.recursiveshape.internaldafny.wrapped\"} WrappedSimpleRecursiveShapeService refines WrappedAbstractSimpleRecursiveShapeService {"
+WRAPPED_INDEX_FILE_WITHOUT_EXTERN_STRING="module WrappedSimpleRecursiveShapeService refines WrappedAbstractSimpleRecursiveShapeService {"


Again, we don't need these.

No. We need TYPES_FILE sed processing because make polymorph_dafny will generate module {:extern "simple.recursiveshape.internaldafny.types" } SimpleRecursiveShapeTypes. INDEX_FILE and WRAPPED_INDEX are also needed because when ENABLE_EXTERN_PROCESSING=1 we will need TYPES_FILE, INDEX_FILE and WRAPPED_INDEX. Things will change in main between now and when we start merging the Go. Thats why there is a TODO.

ShubhamChaturvedi7 · 2024-10-03T22:20:30Z

TestModels/SimpleTypes/SimpleEnumV2/runtimes/go/ImplementationFromDafny-go/go.mod

@@ -0,0 +1,12 @@
+module github.com/smithy-lang/smithy-dafny/TestModels/SimpleTypes/SimpleEnumV2


Leave it in since you've already implemented it.

ShubhamChaturvedi7 · 2024-10-09T22:58:25Z

TestModels/LocalService/runtimes/go/ImplementationFromDafny-go/go.mod

@@ -0,0 +1,12 @@
+module github.com/smithy-lang/smithy-dafny/TestModels/LocalService


Is LocalService implemented in Go?

ShubhamChaturvedi7 · 2024-10-09T23:14:38Z

TestModels/RecursiveShape/test/SimpleRecursiveShapeImplTest.dfy

+    ensures client.ValidState()
+  {
+    var myDataMap: MapWithRecursion := map[];
+    myDataMap := myDataMap["key2" := StructureWithRecursion(content := Some(IntegerValue(42)))];


nit: Do we need self assignment? Can we not just assign to the key?

Done in here: 1987f7d

ShubhamChaturvedi7 · 2024-10-09T23:38:26Z

...software/amazon/polymorph/smithygo/localservice/DafnyLocalServiceTypeConversionProtocol.java

@@ -322,6 +328,7 @@ public void generateSerializers(GenerationContext context) {
    if (serviceShape.hasTrait(LocalServiceTrait.class)) {
      generateConfigSerializer(context);
    }
+    generateSerializerFunctions(context, alreadyVisited);


ShubhamChaturvedi7 · 2024-10-09T23:42:03Z

.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java

            namespace.concat(
              context.symbolProvider().toSymbol(serviceShape).getName()
            ),
            dataSource
          );
      }
      return """
-      func () *%s {
+      return func () *%s {


I think we discussed that we don't need these inline function calls anywhere. Let's see if we can get it done cheaply. Otherwise add a TODO.

I think if I just remove the function signature and make shape visitor return return <Implementation> this would "just" work as right now. However, I also think there could be a hidden failure that I am not thinking about right now. So, the best thing to do rn is to add TODO. When we have CI working fully and MPL in a stable state, I think at we should make an attempt to remove this anonymous function and test it.

ShubhamChaturvedi7 · 2024-10-09T23:42:57Z

.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java

@@ -198,31 +201,43 @@ public String structureShape(final StructureShape shape) {
      ),
      DafnyNameResolver.dafnyTypesNamespace(shape)
    );
-
+    String maybeAddress = (this.isOptional) ? "&" : "";


referenceType ?

Renamed it to referenceType

ShubhamChaturvedi7 · 2024-10-09T23:44:04Z

...in/java/software/amazon/polymorph/smithygo/localservice/shapevisitor/ShapeVisitorHelper.java

+
+public class ShapeVisitorHelper {
+
+  public static final Map<MemberShape, Boolean> toDafnyOptionalityMap =


ShubhamChaturvedi7 · 2024-10-09T23:46:07Z

...thy-dafny-codegen/src/main/java/software/amazon/polymorph/smithygo/utils/GoCodegenUtils.java

@@ -32,6 +32,26 @@ public static String getType(Symbol symbol, ServiceTrait serviceTrait) {
    throw new RuntimeException("Failed to determine shape type");
  }

+  public static String getType(final Symbol symbol, final Shape shape) {
+    // symbol.getProperty(SymbolUtils.GO_ELEMENT_TYPE, Symbol.class).isEmpty()


Remove commented out code.

* chore: Style fix * chore: formatting fix --------- Co-authored-by: Shubham Chaturvedi <[email protected]>

This reverts commit d0b2edc.

ShubhamChaturvedi7

Added few final thoughts. I'm approving this PR since it has been open for long and likely things have changed on the other branch. Go ahead merge this PR in the Golang/dev branch and open a new PR with changes to address some of the comments.

ShubhamChaturvedi7 · 2024-10-16T22:27:09Z

.../java/software/amazon/polymorph/smithygo/awssdk/DafnyAwsSdkClientTypeConversionProtocol.java

+          }
+          alreadyVisited.add(visitingMemberShape.toShapeId());
+          var outputType = SmithyNameResolver.getSmithyTypeAws(serviceTrait, context.symbolProvider().toSymbol(visitingShape), true);
+          switch (visitingShape.getType()) {


GoCodegenUtils.getType should take care of this. Add a TODO and fix it in another PR.

ShubhamChaturvedi7 · 2024-10-16T22:33:27Z

.../java/software/amazon/polymorph/smithygo/awssdk/DafnyAwsSdkClientTypeConversionProtocol.java

+              visitingMemberShape,
+              "FromDafny"
+            ),
+            "interface {}",


Add a TODO because we should be able to use the native type instead of the interface.

ShubhamChaturvedi7 · 2024-10-16T22:57:11Z

...n/java/software/amazon/polymorph/smithygo/awssdk/shapevisitor/DafnyToAwsSdkShapeVisitor.java

+        final var memberName = context.symbolProvider().toMemberName(member);
+        // unwrap union type, assert it then convert it to its member type with Dtor_ (example: Dtor_BlobValue()). unionDataSource is not a wrapper object until now.
+        var unionDataSource = dataSource.concat(".Dtor_").concat(memberName.replace(shape.getId().getName().concat("Member"), "")).concat("()");
+        final var isMemberShapePointable = (awsSdkGoPointableIndex.isPointable(targetShape) && awsSdkGoPointableIndex.isDereferencable(targetShape)) && !targetShape.isStructureShape();


Next PR: Check if you can just use the awsSdkGoPointableIndex.isPointable(memberShape)

ShubhamChaturvedi7 · 2024-10-16T23:09:17Z

...ny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/ValidationGenerator.java

+    return memberShape
+      .getId()
+      .toString()
+      .replaceAll("[.$#]", "_")
+      .concat("_")
+      .concat(suffix);


Use the namespace and name concat.

ShubhamChaturvedi7 · 2024-10-16T23:12:50Z

...ny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/ValidationGenerator.java

+    for (final MemberShape key : validationFuncMap.keySet()) {
+      String inputType = "";
+      if (validationFuncInputTypeMap.containsKey(key)) {
+        inputType = "Value ".concat(validationFuncInputTypeMap.get(key));


Use Value %s.format()` otherwise it's hard to read the whitespace.

ShubhamChaturvedi7 · 2024-10-16T23:33:03Z

.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java

-        return nil
+    if (isOptional) {
+      return """
+      return func () *float64 {


Use the if check with %s to figure out if it's a pointer or not.

Let's do this as a clean up task at the end. Every non container shape is written in this way. I will add a TODO.

rishav-karanjit added 10 commits September 22, 2024 22:06

Remove redundant files

015825f

CI fixes

355d74a

Remove redundant files

aee93ba

Refactoring

9a452e2

Update src module name

e92d40c

Update module name

99df7d7

Update module name

401c64d

Add go mod

0d4547e

update module name

ae6eecb

rishav-karanjit changed the title ~~(feat): Add support for recursive shapes in Go (#607)~~ (feat): Add support for recursive shapes in Go Sep 23, 2024

rishav-karanjit added 7 commits September 23, 2024 10:40

Format

de202ba

Change shape name

bb25f0a

Remove redundant codes

78cca65

Remove redundant codes

1870322

Add go mod

7ce1847

Update Makefile

c8883fb

rm go mod

dd83e91

rishav-karanjit marked this pull request as ready for review September 23, 2024 18:25

rishav-karanjit requested a review from a team as a code owner September 23, 2024 18:25

ShubhamChaturvedi7 requested changes Sep 25, 2024

View reviewed changes

rishav-karanjit added 9 commits September 27, 2024 21:00

Recursive shape support in aws sdk go

f0e08c7

Add is timestamp

135309b

Copy local service changes to sdk

ec28625

PR Comment fix

622071e

comment fix

0c6ff38

Remove print

99ce8d5

Fix comments

8155e77

Add recursive shape to disabled test except in go

f176be6

Delete redundant file

c00a978

rishav-karanjit commented Oct 2, 2024

View reviewed changes

.../java/software/amazon/polymorph/smithygo/awssdk/DafnyAwsSdkClientTypeConversionProtocol.java Outdated Show resolved Hide resolved

rishav-karanjit added 3 commits October 3, 2024 11:01

put final whereever I found its necessary

debec9d

Add more finals

61531b8

add import time

d15373f

ShubhamChaturvedi7 requested changes Oct 9, 2024

View reviewed changes

rishav-karanjit and others added 18 commits October 10, 2024 11:58

Add more finals

4727beb

Add snakecase VISITOR_FUNCTION_MAP

f6e14a5

One more finals

7cf7d0c

Snakecase toDafnyOptionalityMap to TO_DAFNY_OPTIONALITY_MAP

c091fe4

Snakecase

4aeb4d5

Remove redundant var

9011bb8

remove redundant extern

9ec87ce

chore: Style fix (#631)

838a0e4

* chore: Style fix * chore: formatting fix --------- Co-authored-by: Shubham Chaturvedi <[email protected]>

Merge branch 'Golang/dev' into add-recursive-Go

4b46695

just assign to the key

1987f7d

clean up

75c8e6a

typo unassertDatasource -> unAssertedDataSource

7121944

change variable name to referenceType

f891cc9

Put more finals

c9094ad

clean up

d0b2edc

Revert "clean up"

bffadc8

This reverts commit d0b2edc.

clean up

729b742

Add TODO to remove anonymous function

319775f

ShubhamChaturvedi7 approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'Golang/dev' into add-recursive-Go

5a36f26

ShubhamChaturvedi7 approved these changes Oct 16, 2024

View reviewed changes

rishav-karanjit merged commit f04495a into Golang/dev Oct 16, 2024
14 of 108 checks passed

rishav-karanjit deleted the add-recursive-Go branch October 17, 2024 16:28

This was referenced Oct 17, 2024

fix: Minor fixes and add TODO #639

Merged

chore: Change function names and refactor #641

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat): Add support for recursive shapes in Go #608

(feat): Add support for recursive shapes in Go #608

rishav-karanjit commented Sep 23, 2024 •

edited

Loading

ShubhamChaturvedi7 left a comment

ShubhamChaturvedi7 Sep 25, 2024

ShubhamChaturvedi7 left a comment

ShubhamChaturvedi7 Oct 3, 2024

rishav-karanjit Oct 10, 2024

ShubhamChaturvedi7 Oct 3, 2024

rishav-karanjit Oct 10, 2024

ShubhamChaturvedi7 Oct 3, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 10, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 15, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 15, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 16, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 15, 2024

ShubhamChaturvedi7 Oct 9, 2024

ShubhamChaturvedi7 Oct 9, 2024

rishav-karanjit Oct 15, 2024

ShubhamChaturvedi7 left a comment

ShubhamChaturvedi7 Oct 16, 2024

ShubhamChaturvedi7 Oct 16, 2024

ShubhamChaturvedi7 Oct 16, 2024

ShubhamChaturvedi7 Oct 16, 2024

ShubhamChaturvedi7 Oct 16, 2024

ShubhamChaturvedi7 Oct 16, 2024

rishav-karanjit Oct 17, 2024

		#TODO: Think about extern processing in new test models
		ENABLE_EXTERN_PROCESSING=1

		@@ -0,0 +1,12 @@
		module github.com/smithy-lang/smithy-dafny/TestModels/SimpleTypes/SimpleEnumV2


		public class ShapeVisitorHelper {

		public static final Map<MemberShape, Boolean> toDafnyOptionalityMap =

(feat): Add support for recursive shapes in Go #608

(feat): Add support for recursive shapes in Go #608

Conversation

rishav-karanjit commented Sep 23, 2024 • edited Loading

ShubhamChaturvedi7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShubhamChaturvedi7 left a 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

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

ShubhamChaturvedi7 left a 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

rishav-karanjit commented Sep 23, 2024 •

edited

Loading