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

Implement Function Objects #141

Closed
jasonwilliams opened this issue Oct 8, 2019 · 7 comments · Fixed by #255
Closed

Implement Function Objects #141

jasonwilliams opened this issue Oct 8, 2019 · 7 comments · Fixed by #255
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@jasonwilliams
Copy link
Member

jasonwilliams commented Oct 8, 2019

The goal of this issue is to have a function object which more closely resembles https://tc39.es/ecma262/#sec-ecmascript-function-objects.

I created function_object (which will hopefully replace function).

In the specification function objects are actually just ordinary objects with some additional methods and internal slots. I originally attempted to do the same but ran into issues.

I needed to deviate from the spec a little bit here…

Deviation

Implementing https://tc39.es/ecma262/#sec-ordinaryfunctioncreate is tricky, we can't just take an object and bolt on properties + methods on to it, Rust doesn't work in that way, its not a dynamic language and also doesn't have inheritance.
In the spec Functions are basically objects except they have quite a few additional internal slots, and some additional internal methods (Call, Construct).

Now internal slots can be handled in Object Create (where we basically pass through the slots we want on that object) that isn't a big issue.
The problem is Internal Methods.

We can't just add pub fn call and pub fn construct to every object, as 90% of the objects won't need those methods.

We can't have an internal slot that’s a [[Call]] function either because the built-in and dynamic functions have different types. Another reason for not going down this route is because we would need to be checking if the current object is a function all over the place, (loads of if ObjectKind::Function).

At this point its better to just have function be its own thing, I think call can be a method which will either iterate through an expression (dynamic function) or call a Rust built-in function (builtin function), I think we can abstract this inside of call, the end user won't care so long as it returns a <Value>.

With this in mind we can create a new function function::fromDynamicFunction([expr]) or function::fromBuiltIn([rust_func])

So thats my rambling, would be interested to here from @Razican @jorendorff @IovoslavIovchev as to what you think is best here?

I made a start here:
jasonwilliams#255

@jasonwilliams jasonwilliams self-assigned this Oct 8, 2019
@jasonwilliams jasonwilliams added the help wanted Extra attention is needed label Oct 11, 2019
@IovoslavIovchev
Copy link
Contributor

@jasonwilliams, while I do agree that we could have functions be their own thing and that 90% of the objects won't need [[Call]] and [[Construct]], I do not see why we can't have an internal slot that’s a [[Call]] (and possibly also [[Construct]]).

Correct me if any of the following is wrong or if I have misunderstood..
Built-in and dynamic functions might have different types (and by that I am assuming you mean parameters and return type), but [[Call]] has the same signature for either -- (thisArgument, argumentList).

Another thing that I am missing is why "we would need to be checking if the current object is a function all over the place". Wouldn't once (in the beginning) be enough? Could you give a more detailed explanation?

All of that said, I do fancy the idea of the dynamic/native function abstraction.

@jasonwilliams
Copy link
Member Author

jasonwilliams commented Mar 10, 2020

I do not see why we can't have an internal slot that’s a [[Call]] (and possibly also [[Construct]]).

We could take an OrdinaryObject and attach [[Call]] to it, but the signature of internal slots is {string: <Value>} and this will be different depending on if call is a dynamic function or a built-in function.
I think this is why in the spec they call these internal methods instead of slots because they behave very differently to static data.
this means they need to be actually attached to object as an impl pub fn, rather than some data added in the internal_slots hashMap.

Correct me if any of the following is wrong or if I have misunderstood..
Built-in and dynamic functions might have different types (and by that I am assuming you mean parameters and return type), but [[Call]] has the same signature for either -- (thisArgument, argumentList).

Yes this is correct, we need to find some way to abstract this so we only have 1 module dealing with functions regardless of native/dynamic.

Another thing that I am missing is why "we would need to be checking if the current object is a function all over the place". Wouldn't once (in the beginning) be enough? Could you give a more detailed explanation?

Ok so this is the annoying bit, the spec assumes some sort of inheritance in the way internal methods are laid out.
For example, ordinary object defines getOwnProperty then you have the String exotic object which overwrites getOwnProperty to its own implementation
Array also does the same thing https://tc39.es/ecma262/#sec-array-exotic-objects-defineownproperty-p-desc it overwrites defineOwnProperty to its own implementation.

In rust we can't do inheritance, so instead i moved common methods to a trait https://github.com/jasonwilliams/boa/blob/master/boa/src/builtins/object/internal_methods_trait.rs

On second look functions don't do any overriding instead they only add Call and Construct. But this is still something we need to find a working design for.

@Razican
Copy link
Member

Razican commented Mar 12, 2020

@jasonwilliams I'm not very aware of the implementation details of functions in Boa, but if these are internal methods, can't we have a different hash map for those, separate from the slots?

This would allow to have some kind of inheritance (when you have to call a method, you select the one in the internal method, which can be re-written in the hash map).

@Razican Razican added the enhancement New feature or request label Apr 3, 2020
@jasonwilliams
Copy link
Member Author

jasonwilliams commented Apr 19, 2020

@Razican I don't think they need to be, they're very specific to functions and used nowhere else. If they go into slots that means we need to convert them into JSValues (which im not sure is possible in this sense as youll have a chicken and egg problem.)

So far function object is here:
https://github.com/jasonwilliams/boa/pull/255/files#diff-6f0f25f60a0da52946c66d29f4b79fd6

I'm basically trying to implement this
https://tc39.es/ecma262/#sec-ordinaryfunctioncreate

step 4 is interesting, as there are 2 type of functions, internal ones and dynamic ones, so call will be tricky, it may need to be an Enum and we go down 2 branches depending on what the passed in value is? The body could either be rust code or a StatementList basically...

You know what, looking at the spec it seems built-in function objects are just a completely different class altogether, i don't mind doing that, but there will be a proportional amount of duplication.

Step 5 is easy now we have FormalParameters

Step 6 will need to store the ECMAScript code (this would be our StatementList Node)

Step 13 would be a new Function Environment Record we've created and passed in.

Step 15 would be a reference to the realm object

@jasonwilliams
Copy link
Member Author

jasonwilliams commented Apr 19, 2020

If we decide to follow the spec we would have 2 structs, built-in function object and function object.
We would then need to add them here: https://github.com/jasonwilliams/boa/blob/master/boa/src/builtins/value/mod.rs#L37-L54

Or we do what we do now where Function is in ValueData and function is an enum of the 2 possible function types.

would love to know what you think @Razican @HalidOdat

@HalidOdat
Copy link
Member

Or we do what we do now where Function is in ValueData and function is an enum of the 2 possible function types.

would love to know what you think @Razican @HalidOdat

I think it's best if we don't split them, and I don't think v8 does either.

This is only an abstraction, not splitting them would allow us keep the function code in one place, making the code much cleaner and intuitive.

Would like to know what @Razican thinks of this.

@Razican
Copy link
Member

Razican commented Apr 19, 2020

I think it's best not to duplicate code. This will also help with performance a bit, as we use the same code more times (cache performance should improve).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants