Skip to content

First-class functions #1384

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

Merged
merged 7 commits into from
Jul 16, 2020
Merged

First-class functions #1384

merged 7 commits into from
Jul 16, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jul 10, 2020

Introduces a wrapper class Function<T> to represent references of a function type, including preparation for closure environments and an initial implementation of Function#call supporting calls with substituted this arguments.

Turned out to be a much larger change than initially expected due to tidying up types, also anticipating that we'll have both internal and external references eventually. Similarly, what previously was FunctionTarget now is represented by a runtime class, and there is an additional opportunity now to also get rid of Signature by deriving from the class.

In general this is getting somewhat too messy for my taste in that the code wasn't initially designed with first-class-everything in mind, so refactoring this even more by also introducing Object might help and making the compiler-side integration more natural might be good.

  • I've read the contributing guidelines


// @ts-ignore: this on getter
get name(this: T): string {
return "";
Copy link
Member

Choose a reason for hiding this comment

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

is it generated in compile time or just not yet implemented?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps will be gread extend nameof<T>() to nameof<T>(value: T) and do something like this return nameof(this)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a difference to JS here in that a Function class represents unique signatures. There can be multiple functions with the same signature, and signatures don't have names. So far, the implementation here does what JS does for anonymous functions, essentially dropping any names that would have to be baked into the binary otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be a way to make .name a builtin if that's important, but I assume that it turns out that we need to ship a full symbol map (names of all the functions potentially called indirectly) with each binary to achieve this. Imagine for example that there is a variable one can assign multiple functions to, then we'll have to look up the name at runtime somehow, for example by mapping from indexes to names. Not sure if the extra bytes to ship are worth it.

Copy link
Member

@MaxGraey MaxGraey Jul 10, 2020

Choose a reason for hiding this comment

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

Yeah. it definitely make sense do only if we could deduce this name in compile time and only if used request this like:

const fn = (a: i32, b: i32, c: i32) => {}
const name  = fn.name;  // could statically reflect as "fn"

function foo(fn: Function<(a: i32, b: i32, c: i32) => void>) {
  const name = fn.name // name is unknown. So output `"anonymous"` or `""`
}

const funcs = [(a: i32, b: i32, c: i32): void => {}, function fn(a: i32): void {}]
const name0 = funcs[0].name // name0 is ""
const name1 = funcs[1].name // name1 is "fn"
funcs[1] = function boo(): void {}
const name1 = funcs[1].name // name1 is "boo"

But yeah, it required dependency analysis, so probably make sense deduce only simple cases

Comment on lines +94 to +96
i32.const 1088
i32.load
call_indirect (type $i32_i32_i32_=>_i32)
Copy link
Member

Choose a reason for hiding this comment

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

definitely we need extra pass for binaryen)

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 12, 2020

There's still quite a bit more to do here in order to get (mutable) captures to work. Good next steps would be to complete #1260 to simplify compilation in general, and WebAssembly/binaryen#2586 so we can have a closureify pass on our end that replaces accesses to locals with loads from a function environment to avoid recompiling (and tracking state while doing so). Looks like this'll take a while and overall this PR might become something more like a dev branch, unless you guys want this in master already for your own experiments?

@MaxGraey
Copy link
Member

I guess it make split to several stage (logically finished parts) like this one. WebAssembly/binaryen#2586 could be long story and I'm worry we could loose focus so better merge all useful parts asap

}

/** Tests if this type represents an integer value. */
get isIntegerValue(): bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome, thanks for cleaning this up 😄 👍

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

This PR looks great! Super excited for this! Nothing huge / not requesting anything, but does this mean we will also be able to access a function "this" now? :o That would be awesome if so!

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 13, 2020

There are a couple tests for Function#call, showing what's possible with this arguments. For instance, one can reference an instance method now, pass it around and Function#call it outside of the class by providing this, and the same also works for explicitly annotated this arguments.

@dcodeIO dcodeIO merged commit 7de4745 into master Jul 16, 2020
@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 16, 2020

Merged as per consensus at the WG meeting. It is likely that there are remaining edge cases that need to be covered, so give it a try with your codebase and let me know if there are issues.

@github-actions
Copy link

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@torch2424 torch2424 deleted the first-class-functions branch July 20, 2020 20:47
@torch2424 torch2424 restored the first-class-functions branch July 20, 2020 20:47
@dcodeIO dcodeIO deleted the first-class-functions branch July 21, 2020 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants