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.

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

Adds a new model for code generator #123

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Jun 10, 2024

Issue #, if available:

Description of changes:

This PR is adds a new model for code generator to make it simpler for getting information on nested types and element type in sequence.

List of changes:

  • DataModel is a the new abstract data type that will be used by code generator to render templates.
  • CodeGenType is similar to the prior AbstractDataType but more informative includes fields details and element type details.
  • FullyQualifiedTypeName and FullyQualifiedTypeReference are used for storing all levels of the fully qualified name of the type.
  • This PR only introduces the model but doesn't yet use it in the implementation.

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

@jobarr-amzn jobarr-amzn changed the title Adds a new mdoel for code generator Adds a new model for code generator Jun 10, 2024
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
is_closed: bool,
// Represents the fields of the struct i.e. (field_name, field_value) pairs
// field_value represents the type of the value field as fully qualified name
// _Note: that a hashmap with (FullQualifiedTypeReference, DataModel) pairs will be stored in code generator to get information on the field_value name used here._
Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. This model is going to use FullyQualifiedTypeReferences instead of pointing to DataModel itself. But in some case, for example sequence we need to look at the DataModel to check its element_type then we can use a HashMap which stores (FullQualifiedTypeReference, DataModel) pairs.

src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

The model you have for an "entity" (type, struct, class, whatever) is a struct with common fields and an enum for the things that are different.

You may want to consider refactoring so that you have an enum as an "umbrella", and all of the data in the structs for the variants, even if there is some duplication. For example:

pub struct DataModelNode {
  name: String, // unqualified name
  this_type: Option<AnyDataType>,
  children: Vec<DataModelNode>,
}

pub enum AnyDataType {
  Struct(StructDataType),
  Scalar(ScalarDataType),
  Collection(CollectionDataType),
  // more types...
}

impl AnyDataType {
  fn source_isl() -> IslType { 
    // ...
  }
  fn doc_comment() -> Option<String> {
    // ...
  }
  fn qualified_name() -> FullyQualifiedTypeName {
    // ...
  }
}

pub struct StructDataType {
  qualified_name: FullyQualifiedTypeName,
  is_closed: bool,
  fields: HashMap<String, FullyQualifiedTypeReference>,
  source_isl: IslType,
}
// more types...

This will allow you to define functions that apply to all data model types using AnyDataType, or for specific data model types using e.g. StructDataType. I think that can go a long way toward having clear contracts and helping with readability in the long run.

If it makes it easier to reason about, you could create a trait that defines what AnyDataType and all the subtypes/variants have in common.

src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
// 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.
// This will also be useful for `text` type to verify if this is a `string` or `symbol`.
source: Option<IslType>,
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 more important things right now, but in the future, we should consider changing this to Option<&'generator IslType>.

* Added `Scalar` struct that represents nested/inline scalars.
* Added `WrappedScalar` struct that represents top level scalar type
definition.(This has a name attached to it)
* Renames `AbstractDataType` to `DataModelNode`.
* Renames `AbstractDataKind` to `AbstractDataType`.
* Adds doc comment changes.
@desaikd
Copy link
Contributor Author

desaikd commented Jun 17, 2024

The model you have for an "entity" (type, struct, class, whatever) is a struct with common fields and an enum for the things that are different.

You may want to consider refactoring so that you have an enum as an "umbrella", and all of the data in the structs for the variants, even if there is some duplication.

I have changed the model to store all information on a data type with its own implementation (e.g. Scalar, WrappedScalar, Sequence, Structure). A new variant WrappedScalar is added that can be used to represent a top level scalar type definition. A top level scalar type definition can have a name which is why its better to have it as a separate variant to have specific methods to it in WrappedScalar.

For example given below ISL,

type::{
  name: wrapped_scalar_type,
  type: int
}

Corresponding generated code in Rust would look like following:

struct WrappedScalarType {
    value: i64
 }

src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
* Doc comment changes
* Adds `FiedlPresence` enum and `FieldReference` struct
* Changes getter methods to return a reference instead of an owned value
src/bin/ion/commands/beta/generate/model.rs Outdated Show resolved Hide resolved
@desaikd desaikd merged commit 4bc895f into amazon-ion:master Jun 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants