Changes to generate anonymous types inside parent type's namespace #109

desaikd · 2024-05-15T23:50:28Z

Issue #103:

Description of changes:

This PR works on first portion of #103 issue which is defining a namespace corresponding to parent type for anonymous types. As described in the issue:

For Java, this means that anonymous types(e.g. AnonymousType1) will be placed inside its parent class (e.g. Foo) in the same namespace (e.g. Foo.AnonymousType1)
For Rust, this means that anonymous types(e.g. AnonymousType1) will be placed inside its parent type module(e.g. foo).

Generated code:

Generated code for given ISL test files in Java can be found here
Generated code for given ISL test files in Rust can be found here

List of changes:

Changes to generate anonymous types as child classes in Java
- Adds a new template anonymous_Type.templ which has macros to generate
  anonymous types as child classes
- Modifies class.templ to include anonymous types as child classes
- Modifies setter methods in class.templ to include base types in the
  arguments instead of AnonymousTypes
- Modifies tests to remove reference to AnonymousTypes
- Adds new util structure AnonymousType which will be used to render
  anonymous types as chilsren and modify setter methods to include base
  types
Adds changes for generating anonymous types in child modules for Rust
- Adds a new anonymous_type.templ template to generate a child module
  for anonymous types
- Modifies struct.templ to use anonymous_type template

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Adds a new template anonymous_Type.templ which has macros to generate anonymous types as child classes * Modifies class.templ to include anonymous types as child classes * Modifies setter methods in class.templ to include base types in the arguments instead of `AnonymousType`s * Modifies tests to remove reference to `AnonymousType`s * Adds new util structure `AnonymousType` which will be used to render anonymous types as chilsren and modify setter methods to include base types

* Adds a new `anonymous_type.templ` template to generate a child module for anonymous types * Modifies `struct.templ` to use anonymous_type template

desaikd · 2024-05-16T00:07:38Z

src/bin/ion/commands/beta/generate/templates/java/anonymous_type.templ

