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

Changes from 2 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
99 changes: 88 additions & 11 deletions src/bin/ion/commands/generate/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,9 +351,7 @@ 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 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")) {
} else if Self::contains_scalar_constraints(constraints) {
if is_nested_type {
self.build_scalar_from_constraints(constraints, code_gen_context, isl_type)?
} else {
Expand Down Expand Up @@ -382,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 @@ -458,7 +463,33 @@ 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],
Expand Down Expand Up @@ -510,7 +541,27 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
Ok(AbstractDataType::Structure(structure_builder.build()?))
}

/// Build wrapped scalar data type from constraints
/// 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],
Expand All @@ -521,9 +572,14 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
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!(
Expand All @@ -533,6 +589,7 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {

// 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(
Expand All @@ -547,7 +604,23 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
))
}

/// Build scalar data type from constraints
/// 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],
Expand All @@ -556,18 +629,22 @@ impl<'a, L: Language + 'static> CodeGenerator<'a, L> {
) -> 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(format!(
"Could not determine `FullQualifiedTypeReference` for type {:?}",
isl_type
)))?;
.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."
))?;

// by default fields aren't closed
scalar_builder.base_type(type_name);
found_base_type = true;
}
_ => {
return invalid_abstract_data_type_error(
Expand Down
Loading