Remove Walkers from IR
#2172
Open
Remove Walkers from IR
#2172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to aeeda12 in 1 minute and 41 seconds. Click for details.
- Reviewed
834lines of code in15files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/generators/languages/typescript/src/ir_to_ts/type_aliases.rs:54
- Draft comment:
Review the union conversion branch: the code iterates overunion_type.iter_skip_null()and immediately returns on the first map variant. Confirm that this early return is the desired behavior when multiple map variants occur. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/generators/languages/typescript/src/lib.rs:360
- Draft comment:
The regex used inadd_js_suffix_to_importsis quite complex. Please consider adding inline comments that explain each captured group and the overall pattern to aid future maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/language_server/src/baml_project/mod.rs:314
- Draft comment:
The caching mechanism in the runtime() function (using file contents and env vars for hashing) is well thought‐out. Consider clarifying or improving error propagation when the runtime is not successfully generated. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/generators/languages/go/src/ir_to_go/type_aliases.rs:12
- Draft comment:
Consider refactoring the drop_type check using anif let Some(dt) = drop_typepattern for clarity, rather than checkingis_some()and then unwrapping. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. engine/generators/languages/go/src/lib.rs:48
- Draft comment:
The use of the newlet Some(go_mod_name) = ... else { ... }syntax is idiomatic. Ensure the error message clearly indicates that the Go client package name is required. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. engine/generators/languages/python/src/ir_to_py/type_aliases.rs:10
- Draft comment:
The conversion of a type alias to a Python type alias is clear and consistent. No changes needed, but maintain the consistent cloning of docstrings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. engine/generators/languages/ruby/src/lib.rs:65
- Draft comment:
The file wrapping logic in on_file_finished is nicely structured. Consider extracting the module-wrapping (adding 'module BamlClient' and the indenting) into a helper function to improve readability and reusability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. engine/generators/languages/typescript/src/lib.rs:350
- Draft comment:
The ESM transformation block that modifies file imports is a useful addition. Double-check that the regex logic only affects relative paths and does not inadvertently alter external module imports. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. engine/language_server/src/baml_project/mod.rs:95
- Draft comment:
The logic for gathering files and creating document keys is robust. It may help to add more detailed logging for cases where file reading fails, to ease debugging in production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. engine/baml-lib/baml-core/src/ir/ir_hasher/type_alias.rs:10
- Draft comment:
Typographical error: "thigns" should be corrected to "things". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_G4M0FytwY0bfZJlX
You can customize 
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d12abc4 in 1 minute and 41 seconds. Click for details.
- Reviewed
1252lines of code in24files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/generators/languages/openapi/src/generate_types.rs:43
- Draft comment:
Sorting keys via sort_keys() ensures deterministic output; consider adding a comment to explain its intent and to document any assumptions about key ordering. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is suggesting adding a comment to explain the intent of using sort_keys() and to document assumptions about key ordering. This is asking the author to document their code, which is not allowed by the rules. The comment is not making a specific code suggestion or pointing out a potential issue with the code itself.
2. engine/generators/languages/ruby/src/ir_to_rb/classes.rs:88
- Draft comment:
Several tests in this module are marked #[ignore]. Consider fixing and enabling these tests to ensure full coverage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/generators/languages/typescript/src/lib.rs:362
- Draft comment:
The regex-based function add_js_suffix_to_imports is quite complex; consider adding more inline comments or refactoring for clarity, and include additional tests for various relative import edge cases. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/language_client_codegen/src/lib.rs:77
- Draft comment:
There is a large commented-out block in the version check section. Consider removing stale commented code to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. engine/generators/languages/typescript/src/ir_to_ts/classes.rs:100
- Draft comment:
Test assertions here check streaming behavior via meta(). Consider expanding the test cases to cover more edge cases of streaming type conversion for robustness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. engine/generators/languages/ruby/src/lib.rs:87
- Draft comment:
The on_file_finished hook wraps file content in a module and injects require/require_relative statements. Consider adding inline documentation explaining the rationale behind this design and how the relative paths are determined. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. engine/generators/languages/typescript/src/lib.rs:125
- Draft comment:
The processing of recursive alias cycles and synthetic type alias generation is complex. Consider refactoring this logic into smaller helper functions to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. engine/baml-lib/baml-core/src/ir/ir_hasher/class.rs:10
- Draft comment:
Typographical error: "thigns" should be "things". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. engine/baml-lib/baml-core/src/ir/repr.rs:2968
- Draft comment:
Typo in comment: "doesnt" should be "doesn't". - Reason this comment was not posted:
Comment was on unchanged code.
10. engine/baml-lib/jsonish/src/tests/mod.rs:241
- Draft comment:
Typographical error: There appears to be a double space in the description text ('book names provided'). Please fix to use a single space. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_B1ExJ7k4vKqXLgBF
You can customize
antoniosarosi
force-pushed
the
antonio/remove-type-alias-ir-walker
branch
from
d12abc4 to
5fb641d
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5fb641d in 1 minute and 50 seconds. Click for details.
- Reviewed
1306lines of code in24files - Skipped
0files when reviewing. - Skipped posting
13draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/generators/languages/typescript/src/lib.rs:362
- Draft comment:
Consider caching the regex in 'add_js_suffix_to_imports' to avoid recompilation on every call. Using lazy initialization (e.g. once_cell or lazy_static) could improve performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/generators/languages/ruby/src/ir_to_rb/classes.rs:88
- Draft comment:
Tests in this file are marked with #[ignore]. It might be beneficial to re-enable these tests once the underlying functionality stabilizes, to catch any regressions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/generators/languages/python/src/lib.rs:52
- Draft comment:
The default for 'is_pydantic_2' is set to true using unwrap_or(true). Ensure this behavior is clearly documented since it directly affects the generated client code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/generators/languages/ruby/src/lib.rs:69
- Draft comment:
The 'on_file_finished' function concatenates require and require_relative statements and wraps content in a module. Consider refactoring for improved readability and possibly reducing redundant locking. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. engine/generators/languages/openapi/src/generate_types.rs:45
- Draft comment:
The use of 'types.sort_keys()' on an IndexMap is unusual. Verify that the sorting behavior is deterministic and documented, since it impacts the generated OpenAPI schema. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. engine/language_client_codegen/src/lib.rs:79
- Draft comment:
There is a block of commented-out code for supporting additional generator types. Consider removing or clearly annotating this code as work-in-progress to reduce clutter. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
7. engine/generators/languages/typescript/src/ir_to_ts/classes.rs:95
- Draft comment:
Ensure that the 'wrap_stream_state' flag in the metadata is well documented, as tests rely on it for streaming fields. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking for documentation of a specific flag, which is not allowed as it falls under ensuring behavior is intended or tested. It doesn't provide a specific code suggestion or ask for a test to be written.
8. engine/generators/languages/go/src/lib.rs:48
- Draft comment:
Consider providing more contextual information in the error message when the Go client package name is missing, to aid debugging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
9. engine/generators/languages/python/src/ir_to_py/classes.rs:12
- Draft comment:
Docstring extraction uses '.map(|docstring| docstring.0.clone())'. Verify that this pattern is consistent across all generators to maintain uniform documentation output. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
10. engine/generators/languages/typescript/src/lib.rs:360
- Draft comment:
The ESM transformation in 'add_js_suffix_to_imports' is clever; consider adding additional inline documentation and edge-case tests to help future maintainers understand the regex logic. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
11. engine/baml-lib/baml-core/src/ir/ir_hasher/class.rs:10
- Draft comment:
There appears to be a typographical error: "thigns" should likely be "things". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. engine/baml-lib/baml-core/src/ir/repr.rs:2968
- Draft comment:
Typo: In the comment, "doesnt" should be corrected to "doesn't" to ensure proper punctuation. - Reason this comment was not posted:
Comment was on unchanged code.
13. engine/baml-lib/jsonish/src/tests/mod.rs:241
- Draft comment:
Typographical error: There appears to be an extra space in the description string "The list of book names provided". Consider removing the redundant space to read "The list of book names provided". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_gn2AiP8jeUNC56hm
You can customize
antoniosarosi
commented
Jul 19, 2025
Comment on lines
+1399
to
+1419
| impl<T> Node<T> { | ||
| pub fn span(&self) -> Option<&ast::Span> { | ||
| self.attributes.span.as_ref() | ||
| } | ||
|
|
||
| pub fn alias(&self, ctx: &EvaluationContext<'_>) -> Result<Option<String>> { | ||
| self.attributes.alias().map(|v| v.resolve(ctx)).transpose() | ||
| } | ||
|
|
||
| pub fn description(&self, ctx: &EvaluationContext<'_>) -> Result<Option<String>> { | ||
| self.attributes | ||
| .description() | ||
| .map(|v| v.resolve(ctx)) | ||
| .transpose() | ||
| } | ||
|
|
||
| pub fn streaming_behavior(&self) -> StreamingBehavior { | ||
| self.attributes.streaming_behavior() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These are generic to all node types. Don't have to be copy pasted on every single walker.
antoniosarosi
commented
Jul 19, 2025
Comment on lines
-315
to
-325
| pub fn alias(&self, ctx: &EvaluationContext<'_>) -> Result<Option<String>> { | ||
| self.item | ||
| .attributes | ||
| .alias() | ||
| .map(|v| v.resolve(ctx)) | ||
| .transpose() | ||
| } | ||
|
|
||
| pub fn streaming_behavior(&self) -> StreamingBehavior { | ||
| self.item.attributes.streaming_behavior() | ||
| } |
There was a problem hiding this comment.
These only access the attributes field of Node. Can live under Node<T> impl.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Remove
Walkerabstraction from IR, updating class and type alias handling across code generation and validation modules.Walkerabstraction from IR, replacing with directNodeaccess inclass.rs,type_alias.rs, andmod.rs.IRHelperandIRHelperExtendedto useNodedirectly instead ofWalker.Nodefor class and type alias handling.ir_to_go,ir_to_py,ir_to_rb, andir_to_tsmodules to reflectWalkerremoval.expr_typecheck.rsandjson_schema.rsto work withNode.baml_value_to_jinja_value.rsandhelpers/mod.rsforNodeusage.Walkertype definitions fromir_helpers/mod.rs.Nodechanges.This description was created by
for 5fb641d. You can customize this summary. It will automatically update as commits are pushed.