Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Implement support for Closures. #26

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

sjrd
Copy link
Collaborator

@sjrd sjrd commented Mar 18, 2024

Based on #21.

@sjrd sjrd requested a review from tanishiking March 18, 2024 20:31
@sjrd sjrd marked this pull request as ready for review March 19, 2024 07:07
Copy link
Owner

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

Overall, it looks great!
I left some comments especially around the restParameter

val typ = TypeTransformer.transformType(param.ptpe)
WasmLocal(WasmLocalName.fromIR(param.name.name), typ, isParameter = true)
}
val resultTyps = TypeTransformer.transformResultType(IRTypes.AnyType)
Copy link
Owner

Choose a reason for hiding this comment

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

Can't we use tree.body.tpe ?

Suggested change
val resultTyps = TypeTransformer.transformResultType(IRTypes.AnyType)
val resultTyps = TypeTransformer.transformResultType(tree.body.tpe)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the declared result type is implicitly always any. If the body happens to be a primitive int, we need to box it to match the declared result type. I expanded a little bit on the comment on generateIRBody.

Comment on lines +67 to +68
closureRest: (f, data, n) => ((...args) => f(data, ...args.slice(0, n), args.slice(n))),
closureThisRest: (f, data, n) => function(...args) { return f(this, data, ...args.slice(0, n), args.slice(n)); },
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that the imported function for closureRest and closureThisRest receive two args (f and data) but doesn't have third argument (n). Where this arg comes from?

(import "__scalaJSHelpers" "closureRest" (func $__scalaJSHelpers#closureRest___fun (param (ref func)) (param anyref) (result (ref any))))
(import "__scalaJSHelpers" "closureThisRest" (func $__scalaJSHelpers#closureThisRest___fun (param (ref func)) (param anyref) (result (ref any))))

Also, how can we test these? it seems the test cases doesn't have a restParameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, good catch! I fixed it in the expression builder.

Also, how can we test these? it seems the test cases doesn't have a restParameter.

As the comment in the tests mentions, currently we cannot test the ...rest parameters. Although the IR does not require it, the source syntax that creates Closures with rest params also requires Scala's full-blown Seqs (for vararags). We'll be able to add tests when we have enough things to support Seq.

Copy link
Owner

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

Thanks! The comments make sense 💯

Comment on lines +1717 to +1721
/* If there is a ...rest param, the helper requires as third argument the
* number of regular arguments.
*/
if (hasRestParam)
instrs += I32_CONST(I32(tree.params.size))
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@tanishiking tanishiking merged commit 84a6dc8 into tanishiking:main Mar 20, 2024
1 check passed
@sjrd sjrd deleted the closure branch March 20, 2024 07:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants