Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support rest params in function calls #2905

Merged
merged 8 commits into from
Feb 7, 2025

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Feb 4, 2025

Fixes #377
Fixes #2291

Changes proposed in this pull request:

  • Implement support for rest parameters when calling functions or constructors.

Note, does not yet support rest arguments, as the spread operator and iterators would need to be implemented first.

ie. this now works:

function foo<T>(...args: T[]): void {
}

foo(1, 2, 3, 4, 5);

but this does not (yet):

const nums = [1, 2, 3, 4, 5];
foo(...nums);

Behavior

Fundamentally,

function foo<T>(...args: T[]): void { }
foo(1, 2, 3);

is equivalent to:

function foo<T>(args: T[] = []): void { }
foo([1, 2, 3]);

The only difference is that the caller does not construct the array - but the array still exists in the function scope.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@MaxGraey
Copy link
Member

MaxGraey commented Feb 4, 2025

There is also another very specific case when arguments are processed not in a loop:

function fixedArgs(...args: i32[]): i32 {
  return args[0] + args[1];
}

fixedArgs(1, 2); // ok

But this is become undefined behavior for such cases:

fixedArgs(1);
fixedArgs(1, 2, 3);

TypeScript does not generate an error in this case, but we must anticipate this option and reject all options except those with two arguments to the call site and preferably during compile time

@CountBleck
Copy link
Member

@MaxGraey that kind of checking would probably be a TODO, since I don't believe the compiler in its current state has a way to note metadata like that on functions. Maybe that would be set aside for a separate pass, in combination with a custom IR (or something that could detect things like const aaarghs = args)?

In any case, the heuristic wouldn't work as soon as someFunction(args) is in the function (someFunction could mutate the array).

@mattjohnsonpint
Copy link
Contributor Author

... TypeScript does not generate an error in this case

It doesn't generate an error for fixedArgs(1), because args[1] is undefined, and 1 + undefined === NaN, so it returns NaN. But your other example fixedArgs(1, 2, 3) should be valid in both TS and AS. There's no requirement that all args passed are used.

... but we must anticipate this option and reject all options except those with two arguments to the call site and preferably during compile time

Why? I don't think we do that type of analysis for anything similar such as passing arrays.

IMHO function fixedArgs(...args: i32[]): i32 and function fixedArgs(args: i32[]=[]): i32 should be identical in how the args are treated within the function. It's only the caller that changes syntax.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 4, 2025

Why I'm concerned about the implementation via a dynamic array. Many math methods such as Math.min, Math.max, Math.hypot as well as Array#push, Array#concat and many others require this feature. If every time there will be created an implicit array even for one argument, it will significantly affect the performance and also significantly increase memory pressure.

@mattjohnsonpint
Copy link
Contributor Author

... it will significantly affect the performance and also significantly increase memory pressure.

Those are valid concerns indeed. I'm open to suggestions.

Would it help if the array were @unmanaged ?

@mattjohnsonpint
Copy link
Contributor Author

Would it help if the array were @unmanaged ?

though it couldn't be implemented as easily as this:

@unmanaged
class UnmanagedArray<T> extends Array<T> { }

... because: ERROR AS207: Unmanaged classes cannot extend managed classes and vice-versa.

Another thought. Does AssemblyScript have any concept (internal or otherwise) similar to stackalloc in C# (or Go's implicit stack allocations, etc.)?

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Feb 4, 2025

Also, I'm thinking about your concern with Math.max, etc. In typescript, both rest params and rest args are supported, so you can do something like this:

function maxNum(str: string): number {
  const arr = str.split(' ').map(s => parseInt(s))
  return Math.max(...arr)
}

console.log(maxNum("10 3 40 1 2")) // 40
console.log(maxNum("100 200 300")) // 300
console.log(maxNum(someArbitraryStringFromUserInput))

While this PR doesn't implement rest args, I presumed that we'd want to do that eventually.

Since in this example, the array size cannot be known at compile time, I'm really not sure how one could implement Math.max that would allow this, without using a dynamically sized array.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 4, 2025

Well, you will definitely need to separate the static and dynamic case for rest parameters. Perhaps it would be a good option to leave everything as it is and in the future add a special optimizing pass or conditional optimization for the case when all the arguments are known at compile time and not to use an array for this case but to use shadow stack or static data segments

@mattjohnsonpint
Copy link
Contributor Author

Well, you will definitely need to separate the static and dynamic case for rest parameters. Perhaps it would be a good option to leave everything as it is and in the future add a special optimizing pass or conditional optimization for the case when all the arguments are known at compile time and not to use an array for this case but to use shadow stack or static data segments

That sounds reasonable. I could also see an approach similar to generics, where the compiler makes internal representations of the function signature with fixed sizes to be used by those callers. ie, Math.max$2(p0, p1), Math.max$3(p0, p1, p2), etc.

So then, for now - you good with this PR as is?

@MaxGraey

This comment was marked as outdated.

@MaxGraey MaxGraey self-requested a review February 6, 2025 00:04
Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

LGTM

@CountBleck
Copy link
Member

Okay, I just tested it myself as a sanity check, and everything looks good...I'll merge it now!

@CountBleck CountBleck merged commit 6e151f8 into AssemblyScript:main Feb 7, 2025
17 checks passed
@mattjohnsonpint mattjohnsonpint deleted the rest-params branch February 7, 2025 04:08
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.

No support for rest paramaters Support rest parameters
4 participants