Skip to content

Modules: Switch Module Closures to be AsyncGenerators#29

Open
AaravMalani wants to merge 2 commits into
mainfrom
feat/switch-to-generator
Open

Modules: Switch Module Closures to be AsyncGenerators#29
AaravMalani wants to merge 2 commits into
mainfrom
feat/switch-to-generator

Conversation

@AaravMalani
Copy link
Copy Markdown
Member

This PR migrates foreign functions in the module types to represent AsyncGenerators instead of raw functions.

Rationale

This has been implemented to solve a re-entrancy issue for module function execution in CSE machines. If a module function needs to call a CSE closure, the CSE machine is stalled by the module function and cannot evaluate the closure

With the new update, the AsyncGenerator allows the module function to yield when it needs to evaluate a CSE closure.
For example

function f(x: TypedValue<DataType.CLOSURE>): AsyncGenerator<void, TypedValue<DataType.VOID>, undefined> {
    // NOTE: yield* is used since it delegates yields to the data handler on the runner side to decide when 
    // the closure needs to yield (aka, is a CSE closure)
    yield* closure_call_unchecked(x, []);
    return mVoid();
}

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors closure execution and list accumulation to return AsyncGenerators instead of Promises, allowing deferred execution in CSE machines. The review feedback correctly identifies several outdated JSDoc @returns annotations in accumulate.ts and IDataHandler.ts that still reference Promise, and suggests a clearer phrasing for the new documentation in README.md.

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.

* @returns A Promise resolving to the result of accumulating the Closure over the List.
*/
export async function accumulate<T extends Exclude<DataType, DataType.VOID>>(this: IDataHandler, op: TypedValue<DataType.CLOSURE, T>, initial: TypedValue<T>, sequence: TypedValue<DataType.LIST>, resultType: T): Promise<TypedValue<T>> {
export async function* accumulate<T extends Exclude<DataType, DataType.VOID>>(this: IDataHandler, op: TypedValue<DataType.CLOSURE, T>, initial: TypedValue<T>, sequence: TypedValue<DataType.LIST>, resultType: T): AsyncGenerator<void, TypedValue<T>, undefined> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The JSDoc @returns tag is outdated. It still states that the function returns a Promise, but the signature has been updated to return an AsyncGenerator. Please update the JSDoc to reflect this change.

* @returns A promise to the returned typed value.
*/
closure_call<T extends DataType>(c: TypedValue<DataType.CLOSURE, T>, args: TypedValue<DataType>[], returnType: T): Promise<TypedValue<NoInfer<T>>>;
closure_call<T extends DataType>(c: TypedValue<DataType.CLOSURE, T>, args: TypedValue<DataType>[], returnType: T): AsyncGenerator<void, TypedValue<NoInfer<T>>, undefined>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The JSDoc @returns tag for closure_call is outdated. It still documents the return type as a Promise, but it now returns an AsyncGenerator. Please update the JSDoc to reflect this change.

* @returns A promise to the returned typed value.
*/
closure_call_unchecked<T extends DataType>(c: TypedValue<DataType.CLOSURE, T>, args: TypedValue<DataType>[]): Promise<TypedValue<NoInfer<T>>>;
closure_call_unchecked<T extends DataType>(c: TypedValue<DataType.CLOSURE, T>, args: TypedValue<DataType>[]): AsyncGenerator<void, TypedValue<NoInfer<T>>, undefined>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The JSDoc @returns tag for closure_call_unchecked is outdated. It still documents the return type as a Promise, but it now returns an AsyncGenerator. Please update the JSDoc to reflect this change.

Comment thread README.md
Each of the data types passed as identifier have functions to create an instance of that data type,
as well as read and write data to it (or call it, in the case of closures). See `conductor/types/IDataHandler`.

In the case of closures, the `closure_call` and `closure_call_unchecked` return `AsyncGenerator`s. In the case of CSE machines, yielding a closure call to a CSE closure would allow the module function to defer the execution until the closure's return value is executed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The phrase "until the closure's return value is executed" is slightly confusing, as a return value itself is not executed. Consider rephrasing it to "until the closure's execution is completed and its return value is returned" or "until the closure is executed and its return value is obtained".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant