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

Feat/bindgen improve types #2816

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Feat/bindgen improve types #2816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
354d7ec
bump: cainome-0.4.8
MartianGreed Dec 2, 2024
f059637
feat: add CairoOption and CairoCustomEnum type handling
MartianGreed Dec 2, 2024
85504a8
feat: merge schema in models.gen.ts
MartianGreed Dec 2, 2024
6917660
feat: add Input type to omit fieldOrder field
MartianGreed Dec 2, 2024
a541768
feat: recursive types in model definition
MartianGreed Dec 5, 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
Prev Previous commit
Next Next commit
feat: add CairoOption and CairoCustomEnum type handling
  • Loading branch information
@MartianGreed
MartianGreed committed Dec 2, 2024
commit f0596373f7e33386117138fcd846b6b8a8be62a7
9 changes: 9 additions & 0 deletions crates/dojo/bindgen/src/plugins/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ impl Buffer {
}
}

/// Inserts string at the specified index.
///
/// * `s` - The string to insert.
/// * `pos` - The position to insert the string at.
/// * `idx` - The index of the string to insert at.
pub fn insert_at_index(&mut self, s: String, idx: usize) {
self.0.insert(idx, s);
}

/// Finds position of the given string in the inner vec.
///
/// * `pos` - The string to search for.
Expand Down
8 changes: 8 additions & 0 deletions crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,11 @@ pub const CAIRO_BOOL: &str = "bool";
pub const JS_BOOLEAN: &str = "boolean";
pub const JS_STRING: &str = "string";
pub const JS_BIGNUMBERISH: &str = "BigNumberish";

pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Typo in Constant Name BIGNUMNERISH_IMPORT

Ohayo, sensei! There's a typo in the constant name BIGNUMNERISH_IMPORT. It should be BIGNUMBERISH_IMPORT to match the correct spelling of 'BigNumberish'.

Apply this diff to fix the typo:

- pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";
+ pub(crate) const BIGNUMBERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";

Be sure to update all references to BIGNUMNERISH_IMPORT throughout the codebase to prevent any compilation errors.

Committable suggestion skipped: line range outside the PR's diff.

pub(crate) const CAIRO_OPTION_IMPORT: &str = "import type { CairoOption } from 'starknet';\n";
pub(crate) const CAIRO_ENUM_IMPORT: &str = "import type { CairoCustomEnum } from 'starknet';\n";
pub(crate) const CAIRO_OPTION_TYPE_PATH: &str = "core::option::Option";
pub(crate) const SN_IMPORT_SEARCH: &str = "} from 'starknet';";
pub(crate) const CAIRO_OPTION_TOKEN: &str = "CairoOption,";
pub(crate) const CAIRO_ENUM_TOKEN: &str = "CairoCustomEnum,";
67 changes: 56 additions & 11 deletions crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,76 @@
use cainome::parser::tokens::{Composite, CompositeType};

use crate::error::BindgenResult;
use crate::plugins::typescript::generator::JsType;
use crate::plugins::{BindgenModelGenerator, Buffer};

use super::constants::{CAIRO_ENUM_IMPORT, CAIRO_ENUM_TOKEN, SN_IMPORT_SEARCH};
use super::token_is_custom_enum;

const CAIRO_ENUM_TYPE_IMPL: &str = "export type TypedCairoEnum<T> = CairoCustomEnum & {\n\tvariant: { [K in keyof T]: T[K] | undefined };\n\tunwrap(): T[keyof T];\n}\n";

pub(crate) struct TsEnumGenerator;

