binary_tree: accept DataType.ARRAY as equally valid to PAIR for tree nodes#813
Open
Akshay-2007-1 wants to merge 1 commit into
Open
binary_tree: accept DataType.ARRAY as equally valid to PAIR for tree nodes#813Akshay-2007-1 wants to merge 1 commit into
Akshay-2007-1 wants to merge 1 commit into
Conversation
py-slang's module interface no longer has a distinct "pair" representation (source-academy/py-slang#307): pythonToModule now builds every Python list as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A tree built here via make_tree/pair_make and round-tripped out to Python (via moduleToPython) then passed back into entry()/left_branch()/right_branch() now arrives tagged ARRAY, not PAIR - is_tree/assertNonEmptyTree hardcoded `value.type !== DataType.PAIR` checks that read conductor's DataType tag directly, so this broke without a change here. Fixed by accepting either DataType.PAIR or DataType.ARRAY wherever a tree node's own tag is checked (isPairLike) - pair_head/pair_tail already read either shape identically (that's py-slang's own GenericDataHandler bridge), so this is purely about the validation, not the traversal. make_tree's own construction (pair_make calls) is unchanged - it always freshly builds a genuine PAIR for a node it creates itself; only validation of an incoming (possibly round-tripped) value needed to widen. NOTE: written and typechecked by inspection only - this worktree (feat/migrate-binary-tree) has no node_modules installed, and borrowing another worktree's (cross-branch, version-drifted @sourceacademy/conductor copies) produced unrelated pre-existing type errors in the test file that made a real yarn install/tsc/test run unreliable to interpret here. Needs a real `yarn install && yarn tsc && yarn test` pass in this bundle before merging.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
1 task
Contributor
|
What did you mean by you couldn't get the tests to run standalone? |
Contributor
Author
|
Follow-up on the test question: got a real, complete `yarn install` + bundle-scoped `tsc`/`test` run done in this exact worktree (no borrowed cross-branch node_modules this time). All 18 tests pass, including the new round-trip regression test. PR description updated to reflect this - dropping the "please verify in CI" caveat. |
Contributor
For the cross-branch thing, I assume you're talking about using |
Akshay-2007-1
added a commit
that referenced
this pull request
Jul 24, 2026
py-slang's module interface no longer has a distinct "pair" representation (source-academy/py-slang#307): pythonToModule now builds every Python list/ pair as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A Sound built here via soundToConductor and round-tripped out to Python (assigned to a variable, passed to another module call like get_wave/is_sound/play) then arrives tagged ARRAY, not PAIR - conductorToSound/is_sound hardcoded `value.type !== DataType.PAIR` checks broke on this, same class of bug as binary_tree's #813. Fixed the same way: isPairLike(value) accepts either DataType.PAIR or DataType.ARRAY wherever a Sound's own tag is checked - pair_head/pair_tail already read either shape identically (py-slang's GenericDataHandler bridge), so this is purely about validation, not traversal. Also fixed conductorListToSounds (consecutively/simultaneously's list argument) and stacking_adsr's envelopes list: a genuine Python list of any length now crosses as a flat ARRAY too, not just a 2-element pair, so the old PAIR-chain-only walk would silently return zero elements for a real multi-sound list. Added readListElements to read either an ARRAY (via array_length/array_get) or a PAIR/EMPTY_LIST chain, matching py-slang's own GenericDataHandler.readListElements pattern. soundToConductor's own construction (pair_make calls) is unchanged - it always freshly builds a genuine PAIR; only validation of an incoming (possibly round-tripped) value needed to widen.
martin-henz
pushed a commit
that referenced
this pull request
Jul 24, 2026
* Migrate midi module to Conductor Splits midi into a pure, evaluator-free functions.ts (unchanged signatures) and a Conductor-facing index.ts plugin, since sound and stereo_sound import midi_note_to_frequency and friends directly as plain TypeScript and sound/stereo_sound's own Source-facing APIs re-export several of these functions. Migrating index.ts's exports to require an IDataHandler would have broken both call sites immediately. - functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic - conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor list conversion, accidental validation) used by the plugin; kept separate from index.ts so they stay importable from vitest, which hits a decorator syntax error importing index.ts directly - index.ts: BaseModulePlugin subclass wrapping the pure functions; SHARP/FLAT/NATURAL are pushed onto `exports` directly in the constructor since BaseModulePlugin.initialise() only registers exportedNames that are functions - Includes the same __bindExportedMethods() workaround as repeat/rune/binary_tree for the unbound-method issue in BaseModulePlugin.initialise() (source-academy/conductor#41) - sound/stereo_sound's functions.ts and index.ts updated to import midi's pure functions from the new `/functions` subpath instead of the bundle root, which now exports the Conductor plugin * Address review feedback: use string literals instead of .name Function.prototype.name gets mangled under minification, which would turn these error messages into cryptic garbage like "t expects...". Two of these were carried over unchanged from the original module; the third is in the new Conductor-facing index.ts. * Port midi's master-only functions and validation conductor-migration's midi had drifted from master: 6 functions (is_note_with_octave, add_octave_to_note, get_octave, get_note_name, get_accidental, key_signature_to_key) and input validation on the existing ones (midi_note_to_frequency now range-checks its input) only existed on master, added there while this migration was in flight. Confirmed against the published docs page (source-academy.github.io/modules/documentation/modules/midi.html) that this is now the complete function/constant list, no more, no less - verified end-to-end through the actual compiled bundle, not just unit tests, driving every export exactly the way a real evaluator calls a closure (detached, not bound to any instance). Also fixes midi_note_to_letter_name/key_signature_to_key's accidental parameter to match master: it's the Accidental enum value ('#'/'b'), not the word 'flat'/'sharp' my first pass used before I'd found the drift. midi_note_to_letter_name silently treats anything other than exactly SHARP as flat (matching master's actual, slightly loose behavior) rather than validating it - key_signature_to_key is the one that validates, since its own switch has an explicit default case for that. Validation now uses conductor's new EvaluatorParameterTypeError / assertNumberWithinRange (source-academy/conductor#42) in place of modules-lib's InvalidParameterTypeError / assertNumberWithinRange, which functions.ts can't depend on without pulling js-slang's modules-lib re-exports (and everything under it) into sound/ stereo_sound's dependency graph transitively. Message format is unchanged - verified identical to master's existing test expectations. * midi: adapt to conductor's options-object assertNumberWithinRange signature * midi: fix scales' runtime js-slang dependency and address review feedback scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is unavailable under Conductor's module loader (the require() shim it's given is a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for any scale function. Build the intermediate list with a plain array tuple instead, since Pair<H, T> is just [H, T] structurally. Also addresses Aarav's review comments on #791: - Removes __bindExportedMethods now that the upstream binding fix (source-academy/conductor#41) is merged and published. - Widens letter_name_to_midi_note/midi_note_to_letter_name/ letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/ get_accidental/key_signature_to_key's note/accidental parameters to plain string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in index.ts. Doing this exposed two real validation gaps that the casts had been silently papering over: add_octave_to_note never validated `note` at all (just interpolated it into the result string), and midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP accidental as FLAT instead of rejecting it. Both now throw EvaluatorParameterTypeError for invalid input, with regression tests added. * midi: drop unnecessary js-slang runtime dependency, fix real dependency resolution scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type shape, not js-slang itself. Replaced with a local Scale type; js-slang moves to devDependencies since only the test suite still uses it. Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub URL, which yarn.lock had resolved and locked to a commit from before even the BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using a local portal: link for testing) was building against that stale, broken conductor entirely silently. Switched to the same versioned npm range other bundles already use (^0.7.0), and reverted the two calls to assertNumberWithinRange that had been written against conductor PR #43's still-unpublished options-object signature back to the options actually published in 0.7.0. * midi: add override modifiers and fix scale test type error Picked up as part of merging conductor-migration in - noImplicitOverride is now enabled repo-wide, and exportedNames/channelAttach shadow members declared on BaseModulePlugin. Also fixed a test that built its input scale via js-slang's untyped list() instead of midi's own js-slang-free Scale shape, which only surfaced as a real tsc error once the merge brought stricter checking back online. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: rebuild playback/recording on a self-contained Conductor channel Scraps the earlier plugins-repo sound-io plugin pair in favor of the pattern confirmed against rune's migration (PR #765): SoundModulePlugin declares its own channelAttach directly, and modules/src/tabs/Sound implements IPlugin/SoundTabRpc as the host-side counterpart, talking over Conductor's makeRpc helper - no separate runner/web plugin package needed at all. Also fixes plotly's draw_sound_2d, the one other consumer of bundle-sound's types, for the Wave type's redesign from a plain (t: number) => number to an async generator (so that stepping and nested user closures evaluate correctly on the CSE machine). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: unify with stereo_sound - one module, Sound is always stereo Retires stereo_sound as a separate bundle/tab entirely. Sound becomes {leftWave, rightWave, duration} always; "mono" isn't a separate type, it's just the common case where leftWave === rightWave (same reference), produced by make_sound. make_stereo_sound builds a genuinely stereo Sound from two different waves. Every combinator (consecutively/simultaneously/adsr/phase_mod/stacking_adsr/ instruments) now operates on this one shape via small per-channel helpers (joinWaves/sumWaves/adsrWave/phaseModWave), so composing a "mono" sound with a stereo one is a non-issue - there's nothing to convert, and none of stereo_sound's oscillator/envelope math is duplicated a second time the way it was as a separate bundle. get_wave/get_left_wave/get_right_wave: get_wave keeps meaning "the" wave (== left channel), so existing SICP-style code (play(sine_sound(...)), get_wave(sound)) keeps working unchanged for the common mono case. Adds a lower Wave-returning layer alongside the existing Sound layer (sine_wave/square_wave/triangle_wave/sawtooth_wave/noise_wave/ silence_wave), with sine_sound etc. as thin convenience wrappers (sine_sound = (freq, dur) => make_sound(sine_wave(freq), dur)). record()/record_for() use however many channels the input device actually has - a mono microphone (the common case) produces a Sound whose left and right channels are the same wave; no separate record_stereo. play/samplesToSound skip redoing work for a mono Sound (sampled once, not twice) by checking leftWave === rightWave. New stereo-specific operations: make_stereo_sound, play_waves, pan, pan_mod, squash, get_left_wave, get_right_wave. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: rebuild Sound/functions/index/tab for the sound+stereo_sound unification Continuation of the previous commit (which only picked up the stereo_sound/StereoSound deletions due to a failed multi-pathspec git add) - this is the actual Sound={leftWave,rightWave,duration} rework of the sound bundle and its tab described there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * align @sourceacademy/conductor pins with PR #795's catalog: convention #795 (fix/conductor-dependency-pin) is about to merge into conductor-migration first, establishing @sourceacademy/conductor in the yarn catalog and npmPreapprovedPackages. Switching midi/sound/ tab-Sound's package.json over to "catalog:" now (matching #795 verbatim) avoids re-doing this exact reconciliation the next time this branch merges conductor-migration in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: fix O(N) nested yield* delegation in consecutively/simultaneously Addresses Gemini's high-priority review comments on PR #796: reduce() was nesting joinWaves/sumWaves closures sounds.length deep, so sampling near the end of a long chain meant up to N nested async generator delegations per sample at 44100 Hz - real overhead for async generators that plain synchronous functions (the pre-migration implementation) never had. Rewritten as a flat scan that picks the active sound directly and yield*s into it once, keeping delegation depth O(1) regardless of how many sounds are combined, while still threading through user closures correctly. Also fixes a real (if narrow) NaN: linear_decay(0) computed 1 - 0/0 when release_ratio (or a future caller with decay_ratio) is exactly 0, corrupting the sample at that instant. Sample rate/instrument functions never happened to hit it because of how the surrounding branch conditions are structured, but adsr's release branch can. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * midi: align @sourceacademy/conductor pin with catalog: convention Matches the same fix already applied on #795 and #796: adds @sourceacademy/conductor to the yarn catalog and npmPreapprovedPackages, and switches midi's own package.json to "catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't leave midi on the old convention once #795 lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: address CodeRabbit review findings on PR #796 - validateDuration now rejects NaN/Infinity, not just negatives (both used to slip through and crash sampleWave's Float32Array allocation with a raw RangeError instead of a clean module error). - closureToWave validates the closure's result is actually a number before returning it - a student-supplied wave returning e.g. a string previously corrupted playback silently via `as number`. - stacking_adsr validates each envelope-list element is a closure before invoking it, for the same reason. - consecutively/simultaneously/adsr/phase_mod/conductorToSound all preserve the "mono means leftWave === rightWave" invariant again - building both channels independently (even from identical inputs) produced two behaviourally-identical-but-distinct waves, silently doubling sampling work and losing the fast path in play()/etc. - play()/stop() guard against a stale playSamples() completion clobbering a newer play()'s isPlaying state via a generation token (stop-A/start-B/late-settle-A ordering). - tab: requestMicPermission() disposes the previous MediaStream before re-requesting, so a denied re-request can't leave stale tracks running and reusable by startRecording(). - tab: startRecording() now actually awaits MediaRecorder's start event (and rejects on error) instead of resolving as soon as .start() returns, matching the SoundTabRpc contract. Not addressed (flagged as false positive or out of scope for a quick fix, not silently dropped): - The module doc's "a wave returns a number" description is correct as written - it's the student-facing Source-language contract, not the internal async-generator implementation detail (which is already documented separately on the Wave type in types.ts). - Overlapping/concurrent recording sessions (record/record_for only gate on isPlaying, not a dedicated recording-state flag) and pan/pan_mod sampling the shared source/modulator wave twice per channel are both real but non-trivial fixes requiring more substantial state-tracking/sampling-order changes; left for a follow-up rather than a rushed partial fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * sound: cross-reference Wave's internal async-generator contract in the module doc Partial concession on CodeRabbit's doc-comment finding: the primary description stays as the Source-facing "number -> number" contract (that's the actual, correct student experience - the CSE machine threads the async-generator machinery transparently), but adds a one-line pointer to where the internal TS contract is documented, for maintainers reading this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * midi: reuse parseNoteWithOctave as the canonical validator in add_octave_to_note Addresses CodeRabbit's review comment on PR #796 (surfaced there via shared branch history with sound, but the finding is midi's own): add_octave_to_note's inline regex accepted note spellings that noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb - accidentals that don't exist for those note names), and let lowercase input escape unnormalized through a type assertion. Now validates via parseNoteWithOctave (rejecting digits upfront, since that function alone would accept an octave already being present) and reconstructs using the normalized note name, preserving the original accidental spelling exactly as given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix playback/recording races and add ADSR validation, found via manual testing Manually testing all three migrated modules (binary_tree, midi, sound) together against a local frontend build surfaced several issues specific to sound, now fixed: - play() threw "Previous sound still playing!" on any overlapping or looped call. Repeated/looped play() calls now queue and play one after another instead (like consecutively, but built up call-by-call rather than pre-combined into one Sound). stop() cancels anything still queued behind the currently-playing sound, not just the current one. - The tab's "Constructing..." status (shown while play() samples a Wave into a buffer - a duration-proportional step that happens entirely before actual playback starts) never actually appeared, since the notification was fire-and-forget and could race the tab's own asynchronous loading. Now a real acknowledged RPC call, awaited before sampling begins. - init_record() was fire-and-forget: it kicked off the permission request but returned immediately, so calling record()/record_for() right after (the natural way to write it) could race the still- pending permission grant and throw even though permission had genuinely just been granted. init_record() now awaits the actual permission result before returning. - record()/record_for()'s returned "sound promise" threw "recording still being processed" until called again later, requiring manual polling. Both now return a promise that genuinely awaits the recording finishing processing instead. - adsr() had no validation that attack_ratio + decay_ratio + release_ratio stays within 1, or that each ratio/sustain_level is a finite number in [0, 1] - silently producing a discontinuous envelope instead of an error. Added validation, with the actual envelope-shaping logic split into an unvalidated adsrTransformer() used internally by piano/violin/cello/trombone/bell, so validating adsr() doesn't retroactively break trombone's second-harmonic envelope (ratios summing to 1.0236, a quirk present since before the Conductor migration - preserved rather than silently changed). * Move playback sequencing to the host tab; fix premature AudioContext teardown Found via extensive live testing against a local frontend build: - The Worker running a program is terminated as soon as the script finishes (conduit.terminate(), called after every Run to fix a worker-leak). play() is intentionally fire-and-forget, so a script can finish - and the Worker be killed - well before queued playback has actually started or finished. Sequencing that playback via a Worker-side queue (the previous design) meant anything still queued when the Worker died simply never got its playSamples() RPC sent at all. - functions.ts' play() now dispatches its playSamples() call immediately once sampling finishes, instead of waiting its turn in a local queue - so the RPC always gets sent before the Worker can be torn down. Actual sequencing (so playback doesn't overlap) moves to SoundTabPlugin on the host side, which outlives the Worker: it now owns its own playback queue and a stop-generation counter so a still-queued call correctly gets cancelled by stop(). - SoundTabPlugin.destroy() (called on every Run's teardown) no longer closes the AudioContext or unregisters the tab immediately - both are deferred until whatever's playing (or still queued) finishes naturally, tracked via a dedicated pending-playback counter rather than activeSources.size, which hits 0 momentarily between any one sound ending and the next queued one starting and was otherwise misread as "everything is done," silently killing the AudioContext mid-queue. - notifyConstructing()/playSamples() status updates are now recomputed from combined constructing+active-source state instead of set unconditionally, so an earlier sound finishing can't clobber a later sound's still-in-flight 'constructing' status. * sound: fast synchronous path for module-native waves, skip per-sample async overhead Wave is AsyncGenerator-based so a wave wrapping a user-supplied Conductor closure (closureToWave, in index.ts) can be driven through evaluator.closure_call_unchecked - itself an AsyncGenerator, since it steps the calling evaluator's CSE machine. But every wave built entirely from module-native math (oscillators, envelopes, instruments, and every combinator) got the same generator wrapper even though it never actually yields - and every AsyncGenerator#next() resolves via microtask regardless of whether anything inside really awaits. sampleWave calls a wave once per sample (44100 times per second of audio), so a multi-second Sound made of nothing but built-in instruments was paying real async overhead - measured as the dominant cost of play() for a ~21s cello passage - for zero actual asynchrony. Adds Wave.sync: an optional plain (t: number) => number twin, present iff a wave provably never needs to cross into user code. syncWave() builds both forms from one computation. Every combinator (clipToDuration, consecutiveWave, simultaneousWave, adsrWave, phaseModWave, gainWave, squash, panModAmountWave, pan_mod, interpolatedWave) propagates sync when every wave feeding into it has one, and falls back to the existing yield*-driven path the moment a closure-backed wave (which never sets sync) is anywhere in the composition - so a student-supplied wave function's stepping/ breakpoint visibility, and Conductor's worker-boundary async contract, are entirely unaffected. sampleWave itself takes the fast path whenever the wave it's sampling has one. Confirms the fast path is a pure performance change, not a semantic one: correctness depends only on the fallback being exact, which the existing PAIR/ARRAY module-interop and existing sound-bundle test suite (64 tests, all passing) already exercise the composition shapes for. * sound: sync fast path for student-authored wave closures, when the evaluator supports it closureToWave wrapped every student-supplied wave function as a plain async generator calling evaluator.closure_call_unchecked - correct, but paying a real microtask (and a full evaluator round-trip) per sample even for the simplest possible wave, sampled 44100x/sec. The built-in-wave fast path landed earlier in this file's siblings (functions.ts) doesn't help here: a student's own wave has to actually run inside whichever engine (CSE/PVML/py2js) is executing the program - there's no way for the module itself to skip that call. An engine can now opt a closure into a synchronous fast path by implementing closure_call_sync (py-slang's GenericDataHandler, exposed so far only by py2js, whose dual-compiled functions already have a synchronous body internally - rt.callSync - that just wasn't reachable from outside py-slang before). This module checks for the method generically - closureToWave attaches wave.sync only when the evaluator actually provides closure_call_sync, and every other engine (CSE, PVML, until they grow the same capability) is completely unaffected: the method simply doesn't exist on their evaluator instance, so every wave stays on the existing async path, unchanged. The one correctness wrinkle worth being explicit about: closure_call_sync returning undefined means "no sync form for this closure" - safe as a signal before the closure has run, but from inside wave.sync (a plain function, not a generator - there's no way to suspend and retry via the async path once we're in here) a missing sync form after the fact is treated as a hard internal error rather than silently producing a wrong sample. * sound: waveToConductorClosure was silently dropping the sync fast path closureToWave (Conductor closure -> internal Wave) already picks up wave.sync from the evaluator's closure_call_sync when available, but the reverse direction didn't: waveToConductorClosure (internal Wave -> Conductor closure) always built a plain async-only wrapper, with no way for anything downstream to know the original wave ever had a sync form. Since make_sound/ make_stereo_sound/get_wave/etc. all round-trip a Sound's waves through this function to hand it back to Python, every Sound handed back to a student - even one built entirely from a py2js closure that supports closure_call_sync - permanently lost the fast path the instant it was constructed. play() on that same Sound later would then find closure_call_sync returning undefined (no .sync on the re-wrapped closure) and hit the "no synchronous form" internal error closureToWave raises for exactly this shouldn't-happen case. Fixed by having waveToConductorClosure attach the same kind of .sync twin (reusing wave.sync, converting its number result to/from a TypedValue) before handing the function to closure_make, mirroring closureToWave's direction exactly. No behavior change for a wave with no sync form (CSE closures, or a wave that genuinely needs a host round-trip) - conductorWave.sync is simply never attached. * sound: address review feedback on record/record_for and error messages - record/record_for: stop hardcoding the function name in error messages (record: ..., record_for: ...) - use `${record.name}`/`${record_for.name}` like every other error in this file, so a rename can't silently go stale. Same fix for play's EvaluatorParameterTypeError/duration-negative error, which had the same hardcoded-literal bug. - record/record_for: replace the nested setTimeout+.then() towers with a single async IIFE using es-toolkit's delay(), preserving the exact same event ordering (pre-recording-signal pause, recording signal, pre-recording pause, recording, recording signal) but as flat sequential awaits. * sound: accept DataType.ARRAY as equally valid to PAIR for Sound values py-slang's module interface no longer has a distinct "pair" representation (source-academy/py-slang#307): pythonToModule now builds every Python list/ pair as a flat DataType.ARRAY, never a DataType.PAIR/EMPTY_LIST chain. A Sound built here via soundToConductor and round-tripped out to Python (assigned to a variable, passed to another module call like get_wave/is_sound/play) then arrives tagged ARRAY, not PAIR - conductorToSound/is_sound hardcoded `value.type !== DataType.PAIR` checks broke on this, same class of bug as binary_tree's #813. Fixed the same way: isPairLike(value) accepts either DataType.PAIR or DataType.ARRAY wherever a Sound's own tag is checked - pair_head/pair_tail already read either shape identically (py-slang's GenericDataHandler bridge), so this is purely about validation, not traversal. Also fixed conductorListToSounds (consecutively/simultaneously's list argument) and stacking_adsr's envelopes list: a genuine Python list of any length now crosses as a flat ARRAY too, not just a 2-element pair, so the old PAIR-chain-only walk would silently return zero elements for a real multi-sound list. Added readListElements to read either an ARRAY (via array_length/array_get) or a PAIR/EMPTY_LIST chain, matching py-slang's own GenericDataHandler.readListElements pattern. soundToConductor's own construction (pair_make calls) is unchanged - it always freshly builds a genuine PAIR; only validation of an incoming (possibly round-tripped) value needed to widen. * sound: don't crash when closure_call_sync exists but this closure has no .sync twin closure_call_sync lives on GenericDataHandler, the shared IDataHandler implementation across all of py-slang's engines - it's always present regardless of which engine is actually running, so closureToWave's old check ("does the method exist") was never actually engine-specific despite its own comment claiming otherwise. Only some py2js closures ever carry a real .sync twin; CSE and PVML closures never do. Every student-authored wave function played via play()/play_wave() on CSE or PVML was hitting the 'Internal error: closure_call_sync unexpectedly had no synchronous form' throw on its very first sample, since .sync was attached unconditionally whenever the method merely existed. Fixed by determining sync-capability once, with a real probe call, before ever exposing .sync on the Wave at all - closure_call_sync's own contract only returns undefined for an unsupported argument type before the closure ever runs (a wave's argument is always a plain number, always supported), so the probe is free unless the closure genuinely has a .sync twin, in which case its result is reused as the Wave's first real sample instead of being thrown away. If .sync isn't available, the Wave silently stays on the existing async path - no crash, matches every other Sound consumer's existing async fallback. * sound: correct closure-cache comment per Martin's ruling on identity preservation Martin: the module-evaluator bridge isn't obligated to be identity-preserving - two JS functions that are === may be represented by two Python functions that aren't 'is' to each other, and that's fine as a general FFI property. The earlier comment claimed the wave/closure caches make get_wave(s) == get_left_wave(s) a reliable invariant - they don't, since each engine's own moduleToPython still builds a fresh Python-side wrapper per conversion regardless of what's cached on the Conductor side. The caches stay (still real, just for avoiding redundant closure_make calls), the comment now says what they actually guarantee. * sound: describe mono behaviorally, not by reference identity, in doc comments Per Martin: avoid 'identical' when describing the two channels of a mono Sound in specs - say left_wave(t) == right_wave(t) for all t instead of claiming the two waves are the same object/reference. Reworded the public- facing docblocks (index.ts's and functions.ts's @module comments, the Sound type's own doc in types.ts, get_wave/record's JSDoc) to describe the behavioral contract a cadet program can actually observe, rather than an internal reference-equality detail. Where a comment is genuinely about this file's own implementation choice (make_sound assigning one Wave object to both fields, and later code taking advantage of that via leftWave === rightWave checks), left those as-is and called out explicitly that it's an implementation detail, not something the Sound Discipline promises. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
martin-henz
pushed a commit
that referenced
this pull request
Jul 26, 2026
* docs: add Conductor module interface conventions page Records design decisions made while migrating bundles to Conductor, worked out with Martin Henz during the py-slang#307 discussion (source-academy/py-slang#307): numbers are always floats crossing a module boundary in either direction with no exceptions (and why - keeps modules bigint-free, for simplicity/performance/future-language portability); there is no distinct "pair" representation anymore, pairs are just 2-element arrays, and array conversion should be untyped and recursive; DataType.OPAQUE is where structural recursion stops; and binary_tree's is_tree/assertNonEmptyTree hardcoded-PAIR-check exception (#813) is called out explicitly so it isn't "fixed" into false consistency later. Requested by Lee Yi - a Slack conversation isn't discoverable to future bundle authors the way a docs page is. * docs: add 'ints' to cspell word list CI's spellcheck flagged it in Martin's directly-quoted Slack message ("ints are silently converted to floats") - added to the word list rather than reword a direct quote. * docs: restructure Conductor interop page per Lee's review Lee's review on #814: "the listing of the reasoning should be put in its own section of docs: The conventions section is about how to write bundle functions, not necessarily about the why we made these design decisions ... It might fall better under the advanced section ... worth splitting each 'issue' into different pages too." - Moved out of 2-bundle/4-conventions/ into a new 5-advanced/conductor-interop/ subsection. - Split the single page into four: numbers.md, pairs-and-arrays.md, opaque.md, binary-tree-exception.md, plus an index.md linking them. - Updated pairs-and-arrays.md to reflect Martin's follow-up review comment on py-slang#307 (now merged): DataType.PAIR is slated for full removal from py-slang, not just being treated as interchangeable with DataType.ARRAY - Aarav Malani owns that follow-up PR. Called this out explicitly rather than describing the interchangeable-bridge state as final. - Added the empty-list-vs-None pitfall (caught by live testing after an earlier version of the py-slang fix got this wrong) as its own callout. - Added 'Aarav'/'Malani' to the cspell word list. * docs: remove name attribution for the DataType.PAIR removal follow-up Keep the substantive content (a follow-up PR has been claimed to fully remove DataType.PAIR from py-slang) without naming who's doing it in the docs content itself. * docs: wrap complex-numbers note in a GitHub-flavored alert box Lee's inline review comment on the original single-page version (before the restructure moved this line into opaque.md): "Use a dialog box for this - I think of all the languages supported only Python supports complex numbers as a primitive. so it's more of an aside." * docs: rename conductor-interop/index.md to conductor-interop.md This docs site's sidebar plugin (vitepress-sidebar) uses useFolderLinkFromSameNameSubFile to make a folder itself clickable in the sidebar - it needs a same-named file inside the folder (see the existing 5-advanced/flow/flow.md precedent), not index.md. Using index.md instead left the folder's sidebar entry as unclickable text (its title pulled from useFolderTitleFromIndexFile) with no page behind it, and the file itself unreachable through normal site navigation. * docs: number-prefix the conductor-interop pages to fix sidebar order vitepress-sidebar sorts alphabetically by filename by default, which put the pages in the wrong order (binary_tree exception, Numbers, OPAQUE, Pairs and arrays - alphabetical, not the intended reading order). Matches the existing 2-bundle/4-conventions/ convention (1-basic.md, 2-abstractions.md, ...) for forcing a deliberate order. Updated every internal cross-reference between the pages to match the new filenames. * docs: fix 5-advanced/index.md's link to conductor-interop Pointed at the bare folder URL (./conductor-interop/), which 404s now that the landing page is conductor-interop.md instead of index.md (see previous commit) - VitePress only auto-serves a folder's trailing-slash URL from an index.md. Point directly at the actual file instead. * docs: add closure_call_sync design-decision page Per Lee Yi's review on modules#796: document why IDataHandler.closure_call_sync exists as an optional fast path (Conductor's ExternCallable contract mandates AsyncGenerator closure calls, which is correct but costs measurable overhead at 44.1kHz sample-rate hot loops), which engines actually implement it today (py2js only, via dual compilation - PVML/CSE don't have it), and why a module has to check for it at runtime rather than assume it per-engine. Numbers pulled from py-slang's experiments/py2js benchmarks, condensed to what's forward-looking rather than a blow-by-blow history. * docs: document the .sync attachment convention for module implementers Per modules#832: write up, as a general pattern rather than a sound-specific trick, why a module should attach a .sync twin to any closure handed back to cadet code that's provably synchronous, not just the ones the module itself expects to sample in a hot loop. A closure in cadet hands can end up composed and passed into a different module's own synchronous sampling loop, which is exactly the failure mode modules#833 had to defend closureToWave against. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Companion fix for source-academy/py-slang#307 (now merged). That PR removes py-slang's distinct "pair" representation entirely:
pythonToModulenow builds every Python list as a flatDataType.ARRAY, never aDataType.PAIR/EMPTY_LISTchain (per Martin Henz: "there is nothing called a pair anymore. Pairs are just arrays of length 2").Most already-migrated bundles (
sound,midi,repeat,scrabble) need no changes at all for this — they only ever go through the genericIDataHandlerhelpers (pair_head/pair_tail/list_to_vec/etc.), which py-slang'sGenericDataHandleralready bridges onto either representation.binary_treeis the one confirmed exception:is_tree/assertNonEmptyTreeread a tree node's ownvalue.typetag directly (!== DataType.PAIR), bypassing any generic helper. A tree built here (viamake_tree's ownpair_makecalls) and round-tripped out to Python and back in — e.g.t = make_tree(1, ...), then laterentry(t)— now arrives re-encoded asDataType.ARRAYrather thanDataType.PAIR, so these hardcoded checks broke. Confirmed live against the real deployed evaluators too: this reproduces on the very first call after construction (entry(t)right aftert = make_tree(...)), not just on nested trees as I originally assumed.Changes
functions.ts: addedisPairLike()(acceptsDataType.PAIRorDataType.ARRAY), used inis_treeandassertNonEmptyTreein place of the hardcodedDataType.PAIRchecks.make_tree's own construction (pair_makecalls) is unchanged — it always freshly builds a genuinePAIRfor a node it creates itself; only validation of an incoming (possibly round-tripped) value needed to widen.__tests__/index.test.ts: added a regression test constructing a tree entirely out ofDataType.ARRAYvalues (simulating the round-trip shape) and confirmingis_tree/entryboth still work.Verification
I was not able to get a reliable— resolved, see comment for why that happened.yarn testrun in my environmentyarn tsc(bundle-scoped): clean.yarn test(bundle-scoped, realyarn install, no borrowed/cross-branchnode_modules): all 18 tests pass, including the newDataType.ARRAYround-trip regression test.Merge order
This depends on nothing landing first, but only actually matters once source-academy/py-slang#307 is deployed — until then, py-slang still round-trips trees as
PAIR, so this change is backward compatible either way (it only adds acceptance ofARRAY, doesn't remove acceptance ofPAIR).