@@ -0,0 +1,209 @@
+{# following macro defines an anonymous type as children class for its parent type definition #}


(🗺️ PR tour) This file adds 3 new macros for generating anonymous types.

anonymous_type: Generates a new child class for given anonymous type based on its properties (e.g. target_kind_name, abstract_data_type).

initialize_anonymous_type: This is helper macro to initialize anonymous types inside setter methods.

define_params_for_anonymous_type: This is helper macro to define arguments to setter methods. (This macrohelps get the base type as argument instead of anonymous type itself, check this code snippet to see how this is used)

Can we just get rid of the anonymous types when it's a List<Something>? (Is that going to be done in a different PR?)

Did you mean to get rid of creating a nested class which contains List<Something>? I have created a separate class for anonymous type with ISL like {type: list, element: int} because there could be more constraints added to this anonymous type which might require adding more variables/APIs to this anonymous type class with List<Something>.
But I can add a check to see if there are no more constraints then the type and element then I can skip generating anonymous type for it.

desaikd · 2024-05-16T00:08:18Z

src/bin/ion/commands/beta/generate/templates/rust/anonymous_type.templ

@@ -0,0 +1,135 @@
+{# following macro defines an anonymous type as children class for its parent type definition #}


(🗺️ PR tour) This file adds a new macros for generating anonymous types. This macro generates a new child class for given anonymous type based on its properties (e.g. target_kind_name, abstract_data_type).

desaikd · 2024-05-16T00:09:23Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

+{# Includes the macros for anonymous types that will be added as child classes #}
+{% import "anonymous_type.templ"  as macros %}
+use {{ target_kind_name | snake }}::{{ target_kind_name }};
+pub mod  {{ target_kind_name | snake }} {


(🗺️ PR tour) This change adds a new module with the name top level type definition. This module is where all the intermediate/anonymous types will be generated.

desaikd · 2024-05-16T00:12:25Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

@@ -159,4 +173,8 @@ public final class {{ target_kind_name }} {
            writer.stepOut();
        {% endif %}
    }
+
+    {% for inline_type in anonymous_types -%}


(🗺️ PR tour) This is where all the anonymous types will be generated for given top level type definition.

Will this approach also work for nested types within other nested types? E.g.:

type::{ name: Foo fields: { position: { x: { fields: { real_part: float, imaginary_part: float } }, y: { fields: { real_part: float, imaginary_part: float } } } } }

Yes it does because the template defined for anonymous types also has statement to check for its anonymous types and generate it as a child class.
This gist has generated code for your example ISL.
Here's the statement in anonymous type template that generated nested types within other nested types: https://github.com/amazon-ion/ion-cli/pull/109/files#diff-4316040659b5b43183dc6dbee67e1f131e4942f950cd2e017b7c24b352b2e51dR197

desaikd · 2024-05-16T00:13:04Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

    }
+
+    {% for inline_type in anonymous_types -%}


(🗺️ PR tour) This is where all the anonymous types will be generated for given top level type definition.

desaikd · 2024-05-16T00:14:09Z

src/bin/ion/commands/beta/generate/utils.rs

+pub struct AnonymousType {
+    pub(crate) target_kind_name: String,
+    pub(crate) fields: Vec<Field>,
+    pub(crate) abstract_data_type: AbstractDataType,
+    pub(crate) anonymous_types: Vec<AnonymousType>,
+}


(🗺️ PR tour) This is a utility that is used to store anonymous types and use it in the templates to render as child classes/structs for a type definition.

desaikd · 2024-05-16T00:19:08Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

-            {{ field.name | snake }},
-            {% endfor %}
-        }
+    #[derive(Debug, Clone, Default)]


(🗺️ PR tour) Most of the following changes are due to indentation of having struct inside a module. The only change to consider is here.

zslayton

This looks good to me, though I must admit I'm not versed enough in Tera template syntax to feel like I reviewed that part thoroughly.

Do the generated unit tests exercise this new functionality?

code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java

src/bin/ion/commands/beta/generate/generator.rs

zslayton · 2024-05-17T00:53:55Z

src/bin/ion/commands/beta/generate/templates/java/anonymous_type.templ

+    /**
+     * Writes a {{ target_kind_name }} as Ion from an {@link IonWriter}.
+     *
+     * This method does not close the writer after writing is complete.
+     * The caller is responsible for closing the stream associated with the writer.
+     */


I really like that this generates javadoc 👍

src/bin/ion/commands/beta/generate/templates/rust/anonymous_type.templ

code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java

popematt · 2024-05-17T20:29:37Z

code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java

-         s.setC(new AnonymousType3(new ArrayList<String>()));
+         s.setC(new ArrayList<String>());
         assertEquals(true, s.getC().getValue().isEmpty(), "s.getC().isEmpty() should return `true`");


We need to make a similar change to the getter, or else it will just lead to user confusion. If setC(List<String> c) accepts a list, then getC() should return a list.

I didn't change the getters because for an inline type that is for example a struct it would require to return the struct(e.g. check this test case of nested struct hat has a field c which is also a struct ). This will be simplified once we have $codegen_name or similar which will return class C instead of AnonymousTypeX.
Other option is to instead let all the getX() methods of the nested struct be part of the parent class.

popematt · 2024-05-17T20:31:59Z

src/bin/ion/commands/beta/generate/templates/java/anonymous_type.templ

@@ -0,0 +1,209 @@
+{# following macro defines an anonymous type as children class for its parent type definition #}


Can we just get rid of the anonymous types when it's a List<Something>? (Is that going to be done in a different PR?)

popematt · 2024-05-17T20:35:45Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

@@ -159,4 +173,8 @@ public final class {{ target_kind_name }} {
            writer.stepOut();
        {% endif %}
    }
+
+    {% for inline_type in anonymous_types -%}


Will this approach also work for nested types within other nested types? E.g.:

type::{ name: Foo fields: { position: { x: { fields: { real_part: float, imaginary_part: float } }, y: { fields: { real_part: float, imaginary_part: float } } } } }

src/bin/ion/commands/beta/generate/utils.rs

desaikd · 2024-05-17T21:12:51Z

Do the generated unit tests exercise this new functionality?

Yes, it does. The nested_struct.isl file contains a 2-level anonymous type/nested type for which the code generates, In Java the anonymous class into the parent class and for Rust the anonymous struct into parent struct's module.

desaikd · 2024-05-23T23:33:39Z

As per the conversation in this PR and offline, it would be simpler to express sequence and scalar nested types as is instead of creating another nested class/struct for it. This will be implemented in a separate PR.
Example ISL:

type::{
   name: foo,
   fields: {
      sequence_type: { type: list, element: int },
      scalar_type: { type: string }
   }
}

Output Java:

class Foo {
   private sequence_type: ArrayList<Integer>; // current impl would treat it as `NestedTypeX`
   private scalar_type: String; // current impl would treat it as `NestedTypeY`
   ...
}

desaikd added 2 commits May 15, 2024 14:56

Adds changes for generating anonymous types in child modules for Rust

f34a3fe

* Adds a new `anonymous_type.templ` template to generate a child module for anonymous types * Modifies `struct.templ` to use anonymous_type template

desaikd requested review from popematt and zslayton May 15, 2024 23:50

desaikd commented May 16, 2024

View reviewed changes

zslayton approved these changes May 17, 2024

View reviewed changes

popematt reviewed May 17, 2024

View reviewed changes

desaikd added 2 commits May 22, 2024 14:56

Adds suggested changes

06edcda

Renames all references of anonymous type to nested type

e4675b9

desaikd mentioned this pull request May 23, 2024

Sequence and scalar nested types should not generate a separate class/struct #110

Closed

popematt approved these changes May 24, 2024

View reviewed changes

desaikd merged commit b2cfecd into amazon-ion:master May 28, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to generate anonymous types inside parent type's namespace #109

Changes to generate anonymous types inside parent type's namespace #109

desaikd commented May 15, 2024 •

edited

Loading

desaikd May 16, 2024

popematt May 17, 2024

desaikd May 17, 2024

desaikd May 16, 2024

desaikd May 16, 2024

desaikd May 16, 2024

popematt May 17, 2024

desaikd May 17, 2024

desaikd May 16, 2024

desaikd May 16, 2024

desaikd May 16, 2024 •

edited

Loading

zslayton left a comment

zslayton May 17, 2024

popematt May 17, 2024

desaikd May 17, 2024

popematt May 17, 2024

popematt May 17, 2024

desaikd commented May 17, 2024 •

edited

Loading

desaikd commented May 23, 2024

		@@ -0,0 +1,209 @@
		{# following macro defines an anonymous type as children class for its parent type definition #}

		@@ -0,0 +1,135 @@
		{# following macro defines an anonymous type as children class for its parent type definition #}

Changes to generate anonymous types inside parent type's namespace #109

Changes to generate anonymous types inside parent type's namespace #109

Conversation

desaikd commented May 15, 2024 • edited Loading

Issue #103:

Description of changes:

Generated code:

List of changes:

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

desaikd May 16, 2024 • edited Loading

Choose a reason for hiding this comment

zslayton 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

desaikd commented May 17, 2024 • edited Loading

desaikd commented May 23, 2024

desaikd commented May 15, 2024 •

edited

Loading

desaikd May 16, 2024 •

edited

Loading

desaikd commented May 17, 2024 •

edited

Loading