impl TsEnumGenerator {
fn check_import(&self, token: &Composite, buffer: &mut Buffer) {
// type is Enum with type variants, need to import CairoEnum
// if enum has at least one inner that is a composite type
if token_is_custom_enum(token) {
if !buffer.has(SN_IMPORT_SEARCH) {
buffer.push(CAIRO_ENUM_IMPORT.to_owned());
} else if !buffer.has(CAIRO_ENUM_TOKEN) {
// If 'starknet' import is present, we add CairoEnum to the imported types
buffer.insert_after(format!(" {CAIRO_ENUM_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
}
}
if !buffer.has(CAIRO_ENUM_TYPE_IMPL) {
let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap();
buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1);
}
Comment on lines +28 to +30
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Possible Panic when Calling unwrap() on buffer.pos(SN_IMPORT_SEARCH)

Ohayo, sensei! There's a potential issue with the use of unwrap() at line 28. If SN_IMPORT_SEARCH is not found in the buffer, pos() will return None, and calling unwrap() would cause a panic. It's recommended to handle the Option safely to avoid unexpected panics.

Apply this diff to handle the Option safely:

- let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap();
+ if let Some(pos) = buffer.pos(SN_IMPORT_SEARCH) {
+     buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1);
+ } else {
+     // Handle the case where SN_IMPORT_SEARCH is not found
+     // For example, insert CAIRO_ENUM_TYPE_IMPL at a default position or log an error
+ }

Committable suggestion skipped: line range outside the PR's diff.

}
}

impl BindgenModelGenerator for TsEnumGenerator {
fn generate(&self, token: &Composite, buffer: &mut Buffer) -> BindgenResult<String> {
if token.r#type != CompositeType::Enum || token.inners.is_empty() {
return Ok(String::new());
}

let gen = format!(
"// Type definition for `{path}` enum
let gen = if token_is_custom_enum(token) {
self.check_import(token, buffer);
format!(
"// Type definition for `{path}` enum
export type {name} = {{
{variants}
}}
export type {name}Enum = TypedCairoEnum<{name}>;
",
path = token.type_path,
name = token.type_name(),
variants = token
.inners
.iter()
.map(|inner| { format!("\t{}: {};", inner.name, JsType::from(&inner.token)) })
.collect::<Vec<String>>()
.join("\n")
)
} else {
format!(
"// Type definition for `{path}` enum
export enum {name} {{
{variants}
}}
",
path = token.type_path,
name = token.type_name(),
variants = token
.inners
.iter()
.map(|inner| format!("\t{},", inner.name))
.collect::<Vec<String>>()
.join("\n")
);
path = token.type_path,
name = token.type_name(),
variants = token
.inners
.iter()
.map(|inner| format!("\t{},", inner.name))
.collect::<Vec<String>>()
.join("\n")
)
};

if buffer.has(&gen) {
return Ok(String::new());
Expand Down
109 changes: 101 additions & 8 deletions crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,39 @@
use cainome::parser::tokens::{Composite, CompositeType};
use cainome::parser::tokens::{Composite, CompositeType, Token};

use super::JsType;
use super::constants::{BIGNUMNERISH_IMPORT, CAIRO_OPTION_IMPORT, SN_IMPORT_SEARCH};
use super::{token_is_option, JsType};
use crate::error::BindgenResult;
use crate::plugins::typescript::generator::constants::CAIRO_OPTION_TOKEN;
use crate::plugins::{BindgenModelGenerator, Buffer};

const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';";

pub(crate) struct TsInterfaceGenerator;
impl TsInterfaceGenerator {
fn check_import(&self, token: &Composite, buffer: &mut Buffer) {
// only search for end part of the import, as we append the other imports afterward
if !buffer.has("BigNumberish } from 'starknet';") {
buffer.push(BIGNUMNERISH_IMPORT.to_owned());
}

// type is Option, need to import CairoOption
if token_is_option(token) {
// we directly add import if 'starknet' import is not present
if !buffer.has(SN_IMPORT_SEARCH) {
buffer.push(CAIRO_OPTION_IMPORT.to_owned());
} else if !buffer.has(CAIRO_OPTION_TOKEN) {
// If 'starknet' import is present, we add CairoOption to the imported types
buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
Comment on lines +22 to +26
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential Panic when Using buffer.insert_after without Checking Position

Ohayo, sensei! In the check_import method, when SN_IMPORT_SEARCH is present, the buffer.insert_after method is used without ensuring that SN_IMPORT_SEARCH exists at a valid position. This may lead to a panic if the expected string is not found. It's advisable to check the result of buffer.pos(SN_IMPORT_SEARCH) before inserting to prevent unexpected panics.

Apply this diff to handle the Option safely:

- buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
+ if let Some(_) = buffer.pos(SN_IMPORT_SEARCH) {
+     buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
+ } else {
+     // Handle the case where SN_IMPORT_SEARCH is not found
+     // For example, insert CAIRO_OPTION_IMPORT at a default position or log an error
+ }

Committable suggestion skipped: line range outside the PR's diff.

}
}
}
}

impl BindgenModelGenerator for TsInterfaceGenerator {
fn generate(&self, token: &Composite, buffer: &mut Buffer) -> BindgenResult<String> {
if token.r#type != CompositeType::Struct || token.inners.is_empty() {
return Ok(String::new());
}

if !buffer.has(BIGNUMNERISH_IMPORT) {
buffer.push(BIGNUMNERISH_IMPORT.to_owned());
}
self.check_import(token, buffer);

Ok(format!(
"// Type definition for `{path}` struct
Expand All @@ -30,7 +47,15 @@ export interface {name} {{
fields = token
.inners
.iter()
.map(|inner| { format!("\t{}: {};", inner.name, JsType::from(&inner.token)) })
.map(|inner| {
if let Token::Composite(composite) = &inner.token {
if token_is_option(composite) {
self.check_import(composite, buffer);
}
}

format!("\t{}: {};", inner.name, JsType::from(&inner.token))
})
.collect::<Vec<String>>()
.join("\n")
))
Expand Down Expand Up @@ -93,6 +118,21 @@ mod tests {
);
}

#[test]
fn test_check_import() {
let mut buff = Buffer::new();
let writer = TsInterfaceGenerator;
let token = create_test_struct_token();
writer.check_import(&token, &mut buff);
assert_eq!(1, buff.len());
let option = create_option_token();
writer.check_import(&option, &mut buff);
assert_eq!(1, buff.len());
let custom_enum = create_custom_enum_token();
writer.check_import(&custom_enum, &mut buff);
assert_eq!(1, buff.len());
}

fn create_test_struct_token() -> Composite {
Composite {
type_path: "core::test::TestStruct".to_owned(),
Expand Down Expand Up @@ -122,4 +162,57 @@ mod tests {
alias: None,
}
}

fn create_option_token() -> Composite {
Composite {
type_path: "core::option::Option<core::felt252>".to_owned(),
inners: vec![CompositeInner {
index: 0,
name: "value".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic { type_path: "core::felt252".to_owned() }),
}],
generic_args: vec![],
r#type: CompositeType::Struct,
is_event: false,
alias: None,
}
}
fn create_custom_enum_token() -> Composite {
Composite {
type_path: "core::test::CustomEnum".to_owned(),
inners: vec![
CompositeInner {
index: 0,
name: "Variant1".to_owned(),
kind: CompositeInnerKind::NotUsed,
token: Token::CoreBasic(CoreBasic { type_path: "core::felt252".to_owned() }),
},
CompositeInner {
index: 1,
name: "Variant2".to_owned(),
kind: CompositeInnerKind::NotUsed,
token: Token::Composite(Composite {
type_path: "core::test::NestedStruct".to_owned(),
inners: vec![CompositeInner {
index: 0,
name: "nested_field".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::felt252".to_owned(),
}),
}],
generic_args: vec![],
r#type: CompositeType::Struct,
is_event: false,
alias: None,
}),
},
],
generic_args: vec![],
r#type: CompositeType::Enum,
is_event: false,
alias: None,
}
}
}
104 changes: 103 additions & 1 deletion crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use cainome::parser::tokens::{Composite, Token};
use crate::plugins::typescript::generator::constants::CAIRO_OPTION_TYPE_PATH;
use cainome::parser::tokens::{Composite, CompositeType, Token};
use constants::{
CAIRO_BOOL, CAIRO_BYTE_ARRAY, CAIRO_CONTRACT_ADDRESS, CAIRO_FELT252, CAIRO_I128, CAIRO_U128,
CAIRO_U16, CAIRO_U256, CAIRO_U256_STRUCT, CAIRO_U32, CAIRO_U64, CAIRO_U8, JS_BIGNUMBERISH,
Expand Down Expand Up @@ -55,6 +56,21 @@ pub(crate) fn generate_type_init(token: &Composite) -> String {
)
}

/// Checks if Token::Composite is an Option
/// * token - The token to check
///
pub(crate) fn token_is_option(token: &Composite) -> bool {
token.type_path.starts_with(CAIRO_OPTION_TYPE_PATH)
}

/// Checks if Token::Composite is an custom enum (enum with nested Composite types)
/// * token - The token to check
///
pub(crate) fn token_is_custom_enum(token: &Composite) -> bool {
token.r#type == CompositeType::Enum
&& token.inners.iter().any(|inner| inner.token.to_composite().is_ok())
}

#[derive(Debug)]
pub(crate) struct JsType(String);
impl From<&str> for JsType {
Expand Down Expand Up @@ -93,6 +109,16 @@ impl From<&Token> for JsType {
)
.as_str(),
),
Token::Composite(c) => {
if token_is_option(c) {
return JsType::from(format!("CairoOption<{}>", c.generic_args.iter().map(|(_, t)| JsType::from(t.type_name().as_str()).to_string()).collect::<Vec<_>>().join(", ")).as_str());
}
if token_is_custom_enum(c) {
// we defined a type wrapper with Enum suffix let's use it there
return JsType::from(format!("{}Enum", value.type_name()).as_str());
}
return JsType::from(value.type_name().as_str())
},
_ => JsType::from(value.type_name().as_str()),
}
}
Expand Down Expand Up @@ -244,6 +270,82 @@ mod tests {
)
}

#[test]
fn test_option_type() {
assert_eq!(
"CairoOption<GatedType>",
JsType::from(&Token::Composite(Composite {
type_path: "core::option::Option<tournament::ls15_components::models::tournament::GatedType>".to_owned(),
inners: vec![],
generic_args: vec![
(
"A".to_owned(),
Token::Composite(
Composite {
type_path: "tournament::ls15_components::models::tournament::GatedType".to_owned(),
inners: vec![
CompositeInner {
index: 0,
name: "token".to_owned(),
kind: CompositeInnerKind::NotUsed,
token: Token::Composite(
Composite {
type_path: "tournament::ls15_components::models::tournament::GatedToken".to_owned(),
inners: vec![],
generic_args: vec![],
r#type: CompositeType::Unknown,
is_event: false,
alias: None,
},
),
},
CompositeInner {
index: 1,
name: "tournament".to_owned(),
kind: CompositeInnerKind::NotUsed,
token: Token::Array(
Array {
type_path: "core::array::Span::<core::integer::u64>".to_owned(),
inner: Box::new(Token::CoreBasic(
CoreBasic {
type_path: "core::integer::u64".to_owned(),
},
)),
is_legacy: false,
},
),
},
CompositeInner {
index: 2,
name: "address".to_owned(),
kind: CompositeInnerKind::NotUsed,
token: Token::Array(
Array {
type_path: "core::array::Span::<core::starknet::contract_address::ContractAddress>".to_owned(),
inner: Box::new(Token::CoreBasic(
CoreBasic {
type_path: "core::starknet::contract_address::ContractAddress".to_owned(),
},
)) ,
is_legacy: false,
},
),
}
],
generic_args: vec![],
r#type: CompositeType::Unknown,
is_event: false,
alias: None
}
)
),
],
r#type: CompositeType::Unknown,
is_event: false,
alias: None }))
)
}

#[test]
fn test_default_value_basics() {
assert_eq!(
Expand Down
3 changes: 3 additions & 0 deletions crates/dojo/bindgen/src/plugins/typescript/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ impl BindgenWriter for TsFileWriter {
.iter()
.fold(Buffer::new(), |mut acc, g| {
composites.iter().for_each(|c| {
// println!("Generating code for model {}", c.type_path);
// println!("{:#?}", c);
// println!("=====================");
Comment on lines +54 to +56
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo sensei! Consider using proper logging instead of commented-out print statements.

Replace the commented debug prints with proper logging using the log crate's macros for better debugging capabilities.

-    // println!("Generating code for model {}", c.type_path);
-    // println!("{:#?}", c);
-    // println!("=====================");
+    log::debug!("Generating code for model {}", c.type_path);
+    log::trace!("Model details: {:#?}", c);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// println!("Generating code for model {}", c.type_path);
// println!("{:#?}", c);
// println!("=====================");
log::debug!("Generating code for model {}", c.type_path);
log::trace!("Model details: {:#?}", c);

match g.generate(c, &mut acc) {
Ok(code) => {
if !code.is_empty() {
Expand Down