Migrate binary_tree module to Conductor#790
Conversation
Rewrites the binary_tree bundle as a Conductor BaseModulePlugin, backed by IDataHandler pair/list primitives instead of js-slang's stdlib. Tree entries are stored as OPAQUE values at the module boundary; is_tree and is_empty_tree declare no arg type so they can accept any DataType and answer false rather than throw, matching their predicate semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the binary_tree bundle from js-slang to @sourceacademy/conductor, refactoring the tree operations, types, and tests to utilize Conductor's IDataHandler and TypedValue system, and exporting the module via a new BinaryTreeModulePlugin class. The review feedback suggests enhancing robustness by adding null/undefined checks for the value parameter in is_tree, is_empty_tree, and assertNonEmptyTree to prevent runtime crashes. Additionally, it is recommended to throw EvaluatorRuntimeError instead of generic Error objects for arity mismatches in the module plugin methods.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
is_tree/is_empty_tree/assertNonEmptyTree now guard against a missing/undefined value instead of crashing with a raw TypeError, and the plugin's arity-check throws now use EvaluatorRuntimeError (already used elsewhere in this module) instead of a generic Error. Also adds the same __bindExportedMethods() workaround repeat/rune use for the unbound-method issue in BaseModulePlugin.initialise() (conductor#41), since this fix isn't merged upstream yet.
|
@coderabbitai, any review comments? |
|
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughThe binary tree module is migrated from synchronous native structures to Conductor ChangesBinary Tree Conductor Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
master's binary_tree gained this validation while conductor-migration was diverging (make_tree(0, 0, null) previously constructed a malformed tree silently instead of throwing). Ports the same check, using EvaluatorTypeError to match this module's existing Conductor error style rather than modules-lib's InvalidParameterTypeError, which doesn't apply here since this module no longer goes through js-slang.
AaravMalani
left a comment
AaravMalani
left a comment
There was a problem hiding this comment.
Yep, LGTM. The only thing left is documentation, but since that's still pending as per #765, I'm approving it in advance
| return await make_tree_func(this.evaluator, value, left, right); | ||
| } | ||
|
|
||
| // No declared arg type: is_tree is a predicate that must accept a value of any |
There was a problem hiding this comment.
Hmm, for this I think we should add a DataType.ANY which just accepts all values. Thoughts @Akshay-2007-1 @martin-henz?
There was a problem hiding this comment.
Yeah, that'd be cleaner than the current "just don't declare a type" convention for predicates like is_tree. It touches Conductor itself though.
There was a problem hiding this comment.
yes, is_tree should take ANY argument.
The upstream binding fix (source-academy/conductor#41) is merged and published, making the per-module bind workaround unnecessary. Also drops the constructor override, which was left as a pure passthrough to the base class once the workaround was removed.
repeat and testplugin depended on conductor via a bare, unpinned GitHub URL. yarn.lock had resolved and locked that to a commit from before even the BaseModulePlugin binding fix (conductor#41) - a real `yarn install` builds against that stale, broken conductor entirely silently, since nothing forces Yarn to re-resolve an already-locked git dependency. Other bundles (rune, etc.) already depend on the versioned npm release; switched these two to match (^0.7.0), which also resolves a duplicate-package TS error that showed up in any bundle depending on both specs simultaneously.
Was depending on conductor via a bare, unpinned GitHub URL, which yarn.lock had resolved and locked to a commit from before the BaseModulePlugin binding fix (conductor#41). Switched to the versioned npm range other bundles already use (^0.7.0), matching the same fix applied to midi.
Check this |
…igrate-binary-tree # Conflicts: # src/bundles/binary_tree/package.json # src/bundles/binary_tree/src/__tests__/index.test.ts # src/bundles/binary_tree/src/functions.ts # yarn.lock
… stricter tsconfig Picked up as part of merging conductor-migration in - noImplicitOverride is now enabled repo-wide, and exportedNames/channelAttach shadow members declared on BaseModulePlugin.
|
Re: the `is_tree`/`DataType.ANY` discussion above - opened source-academy/conductor#44, which adds `DataType.ANY` (a value of any type, expanding to the union of all concrete `TypedValue`s at the type level - same trick as `DataType.LIST` already does for `PAIR | EMPTY_LIST`) along with the supporting changes to `isSameType`/`isReferenceType`/`ExternTypeOf` so it doesn't fall through unhandled. Not wiring it into `is_tree`/`is_empty_tree` here yet since it depends on an unreleased conductor version - once #44 merges and a new version is published, updating this PR (or a quick follow-up) to declare `[DataType.ANY]` instead of the current `[]`/no-declared-type convention should be a small change. |
* Add DataType.ANY for predicate-style module method arguments Modules like binary_tree's is_tree need a predicate that accepts a value of any DataType and answers based on its shape, rather than throwing on a mismatched type - previously the only way to express this was to omit the argument's declared type entirely from a moduleMethod's signature. DataType.ANY makes this an explicit, self-documenting choice instead. TypedValue<DataType.ANY> expands to the union of all concrete TypedValues (mirroring the existing LIST -> PAIR|EMPTY_LIST special-case), and isSameType/isReferenceType/ExternTypeOf are updated so ANY doesn't fall through their lookups unhandled. Addresses Martin Henz and Aarav Malani's discussion on source-academy/modules#790 (review comment on is_tree's signature). * Check t1 === t2 before DataType.ANY in isSameType Per Gemini review on PR #44: identical types are the common case, so checking equality first avoids two extra ANY comparisons on that path.
3 participants
Description
Fixes #768
Migrates the
binary_treemodule to be Conductor-ready, following the pattern established by therepeatmodule (#698).binary_treehas no tab/visualization component, so this is scoped entirely tosrc/bundles/binary_tree/.functions.tsis rewritten against Conductor'sIDataHandler(pair_make/pair_head/pair_tail) instead of js-slang'sstdlib/list, so tree structure is built from real Conductor Pairs rather than native JS arrays.index.tsis now aBaseModulePluginsubclass with each exported function as a@moduleMethod-decorated closure, mirroringrepeat.DataType.OPAQUEat the module boundary (arbitrary Source/Python values aren't representable as a single concreteDataType).is_tree/is_empty_treedeclare no argument type, since they're predicates that must accept a value of any type and answerfalserather than throw — a declared type would mean the FFI boundary type-asserts and throws before the predicate body ever runs.tsconfig.jsonpicked up the sameexperimentalDecorators/emitDecoratorMetadataoverridesrepeatalready has (this bundle was missing them, which breaks themoduleMethoddecorator under TS's native Stage-3 decorators).Testing
yarn workspace @sourceacademy/bundle-binary_tree run tsc— passesyarn workspace @sourceacademy/bundle-binary_tree run lint— passesyarn workspace @sourceacademy/bundle-binary_tree run test— 16/16 passing, against@sourceacademy/modules-testplugin'sTestDataHandleryarn workspace @sourceacademy/bundle-binary_tree run build— producesbuild/bundles/binary_tree.jsPyCseEvaluator(via py-slang#217's module loader work) and drovemake_tree/entry/left_branch/right_branch/is_treefrom real Python-shaped values through the actualpythonToModule/moduleToPythonconversion code — all round-trip correctly.Notes for reviewers
Getting the end-to-end test working surfaced two bugs upstream of this PR, not fixable within
binary_treeitself:BaseModulePlugin.initialise()inconductorregisters each exported method as a detached function reference (this[name]) without binding it to the instance, sothisisundefinedinside any exported method body once a real evaluator calls it. This breaks every Conductor module that uses ordinary class methods (i.e. all of them), not just this one. Filing a fix againstconductorseparately.moduleToPython/pythonToModulein py-slang#217 flatten a returnedDataType.PAIRinto a Python list and convert Python lists back toDataType.ARRAY, which loses identity for any function returning a Pair that's meant to be passed back into another module call (e.g.left_branch(t)). Flagging this on that PR directly since the affected file is new there.