Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds new model changes for scalar #143

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Adds new model changes for scalar #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa11c82
Adds new model changes for scalar
desaikd Sep 16, 2024
f0d9047
Adds suggested changes
desaikd Sep 16, 2024
72c2cdf
Adds suggested changes for conflicting types
desaikd Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions code-gen-projects/schema/scalar.isl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type::{
name: scalar,
type: string
}
177 changes: 167 additions & 10 deletions src/bin/ion/commands/generate/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::commands::generate::context::CodeGenContext;
use crate::commands::generate::model::{
AbstractDataType, DataModelNode, FieldPresence, FieldReference, FullyQualifiedTypeReference,
StructureBuilder,
ScalarBuilder, StructureBuilder, WrappedScalarBuilder,
};
use crate::commands::generate::result::{
invalid_abstract_data_type_error, invalid_abstract_data_type_raw_error, CodeGenResult,
Expand Down Expand Up @@ -287,6 +287,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
type_name,
isl_type,
&mut code_gen_context,
true,
)?;

// add this nested type to parent code gene context's current list of nested types
Expand Down Expand Up @@ -315,6 +316,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
isl_type_name,
isl_type,
&mut code_gen_context,
false,
)?;

// add the entire type store and the data model node into tera's context to be used to render template
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

) -> CodeGenResult<DataModelNode> {
self.current_type_fully_qualified_name
.push(isl_type_name.to_case(Case::UpperCamel));
Expand All @@ -348,6 +351,12 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
.any(|it| matches!(it.constraint(), IslConstraintValue::Fields(_, _)))
{
self.build_structure_from_constraints(constraints, code_gen_context, isl_type)?
} else if Self::contains_scalar_constraints(constraints) {
if is_nested_type {
self.build_scalar_from_constraints(constraints, code_gen_context, isl_type)?
} else {
self.build_wrapped_scalar_from_constraints(constraints, code_gen_context, isl_type)?
}
} else {
todo!("Support for sequences, maps, scalars, and tuples not implemented yet.")
};
Expand All @@ -371,6 +380,13 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
Ok(data_model_node)
}

/// Verifies if the given constraints contain a `type` constraint without any container type references. (e.g. `sexp`, `list`, `struct`)
fn contains_scalar_constraints(constraints: &[IslConstraint]) -> bool {
constraints.iter().any(|it| matches!(it.constraint(), IslConstraintValue::Type(isl_type_ref) if isl_type_ref.name().as_str() != "list"
&& isl_type_ref.name().as_str() != "sexp"
&& isl_type_ref.name().as_str() != "struct"))
}

fn render_generated_code(
&mut self,
type_name: &str,
Expand Down Expand Up @@ -447,14 +463,43 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
name
}

/// Build structure from constraints
/// Builds `AbstractDataType::Structure` from the given constraints.
/// e.g. for a given type definition as below:
/// ```
/// type::{
/// name: Foo,
/// type: struct,
/// fields: {
/// a: string,
/// b: int,
/// }
/// }
/// ```
/// This method builds `AbstractDataType`as following:
/// ```
/// AbstractDataType::Structure(
/// Structure {
/// name: vec!["org", "example", "Foo"], // assuming the namespace is `org.example`
/// fields: {
/// a: FieldReference { FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }, FieldPresence::Optional },
/// b: FieldReference { FullyQualifiedTypeReference { type_name: vec!["int"], parameters: vec![] }, FieldPresence::Optional },
/// }, // HashMap with fields defined through `fields` constraint above
/// doc_comment: None // There is no doc comment defined in above ISL type def
/// source: IslType {name: "foo", .. } // Represents the `IslType` that is getting converted to `AbstractDataType`
/// is_closed: false, // If the fields constraint was annotated with `closed` then this would be true.
/// }
/// )
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really helpful, thanks!

fn build_structure_from_constraints(
&mut self,
constraints: &[IslConstraint],
code_gen_context: &mut CodeGenContext,
parent_isl_type: &IslType,
) -> CodeGenResult<AbstractDataType> {
let mut structure_builder = StructureBuilder::default();
structure_builder
.name(self.current_type_fully_qualified_name.to_owned())
.source(parent_isl_type.to_owned());
for constraint in constraints {
match constraint.constraint() {
IslConstraintValue::Fields(struct_fields, is_closed) => {
Expand All @@ -479,17 +524,11 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
}
// unwrap here is safe as the `current_abstract_data_type_builder` will either be initialized with default implementation
// or already initialized with a previous structure related constraint at this point.
structure_builder
.fields(fields)
.source(parent_isl_type.to_owned())
.is_closed(*is_closed)
.name(self.current_type_fully_qualified_name.to_owned());
structure_builder.fields(fields).is_closed(*is_closed);
}
IslConstraintValue::Type(_) => {
// by default fields aren't closed
structure_builder
.is_closed(false)
.source(parent_isl_type.to_owned());
structure_builder.is_closed(false);
}
_ => {
return invalid_abstract_data_type_error(
Expand All @@ -501,6 +540,122 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {

Ok(AbstractDataType::Structure(structure_builder.build()?))
}

/// Builds `AbstractDataType::WrappedScalar` from the given constraints.
/// ```
/// type::{
/// name: Foo,
/// type: string,
/// }
/// ```
/// This method builds `AbstractDataType`as following:
/// ```
/// AbstractDataType::WrappedScalar(
/// WrappedScalar {
/// name: vec!["org", "example", "Foo"], // assuming the namespace is `org.example`
/// base_type: FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }
/// doc_comment: None // There is no doc comment defined in above ISL type def
/// source: IslType {name: "foo", .. } // Represents the `IslType` that is getting converted to `AbstractDataType`
/// }
/// )
/// ```
///
/// _Note: Currently code generator would return an error when there are multiple `type` constraints in the type definition.
/// This avoids providing conflicting type constraints in the type definition._
fn build_wrapped_scalar_from_constraints(
&mut self,
constraints: &[IslConstraint],
code_gen_context: &mut CodeGenContext,
parent_isl_type: &IslType,
) -> CodeGenResult<AbstractDataType> {
let mut wrapped_scalar_builder = WrappedScalarBuilder::default();
wrapped_scalar_builder
.name(self.current_type_fully_qualified_name.to_owned())
.source(parent_isl_type.to_owned());

let mut found_base_type = false;
for constraint in constraints {
match constraint.constraint() {
IslConstraintValue::Type(isl_type) => {
if found_base_type {
return invalid_abstract_data_type_error("Multiple `type` constraints in the type definitions are not supported in code generation as it can lead to conflicting types.");
}
let type_name = self
.fully_qualified_type_ref_name(isl_type, code_gen_context)?
.ok_or(invalid_abstract_data_type_raw_error(format!(
"Could not determine `FullQualifiedTypeReference` for type {:?}",
isl_type
)))?;

// by default fields aren't closed
wrapped_scalar_builder.base_type(type_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

found_base_type = true;
}
_ => {
return invalid_abstract_data_type_error(
"Could not determine the abstract data type due to conflicting constraints",
);
}
}
}

Ok(AbstractDataType::WrappedScalar(
wrapped_scalar_builder.build()?,
))
}

/// Builds `AbstractDataType::Scalar` from the given constraints.
/// ```
/// { type: string }
/// ```
/// This method builds `AbstractDataType`as following:
/// ```
/// AbstractDataType::Scalar(
/// Scalar {
/// base_type: FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }
/// doc_comment: None // There is no doc comment defined in above ISL type def
/// source: IslType { .. } // Represents the `IslType` that is getting converted to `AbstractDataType`
/// }
/// )
/// ```
///
/// _Note: Currently code generator would return an error when there are multiple `type` constraints in the type definition.
/// This avoids providing conflicting type constraints in the type definition._
fn build_scalar_from_constraints(
&mut self,
constraints: &[IslConstraint],
code_gen_context: &mut CodeGenContext,
parent_isl_type: &IslType,
) -> CodeGenResult<AbstractDataType> {
let mut scalar_builder = ScalarBuilder::default();
scalar_builder.source(parent_isl_type.to_owned());

let mut found_base_type = false;
for constraint in constraints {
match constraint.constraint() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@desaikd desaikd Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

IslConstraintValue::Type(isl_type) => {
if found_base_type {
return invalid_abstract_data_type_error("Multiple `type` constraints in the type definitions are not supported in code generation as it can lead to conflicting types.");
}
let type_name = self
.fully_qualified_type_ref_name(isl_type, code_gen_context)?
.ok_or(invalid_abstract_data_type_raw_error(
"Could not determine `FullQualifiedTypeReference` for `struct`, `list` or `sexp` as open ended container types aren't supported."
))?;

scalar_builder.base_type(type_name);
found_base_type = true;
}
_ => {
return invalid_abstract_data_type_error(
"Could not determine the abstract data type due to conflicting constraints",
);
}
}
}

Ok(AbstractDataType::Scalar(scalar_builder.build()?))
}
}

#[cfg(test)]
Expand Down Expand Up @@ -535,6 +690,7 @@ mod isl_to_model_tests {
&"my_struct".to_string(),
&isl_type,
&mut CodeGenContext::new(),
false,
)?;
let abstract_data_type = data_model_node.code_gen_type.unwrap();
assert_eq!(
Expand Down Expand Up @@ -620,6 +776,7 @@ mod isl_to_model_tests {
&"my_nested_struct".to_string(),
&isl_type,
&mut CodeGenContext::new(),
false,
)?;
let abstract_data_type = data_model_node.code_gen_type.unwrap();
assert_eq!(
Expand Down
48 changes: 22 additions & 26 deletions src/bin/ion/commands/generate/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ impl AbstractDataType {
AbstractDataType::WrappedScalar(w) => {
Some(w.fully_qualified_type_name().to_owned().into())
}
AbstractDataType::Scalar(s) => Some(s.name.to_owned().into()),
AbstractDataType::Scalar(s) => Some(s.base_type.to_owned()),
AbstractDataType::Sequence(seq) => Some(seq.element_type.to_owned()),
AbstractDataType::Structure(structure) => Some(structure.name.to_owned().into()),
}
Expand All @@ -226,10 +226,11 @@ pub struct Scalar {
// element: string // this is a nested scalar type
// }
// ```
// Corresponding `FullyQualifiedName` would be `vec!["String"]`.
name: FullyQualifiedTypeName,
// Corresponding `FullyQualifiedReference` would be `FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }`.
base_type: FullyQualifiedTypeReference,
// Represents doc comment for the generated code
// If the doc comment is provided for this scalar type then this is `Some(doc_comment)`, other it is None.
#[builder(default)]
doc_comment: Option<String>,
// Represents the source ISL type which can be used to get other constraints useful for this type.
// For example, getting the length of this sequence from `container_length` constraint or getting a `regex` value for string type.
Expand Down Expand Up @@ -265,16 +266,12 @@ pub struct WrappedScalar {
// type: string
// }
// ```
// Corresponding `FullyQualifiedTypeReference` would be as following:
// ```
// FullyQualifiedTypeReference {
// type_name: vec!["Foo"], // name of the wrapped scalar type
// parameters: vec![FullyQualifiedTypeReference {type_name: vec!["String"] }] // base type name for the scalar value
// }
// ```
name: FullyQualifiedTypeReference,
// Corresponding `name` would be `vec!["Foo"]` and `base_type` would be `FullyQualifiedTypeReference { type_name: vec!["String"], parameters: vec![] }`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comments that were removed were helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

name: FullyQualifiedTypeName,
base_type: FullyQualifiedTypeReference,
// Represents doc comment for the generated code
// If the doc comment is provided for this scalar type then this is `Some(doc_comment)`, other it is None.
#[builder(default)]
doc_comment: Option<String>,
// Represents the source ISL type which can be used to get other constraints useful for this type.
// For example, getting the length of this sequence from `container_length` constraint or getting a `regex` value for string type.
Expand All @@ -286,7 +283,7 @@ pub struct WrappedScalar {

impl WrappedScalar {
pub fn fully_qualified_type_name(&self) -> &FullyQualifiedTypeName {
&self.name.type_name
&self.name
}
}

Expand Down Expand Up @@ -396,7 +393,10 @@ mod model_tests {
#[test]
fn scalar_builder_test() {
let expected_scalar = Scalar {
name: vec![],
base_type: FullyQualifiedTypeReference {
type_name: vec!["String".to_string()],
parameters: vec![],
},
doc_comment: Some("This is scalar type".to_string()),
source: anonymous_type(vec![type_constraint(named_type_ref("string"))]),
};
Expand All @@ -405,7 +405,7 @@ mod model_tests {

// sets all the information about the scalar type
scalar_builder
.name(vec![])
.base_type(vec!["String".to_string()])
.doc_comment(Some("This is scalar type".to_string()))
.source(anonymous_type(vec![type_constraint(named_type_ref(
"string",
Expand All @@ -418,12 +418,10 @@ mod model_tests {
#[test]
fn wrapped_scalar_builder_test() {
let expected_scalar = WrappedScalar {
name: FullyQualifiedTypeReference {
type_name: vec!["Foo".to_string()],
parameters: vec![FullyQualifiedTypeReference {
type_name: vec!["String".to_string()],
parameters: vec![],
}],
name: vec!["Foo".to_string()],
base_type: FullyQualifiedTypeReference {
type_name: vec!["String".to_string()],
parameters: vec![],
},
doc_comment: Some("This is scalar type".to_string()),
source: anonymous_type(vec![type_constraint(named_type_ref("string"))]),
Expand All @@ -433,12 +431,10 @@ mod model_tests {

// sets all the information about the scalar type
scalar_builder
.name(FullyQualifiedTypeReference {
type_name: vec!["Foo".to_string()],
parameters: vec![FullyQualifiedTypeReference {
type_name: vec!["String".to_string()],
parameters: vec![],
}],
.name(vec!["Foo".to_string()])
.base_type(FullyQualifiedTypeReference {
type_name: vec!["String".to_string()],
parameters: vec![],
})
.doc_comment(Some("This is scalar type".to_string()))
.source(anonymous_type(vec![type_constraint(named_type_ref(
Expand Down
Loading
Loading