Adds new model changes for scalar #143

desaikd · 2024-09-16T18:52:11Z

Issue #, if available:

Description of changes:

This PR works on adding new model changes following up on #135. This Pr only contains changes for Structure (does not contains changes for model variants Sequence and Scalar). This PR disables Rust support until the entire functionality for the new model change is supported and uses feature branch new-model-changes as target.

List of changes:

Generator changes:
- Adds build_wrapped_scalar_from_constraints which is used for constructing named scalar data type as data model.
- Adds build_scalar_from_constraints which is used for constructing nested scalar data type as data model. (Doesn't have a name)
- Adds is_nested flag in convert_isl_type_def_to_data_model_node which can be used to either call wrapped sclar or scalar ADT construction.
- Modified structure construction to move the common logic at the beginning of the method.
Model changes:
- Changes Scalar and WrappedScalar ADT to represent the underlying scalar type with base_type and its name with name field.
- Adds doc comment changes for the same
Adds templates changes for java/scalar.templ

Generated code:

The generated code still stays the same only the template changes to use the new model. (Only added scalar files for checking this change, other files remain same)

Generated Java code can be found here.

Tests:

Adds tests for ISL to model conversion.
Modifies roundtrip code gen tests for scalar test cases.

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

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

zslayton · 2024-09-16T18:59:36Z

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

+                        )))?;
+
+                    // by default fields aren't closed
+                    wrapped_scalar_builder.base_type(type_name);


This method name is a noun, which made me expect a return value. What is it doing instead? Can the name be updated to reflect that?

So these methods are generated based on the properties of ADT. (In this example, from properties of WrappedScalar). I am using derive_builder crate to derive the builders for different ADT variants. Hence the name reflects tot he property name base_type which represents the base type for this scalar (e.g. string, int, Foo(user defined type def) etc.)

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

zslayton · 2024-09-16T19:01:25Z

src/bin/ion/commands/generate/model.rs

-    // }
-    // ```
-    name: FullyQualifiedTypeReference,
+    // Corresponding `name` would be `vec!["Foo"]` and `base_type` would be `FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }`.


I think the comments that were removed were helpful

So the previous comments reflected to previous properties and this new only explains the modified properties.

popematt · 2024-09-17T17:02:23Z

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

@@ -336,6 +338,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
        isl_type_name: &String,
        isl_type: &IslType,
        code_gen_context: &mut CodeGenContext,
+        is_nested_type: bool,


Doesn't need to be addressed in this PR, but I think that our data model might need to include a distinction between the "new type" pattern and "primitive type" rather than just having a is_nested_type field.

This is_nested type is not necessarily distinction between new type and primitive because even a user defined type when used as a type reference will not be nested type but still a new type right?
e.g.

type::{ name: my_type, type: foo // here `foo` is a type reference but not necessarily a nested type }

type::{ name: my_type, type: foo // here `foo` is a type reference but not necessarily a nested type }

I think this would be a new-type.

Whether or not a type is "nested" is just a logistical thing that relates to whether we have to generate a name for it because it's anonymous. (Since we need to generate a name, we also put it in a namespace to ensure that we don't accidentally run into name conflicts.)

That being said, as our code generator gets more sophisticated, we don't have to generate a wrapper type for things like this:

type::{ name: foo, fields: { bar: { type: int, valid_values: range::[1, max] } } }

Rather than creating a Bar type, we could just use Integer (or u64 or whatever), and add the range validation into the constructor and/or builder of Foo.

We would still need to create a nested type for cases like this...

type::{ name: foo, fields: { bar: { fields: { baz: int, quuz: bool, } } } }

... because even though ISL is strictly subtractive, the type of the bar field is a structural composition of other types rather than just being a simple restriction on an existing type.

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

popematt · 2024-09-17T17:21:12Z

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

+        let mut scalar_builder = ScalarBuilder::default();
+        scalar_builder.source(parent_isl_type.to_owned());
+        for constraint in constraints {
+            match constraint.constraint() {


There are a couple of issues with the way the constraints are being checked here.

As currently written, this will accept something like { type: string, type: int } without complaint. The presence of a contradiction, like { type: string, type: int } should raise an error because struct ordering isn't guaranteed, which means that the generated output could be unstable. That is an issue that should be resolved now.

On the other hand it will complain about something like { type: string, codepoint_length: 20 }. Even if we don't support generating code for constraints like this, we should not fail because someone wants to include it in their ISL. This doesn't necessarily need to be resolved in this PR as long as we document the limitations here.

(This comment also applies to build_wrapped_scalar_from_constraints.)

Thanks for explaining this! I can modify this to add a check for what the base type is for the scalar/wrapped scalar ADT and verify that they don't conflict.

currently changed this to only allow a single type constraint in a type definition. Code generator will return an error when another type constraint is found.

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

zslayton · 2024-09-18T17:58:19Z

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

+    ///     is_closed: false, // If the fields constraint was annotated with `closed` then this would be true.
+    ///  }
+    /// )
+    /// ```


Really helpful, thanks!

popematt

There's a bit of repetitive code with the way that we check for multiple type constraints, but that can be addressed in a separate PR.

LGTM.

* Adds new model changes for `Structure` (#138) * Adds new model changes for scalar (#143) * Adds changes for sequence data type (#144) * Adds tests for new model changes (#146) * Adds new model changes for Rust code generation (#147) * Adds changes for optional/required fields in Java code generation (#148) * Modifies generated setter API to take nested types instead of nested properties (#153) * Adds support for builder API generation in Java (#154) * Adds support for Java enum generation (#158) * Adds namespace fix for nested sequence and adds support for nested types in Sequence and Scalar ADT (#163) * Adds changes for nested type naming (#166) * Adds support for imports in code generation (#169)

Adds new model changes for scalar

fa11c82

desaikd requested review from popematt and zslayton and removed request for popematt September 16, 2024 18:52

zslayton reviewed Sep 16, 2024

View reviewed changes

Adds suggested changes

f0d9047

popematt reviewed Sep 17, 2024

View reviewed changes

Adds suggested changes for conflicting types

72c2cdf

desaikd requested review from popematt and zslayton September 17, 2024 21:32

zslayton approved these changes Sep 18, 2024

View reviewed changes

popematt approved these changes Sep 18, 2024

View reviewed changes

desaikd merged commit c6e0e07 into amazon-ion:new-model-changes Sep 18, 2024
3 checks passed

desaikd added a commit to desaikd/ion-cli that referenced this pull request Nov 1, 2024

Adds new model changes for scalar (amazon-ion#143)

f3ae683

popematt mentioned this pull request Nov 1, 2024

Merge new-model-changes feature branch to main #171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new model changes for scalar #143

Adds new model changes for scalar #143

desaikd commented Sep 16, 2024

zslayton Sep 16, 2024

desaikd Sep 16, 2024

zslayton Sep 16, 2024

desaikd Sep 16, 2024

popematt Sep 17, 2024

desaikd Sep 17, 2024

popematt Sep 17, 2024

popematt Sep 17, 2024

desaikd Sep 17, 2024 •

edited

Loading

desaikd Sep 17, 2024

zslayton Sep 18, 2024

popematt left a comment

Adds new model changes for scalar #143

Adds new model changes for scalar #143

Conversation

desaikd commented Sep 16, 2024

Issue #, if available:

Description of changes:

List of changes:

Generated code:

Tests:

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 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

desaikd Sep 17, 2024 •

edited

Loading