builtinfun.length undefined

[thiagoarrais]

I'm getting undefined when I do 'abc'.matchAll.length. This is incompatible with Chrome's implementation (for example), where I get 1.

Testing with master with the following tests.js:

console.log('abc'.matchAll.length);

Here's the output for cargo run:

$ cargo run
   Compiling Boa v0.5.0 (/data/boa)
    Finished dev [unoptimized + debuginfo] target(s) in 3.20s
     Running `target/debug/boa`
undefined
[src/bin/bin.rs:57] exec(&buffer) = "undefined"

Related to #193 and #206

[Stupremee]

I think I found the error.
The following code:

'abc'.matchAll

returns undefined and not the function object.
Firefox console returns the function object instead.
I will investigate further and try to find the cause

[Stupremee]

If you create a string with " or ', the builtins::string::create_constructor (aka String Constructor) is not used, and so the methods are not set.
You can easily prove this, because this:

(new String("abc")).matchAll.length

returns the correct length.
When a method is called from this string (this = the constant string "abc"), this string is first converted to an object by Interpreter::to_object, this to_object method then uses the string prototype which has all methods set etc, so the matchAll method is also a property in the object. However, there is another problem. If the object you want to get a field from is a function, the to_obj method returns an error. I'm not sure how best to fix the error. At the moment I can only offer the following Quick-Fix:
Change this
to this:

ExprDef::GetConstField(ref obj, ref field) => {
    let mut obj = self.run(obj)?;
    if obj.get_type() != "function"
        && (obj.get_type() != "object" || obj.get_type() != "symbol")
    {
        obj = self.to_object(&obj).expect("failed to convert to object");
    }
    Ok(obj.borrow().get_field_slice(field))
}

If one of the maintainers provide me with a solution that should be used I can implement it.

[Stupremee]

Is there now a good solution which should be used?

[jasonwilliams]

Hey @Stupremee do you think this logic should be inside value.rs on get_field ?

[Stupremee]

I think so yes.
I guess a to_object method in Value would be good.

[Stupremee]

I can take care of this issue. I will add the logic in get_value. If I find a better solution I will ask it here

[Stupremee]

I thought about this issue again, and I think it's the best method to create constant values (string, numbers) via the specific constructor.

[Razican]

I think this particular case could be solved by function objects, in #255.

[jasonwilliams]

I agree

[Razican]

What is the state of this, @jasonwilliams ?

[jasonwilliams]

In theory this is done, but i think there needs to be some checking and testing.
il give this issue a v0.9 milestone.

It's already done for dynamic functions:
https://github.com/jasonwilliams/boa/blob/master/boa/src/exec/mod.rs#L296 I don't think there are any tests for this though.

for builtin functions, i think its just a case of going through and making sure they're all correct (i'm not sure if every single built-in needs a test for the correct length, that might be overkill, but maybe 1 or 2 tests for the builtins as well would be nice)

In terms of matchAll in the original issue, there is now a length passed in https://github.com/jasonwilliams/boa/blob/master/boa/src/builtins/string/mod.rs#L1029

So, outstanding on this is:

1. check the original problem in the OP is solved
2. write a test that creates a function then checks it's length property is correct
3. write a test that checks a built-in function's length is correct

[croraf]

The original problem is solved but

'asd'['matchAll'].length

isn't.

There is a missmatch here: https://github.com/boa-dev/boa/blob/master/boa/src/exec/field/mod.rs#L10-L14

[jasonwilliams]

@croraf #469 Doesn’t really close this issue, it just fixes and tests bracket accessing.
There’s no tests in there for length on built ins

[croraf]

@jasonwilliams I intentionally didn't include this test, as it follows from the two tests that I did include.

'string'.builtinFunction.length is just a combination of

• 'string'.builtinFunction resolution and
• function.property resolution

Both of these were failing, and now both are fixed which I checked with tests, so their combination also works.

[Razican]

Both of these were failing, and now both are fixed which I checked with tests, so their combination also works.

Even if x.y and y.z work, we don't have any reason to believe that x.y.z will work, if in the future the implementation changes.

Nevertheless, note that the test that we want here is to check that the length of built-in functions is correct. So we should add tests for the length of all built-in functions to make sure that the length that they return is the correct one.

[croraf]

Even if x.y and y.z work, we don't have any reason to believe that x.y.z will work, if in the future the implementation changes.

Well you can say also if x.y.z works no reason to belive x.y.z.w works. But the transitivity was not the problem in this issue.

Nevertheless, ote that the test that we want here is to check that the length of built-in functions is correct.

How I understood it from the issue report and discussion is that this is not the issue. The built-in functions are just functions, so they have the same properties as other functions, and length is just one property same as name for example.

That string's built-in matchAll is a function I tested with first test. That you can access function's properties I tested with second test.

Now perhaps we can make an additional set of tests on the Function builtin where we assure that it has all the properties and methods a Function needs to have (like length, name, apply, bind etc) and also an additional set of tests on Strings that they contain all the required properties and methods, but I see this as being out of the scope of this issue.

[Razican]

I think it's on the scope of the issue, from @jasonwilliams's comment:

So, outstanding on this is:
1. check the original problem in the OP is solved
2. write a test that creates a function then checks it's length property is correct
3. write a test that checks a built-in function's length is correct

[croraf]

I saw this. I don't agree that that is related to the issue as I explained above. But we can agree to disagree.

[jasonwilliams]

Now perhaps we can make an additional set of tests on the Function builtin where we assure that it has all the properties and methods a Function needs to have (like length, name, apply, bind etc) and also an additional set of tests on Strings that they contain all the required properties and methods, but I see this as being out of the scope of this issue.

Checking length is not out of scope of this issue.
This issue is specifically about the property length being available on builtin functions, and that’s what we need to test for.

Your fix did allow us to access the built in methods of objects (thanks to boxing up the primitives) but it hasn’t added any checks for that, only that methods can be accessed on objects.

[croraf]

Your fix did allow us to access the built in methods of objects (thanks to boxing up the primitives) but it hasn’t added any checks for that, only that methods can be accessed on objects.

This is incorrect. I checked that method matchAll can be accessed on the string primitive (both with dot notation and with the bracket notation) and that the access of this method returns something which is of function type (meaning it behaves like a function having all of its methods). https://github.com/boa-dev/boa/blob/master/boa/src/exec/tests.rs#L4-L19

I'm thinking now, perhaps builtin methods are not constructed from a function prototype but set its type function in different way, which would be a problem?

[Razican]

I'm thinking now, perhaps builtin methods are not constructed from a function prototype but set its type function in different way, which would be a problem?

Yep, builtins are created using Rust functions and added to objects. As an example: https://github.com/boa-dev/boa/blob/master/boa/src/builtins/bigint/mod.rs#L149-L165

[Razican]

I think this is fixed now, right? I can get the proper results using both methods.

I'm closing this for now, and removing the milestone, since it seems it was solved at least on 0.9.