Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 679
feat: Support rest params in function calls#2905
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mattjohnsonpint commented Feb 4, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
MaxGraey commented Feb 4, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There is also another very specific case when arguments are processed not in a loop: functionfixedArgs(...args: i32[]): i32{returnargs[0]+args[1];}fixedArgs(1,2);// okBut 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 commented Feb 4, 2025
@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 In any case, the heuristic wouldn't work as soon as |
Uh oh!
There was an error while loading. Please reload this page.
mattjohnsonpint commented Feb 4, 2025
It doesn't generate an error for
Why? I don't think we do that type of analysis for anything similar such as passing arrays. IMHO |
MaxGraey commented Feb 4, 2025
Why I'm concerned about the implementation via a dynamic array. Many math methods such as |
mattjohnsonpint commented Feb 4, 2025
Those are valid concerns indeed. I'm open to suggestions. Would it help if the array were |
mattjohnsonpint commented Feb 4, 2025
though it couldn't be implemented as easily as this: @unmanagedclassUnmanagedArray<T>extendsArray<T>{}... because: Another thought. Does AssemblyScript have any concept (internal or otherwise) similar to |
mattjohnsonpint commented Feb 4, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Also, I'm thinking about your concern with functionmaxNum(str: string): number{constarr=str.split(' ').map(s=>parseInt(s))returnMath.max(...arr)}console.log(maxNum("10 3 40 1 2"))// 40console.log(maxNum("100 200 300"))// 300console.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 |
MaxGraey commented Feb 4, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 commented Feb 4, 2025
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, So then, for now - you good with this PR as is? |
This comment was marked as outdated.
This comment was marked as outdated.
HerrCai0907 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CountBleck commented Feb 7, 2025
Okay, I just tested it myself as a sanity check, and everything looks good...I'll merge it now! |
6e151f8 into AssemblyScript:mainUh oh!
There was an error while loading. Please reload this page.
Fixes#377
Fixes#2291
Changes proposed in this pull request:
Note, does not yet support rest arguments, as the spread operator and iterators would need to be implemented first.
ie. this now works:
but this does not (yet):
Behavior
Fundamentally,
is equivalent to:
The only difference is that the caller does not construct the array - but the array still exists in the function scope.