Allow structural recursion in type aliases #1207
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e38e5e7 in 53 seconds
More details
- Looked at
286lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:20
- Draft comment:
Consider usingcollectinstead ofHashMap::from_iterfor better readability and idiomatic Rust.
let structural_type_aliases: HashMap<_, _> = ctx.db.walk_type_aliases().map(|alias| {
- Reason this comment was not posted:
Confidence changes required:50%
The comment is about the use ofHashMap::from_iterwhich can be replaced withcollectfor better readability and idiomatic Rust.
2. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:50
- Draft comment:
Consider usingcollectinstead ofHashMap::from_iterfor better readability and idiomatic Rust.
let class_dependency_graph: HashMap<_, _> = ctx.db.walk_classes().map(|class| {
- Reason this comment was not posted:
Confidence changes required:50%
The comment is about the use ofHashMap::from_iterwhich can be replaced withcollectfor better readability and idiomatic Rust.
3. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:128
- Draft comment:
Consider refactoring this function to use a struct for managing state, as there are too many parameters being passed. This will improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The functioninsert_required_class_depshas a TODO comment about using a struct to manage state due to too many parameters. This is a valid concern and should be addressed to improve code maintainability.
4. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/cycle.rs:213
- Draft comment:
Consider refactoring this function to use a struct for managing state, similar to the suggestion forinsert_required_class_deps. This will improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The functioninsert_required_alias_depsis similar toinsert_required_class_depsand could also benefit from using a struct to manage state.
5. engine/baml-lib/parser-database/src/lib.rs:184
- Draft comment:
Consider including the actualdepvalue in the panic message for better debugging information.
panic!("Unknown class `{}`", dep)
- Reason this comment was not posted:
Confidence changes required:50%
The panic messages infinalize_dependenciescould be more informative by including the actualdepvalue that caused the panic.
6. engine/baml-lib/parser-database/src/lib.rs:188
- Draft comment:
Consider including the actualdepvalue in the panic message for better debugging information.
panic!("Unknown class `{}`", dep)
- Reason this comment was not posted:
Confidence changes required:50%
The panic messages infinalize_dependenciescould be more informative by including the actualdepvalue that caused the panic.
Workflow ID: wflow_P4qjyYzxcUhJMV6L
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 794b3f4 in 40 seconds
More details
- Looked at
100lines of code in4files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/types/mod.rs:284
- Draft comment:
Ensure that all references tostructural_recursive_type_aliasesare updated tostructural_recursive_alias_cyclesfor consistency. - Reason this comment was not posted:
Confidence changes required:50%
The renaming ofstructural_recursive_type_aliasestostructural_recursive_alias_cyclesintypes/mod.rsshould be consistent across the codebase. Ensure that all references are updated accordingly.
Workflow ID: wflow_2YiAGGktzCIRmBOI
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -138,6 +138,11 @@ impl ParserDatabase { | |||
| self.types.resolved_type_aliases.insert(*alias_id, resolved); | |||
| } | |||
|
|
|||
| // Cycles left here after cycle validation are allowed. Basically lists | |||
There was a problem hiding this comment.
The comment here is misleading. It states that cycles left after validation are allowed, but the code actually rebuilds the cycles for rendering purposes. Consider updating the comment to reflect the actual behavior.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9869707 in 31 seconds
More details
- Looked at
102lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_k6DHP92C4hBCSdkt
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| // TODO: @antoniosarosi: HashSet is random, won't always iterate in the | ||
| // same order. Fix this with IndexSet or something, we really don't want | ||
| // to sort this every single time. | ||
| let mut successors = Vec::from_iter(&self.graph[&node_id]); |
There was a problem hiding this comment.
Consider using IndexSet instead of HashSet for successors to maintain a consistent iteration order without needing to sort repeatedly.
antoniosarosi
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fab92f5 in 37 seconds
More details
- Looked at
1195lines of code in24files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integ-tests/typescript/tests/integ-tests.test.ts:171
- Draft comment:
The test case 'single optional string' is missing an assertion. Consider adding an assertion to verify the expected behavior. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_idjBdrhuZjILUbPJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 18c1bde in 18 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/lib.rs:329
- Draft comment:
The change fromlet mut dbtolet dbis appropriate here sincedbis not modified after initialization. This aligns with Rust's best practices for immutability. - Reason this comment was not posted:
Confidence changes required:0%
The change fromlet mut dbtolet dbin theassert_finite_cyclesfunction is unnecessary sincedbis not modified after its initialization. This change is correct and aligns with Rust's best practices for immutability.
Workflow ID: wflow_Xqma6UVtkZq3GKJT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c09997a in 1 minute and 17 seconds
More details
- Looked at
3554lines of code in81files - Skipped
4files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. integ-tests/typescript/test-report.html:263
- Draft comment:
The test failure here is due to a missing service account file, which is likely an issue with the test environment setup rather than the code changes in this PR. - Reason this comment was not posted:
Confidence changes required:50%
The test report indicates two failed tests. One is due to a missing service account file, and the other is due to a connection refused error. These issues are likely related to the test environment setup rather than the code changes in the PR.
2. integ-tests/typescript/test-report.html:265
- Draft comment:
The test failure here is due to a connection refused error, which is likely an issue with the test environment setup rather than the code changes in this PR. - Reason this comment was not posted:
Confidence changes required:50%
The test report indicates two failed tests. One is due to a missing service account file, and the other is due to a connection refused error. These issues are likely related to the test environment setup rather than the code changes in the PR.
Workflow ID: wflow_nK4laF4giOXp81U3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 72ea8cb in 31 seconds
More details
- Looked at
1290lines of code in29files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integ-tests/typescript/baml_client/sync_client.ts:2146
- Draft comment:
Ensure that theSimpleRecursiveMapAliasfunction is correctly implemented and follows the pattern used in other similar functions. Verify that the input and output types are correctly defined and that the function handles errors appropriately. - Reason this comment was not posted:
Confidence changes required:50%
The functionSimpleRecursiveMapAliasis newly added in this PR. It is important to ensure that the function is correctly implemented and follows the pattern used in other similar functions.
Workflow ID: wflow_O04zlPHOFObJRnMX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on abb7430 in 38 seconds
More details
- Looked at
799lines of code in17files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integ-tests/typescript/baml_client/sync_client.ts:1571
- Draft comment:
Ensure that theJsonTypeAliasCyclefunction correctly handles theJsonValuetype for both input and output. The error handling logic should be consistent with other functions in this file. - Reason this comment was not posted:
Confidence changes required:50%
The functionJsonTypeAliasCycleis newly added in this PR. It is important to ensure that the function signature and implementation are correct. The function should handle the input and output types correctly, and the error handling should be consistent with other functions in the file.
Workflow ID: wflow_YDjIdt8Nxh8pEZv4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c4e8b85 in 32 seconds
More details
- Looked at
608lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:227
- Draft comment:
Theis_subtypefunction has a potential performance issue due to its O(n) complexity when handling recursive type aliases. Consider optimizing this to improve performance. - Reason this comment was not posted:
Confidence changes required:50%
Theis_subtypefunction has a potential performance issue due to its O(n) complexity when handling recursive type aliases. This could be optimized to improve performance.
2. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:43
- Draft comment:
Thecoerce_argfunction has a potential performance issue due to its O(n) complexity when handling recursive type aliases. Consider optimizing this to improve performance. - Reason this comment was not posted:
Confidence changes required:50%
Theis_subtypefunction inmod.rsand thecoerce_argfunction into_baml_arg.rsboth have a TODO comment about O(n) complexity. This indicates a potential performance issue that should be addressed.
Workflow ID: wflow_3r3kmKJKK0vcdLB4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on d6b1e9e in 43 seconds
More details
- Looked at
63lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_IozJLzp3syXuO4Rk
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| expect(res).toEqual({ one: { two: { three: {} } } }) | ||
| }) | ||
|
|
||
| it('simple recursive map alias', async () => { |
There was a problem hiding this comment.
Duplicate test case name 'simple recursive map alias'. Consider renaming for clarity.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c5267b5 in 3 minutes and 7 seconds
More details
- Looked at
42lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integ-tests/typescript/test-report.html:263
- Draft comment:
The test 'json type alias cycle' failed, indicating a potential issue with the handling of recursive type aliases. Please investigate the cause of the discrepancy between the expected and received values. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_XEyOmGGQRgg5zVq1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cac1a16 in 2 minutes and 23 seconds
More details
- Looked at
77lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/jsonish/src/tests/test_aliases.rs:41
- Draft comment:
The test nametest_json_without_nested_objectsshould be more descriptive, e.g.,test_recursive_alias_without_nested_objects. This applies to other test names as well, such astest_json_with_nested_listandtest_json_with_nested_object. - Reason this comment was not posted:
Confidence changes required:50%
The test names should be descriptive and consistent with the test content.
Workflow ID: wflow_ZzoTCo5pmi0oumGr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 39141cb in 1 minute and 7 seconds
More details
- Looked at
149lines of code in6files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_I35CvUUMutg0SHfC
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| type RecAliasThree = RecAliasOne[] | ||
|
|
||
| type JsonValue = number | string | boolean | number | JsonObject | JsonArray |
There was a problem hiding this comment.
The type alias JsonValue includes number twice, which is redundant. Consider removing the duplicate number.
| type JsonValue = number | string | boolean | number | JsonObject | JsonArray | |
| type JsonValue = number | string | boolean | JsonObject | JsonArray |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 401a97d in 48 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/language_client_codegen/src/ruby/field_type.rs:14
- Draft comment:
Consider documenting the limitation of usingT.anythingforRecursiveTypeAliasdue to Sorbet's lack of support for recursive type aliases. This will help future developers understand the reasoning behind this choice. - Reason this comment was not posted:
Confidence changes required:50%
The implementation forFieldType::RecursiveTypeAliasreturnsT.anything, which is a placeholder and may not be the best representation for recursive type aliases in Ruby. However, given the comment that Sorbet does not support recursive type aliases, this might be the best workaround for now. It's important to note this limitation in the documentation or comments for future reference.
Workflow ID: wflow_fO5cZFTz09QoWC9h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fc25050 in 53 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/language_client_codegen/src/openapi.rs:583
- Draft comment:
The error message could be more specific by including the unsupported key type:
anyhow::bail!(format!("BAML<->OpenAPI only supports strings, enums, and literal strings as map keys but got an unsupported key type: {key:?}"))
- Reason this comment was not posted:
Confidence changes required:50%
The error message should be more specific about the unsupported key type.
Workflow ID: wflow_z3xBtF5zYnqDlksr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8ff0397 in 3 minutes and 57 seconds
More details
- Looked at
285lines of code in7files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. engine/baml-lib/jsonish/src/deserializer/coercer/field_type.rs:32
- Draft comment:
Remove theeprintln!macro as it is used for debugging purposes and should not be present in production code. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_wKc9g7q5tmVnjuXf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -166,6 +166,7 @@ fn test_union_of_map_and_class() { | |||
| assert!(result.is_ok(), "Failed to parse: {:?}", result); | |||
|
|
|||
| let value = result.unwrap(); | |||
| dbg!(&value); | |||
There was a problem hiding this comment.
Remove the dbg! macro as it is meant for debugging and should not be present in production code.
| } | ||
|
|
||
| eprintln!("ret_v: {:?}", ret_v); |
There was a problem hiding this comment.
Remove the eprintln! macro as it is used for debugging purposes and should not be present in production code.
Allow structural recursion (like TS):
Follow up on #1163
Important
Enable structural recursion in type aliases by updating cycle validation logic, adding tests, and supporting recursive type aliases in Python, Ruby, and TypeScript clients.
cycle.rs.validate()incycle.rsto handle structural recursion and report cycles.ParserDatabase::finalize_dependencies()inlib.rsto resolve type aliases and handle cycles.insert_required_alias_deps()incycle.rsto handle alias dependencies.insert_required_deps()toinsert_required_class_deps()incycle.rs.structural_recursive_alias_cyclestoTypesinmod.rs.recursive_type_aliases.bamlto reflect new cycle validation behavior.lib.rsto verify structural recursion handling.is_subtype()toIRHelperinmod.rsto support subtyping checks.coerce_alias()incoerce_alias.rsto handle alias coercion.integ-tests/typescript/tests/integ-tests.test.ts.This description was created by
for 8ff0397. It will automatically update as commits are pushed.