Skip to content

Operators Fix#1995

Open
leeyi45 wants to merge 35 commits into
masterfrom
operators-fix
Open

Operators Fix#1995
leeyi45 wants to merge 35 commits into
masterfrom
operators-fix

Conversation

@leeyi45

@leeyi45 leeyi45 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Fix Module Functions not being called properly

With the way that functions need to be called in js-slang, modules need access to the callIfFuncAndRightArgs operator, which is not properly being exported via the requireProvider right now.

Fix Argument Count for Functions

Currently, js-slang allows us to set minArgsNeeded when the function takes in a variable number of parameters. But this is actually unnecessary since this is always inferrable from f.length. Instead, what is unknown is the maximum number of arguments:

// Has length 1, but has maximum number of args 2
function foo(x1: number, x2?: number) {}

wrap has been updated to take the number of optional parameters instead of the minimum number of arguments. If true is passed in instead, then the function is assumed to have a rest parameter and thus no argument maximum.

Functions should declare all required parameters in their signature.

With this, errors will now be thrown if a function is called with more arguments than is allowed.

I lied It turns out certain functions like Math.max and Math.min have length of 2 but take any number of parameters because reasons. Easy workarounds like just wrapping both those functions in an arrow function.

Variable numbers of arguments for modules

If a bundle's function is defined using wrap, then it will now be able to take in default parameters and rest parameters, fixing #1238.

leeyi45 added 2 commits June 12, 2026 22:12
(cherry picked from commit db806e4d2f35eb8560aeb1f33042d8ec8985ca3e)

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

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.

Code Review

This pull request introduces several utility updates and refactorings, including adding a callWithoutMetadata wrapper in operators.ts to simplify function calls, updating valueToStringDag to use this wrapper, and refactoring getChapterName in misc.ts to use direct enum indexing. Additionally, it updates mock import declarations to include an empty attributes array and adds tests for getVariantName. The review feedback suggests simplifying the type definition for ChapterStrings by using keyof typeof Chapter directly instead of introducing a redundant custom EnumKeys helper.

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.

Comment thread src/utils/misc.ts Outdated
@coveralls

coveralls commented Jun 12, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 28381718839

Coverage increased (+0.2%) to 78.717%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: 7 uncovered changes across 5 files (117 of 124 lines covered, 94.35%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
src/stdlib/stream.ts 7 5 71.43%
src/utils/ast/helpers.ts 11 9 81.82%
src/index.ts 2 1 50.0%
src/utils/operators.ts 23 22 95.65%
src/utils/testing/index.ts 6 5 83.33%
Total (19 files) 124 117 94.35%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 8799
Covered Lines: 7090
Line Coverage: 80.58%
Relevant Branches: 4249
Covered Branches: 3181
Branch Coverage: 74.86%
Branches in Coverage %: Yes
Coverage Strength: 204409.23 hits per line

💛 - Coveralls

@Akshay-2007-1 Akshay-2007-1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey, CI is failing on the format check, just a missing semicolon in rttc.test.ts:

  })   // ← line 42, needs a semicolon

Just formatting issues.

I feel that the Gemini comment is valid although it isnt blocking and is just a trivial code improvement.

@leeyi45

leeyi45 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Hey, CI is failing on the format check, just a missing semicolon in rttc.test.ts:

  })   // ← line 42, needs a semicolon

Just formatting issues.

I feel that the Gemini comment is valid although it isnt blocking and is just a trivial code improvement.

I think I wrote it the way I did because of some weird Typescript shennenigans not doing the type inference correctly, but as far as I can tell Typescript is behaving correctly, so not sure where that came from.

@Akshay-2007-1

Copy link
Copy Markdown

Nice cleanup overall, the split into TooFewArgumentsError/TooManyArgumentsError is much cleaner than the old single class. A few things I noticed while reading through:


Stepper doesn't handle default params in arrow function arity check
src/stepper/nodes/Expression/FunctionApplication.ts, around line 67:

if (arrowFunction.params.length > this.arguments.length) {
  throw new TooFewArgumentsError(this, this.arguments.length, arrowFunction.params.length, false);
}

This counts all params including AssignmentPattern nodes (default params). The CSE machine path was updated to compute minArgs by filtering those out, but the stepper wasn't. So something like:

const f = (x, y = 0) => x + y;
f(1);

will throw TooFewArgumentsError in the stepper even though it's a valid call. Also, isVarArgs is hardcoded to false here which would misreport the error for rest-param arrow functions too.


display is registered with optArgs=2 but only has 1 optional parameter
src/createContext.ts, line 299:

defineBuiltin(context, 'display(val, prepend = undefined)', display, 2);

display is defined as (v: Value, ...s: string[]), so display.length === 1. With optArgCount=2, maxArgsAllowed becomes 1 + 2 = 3, which means display(val, prepend, extra) passes the arg check silently even though only two parameters are ever used. Based on the signature string display(val, prepend = undefined) the correct value should be 1 (one optional param beyond the required val). Same logic as display_list which correctly uses 1.


Null guard on error.explain in src/index.ts (around line 456) doesn't prevent the crash

if (!error.explain) {
  console.error('Unhandled error', error);
}
const explanation = error.explain();

If error.explain is falsy the console.error fires but execution falls straight through to error.explain() which will throw a TypeError. Either add a return / early exit inside the guard, or use error.explain?.() with a fallback.


Debug log left as a comment in src/utils/operators.ts (line 168)

// console.log(f.name, f.length, maxArgsAllowed, receivedLength);

This sits inside callIfFuncAndRightArgs which is a hot path. Worth removing before merge rather than leaving it for someone to accidentally uncomment later.


Stepper skips arity check for list but CSE machine no longer special-cases it
src/stepper/builtins/index.ts, around line 1264, the if (name !== 'list') guard is still there with a comment calling it a "brute force" fix. The CSE machine path dropped its equivalent special case in this PR. If list changes in future the two evaluators will diverge silently on arity errors.


wrapUnsafe exports an untyped entry point permanently
The two call sites that need it (loadModuleBundleAsync and defineBuiltin) type their functions as unknown/Value, which is why typed wrap doesn't work there. But exporting wrapUnsafe means any future contributor can reach for it instead of typing properly. A localised // @ts-expect-error at each of those two call sites would confine the unsafety to where it actually lives.

@leeyi45

leeyi45 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Akshay-2007-1.

  1. I've fixed the disparities between the stepper and cse-machines, and actually with the regular runner as well. There should be parity across the three with how function arity is handled, and the tests seem to account for and indicate that is the case. The specific case you highlighted, although now handled, is technically impossible since the stepper is only used with chapters of Source that wouldn't support a default value like that.

  2. Fixed the display bug.

  3. I can't decide what to do about this one. For the longest time random errors that don't get properly handled by js-slang get bubbled up into parseErrors and then result in error.explain being undefined. This is usually a sign something has gone terribly wrong, so I'm just logging the error to the console. I don't know what we should do about errors like these tbh. So far I've just been letting the error get thrown.

  4. Removed the console.log comment

  5. I'm undecided about wrapUnsafe. I foresee other use cases like in loader.ts or createContext.ts where its useful to have a signature that's not going to complain about the strict typing, but you're also right to say that that might make people lazy oops.

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.

3 participants