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 abstract operation GetPrototypeFromConstructor #1561

Merged
merged 2 commits into from
Sep 8, 2021

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Sep 7, 2021

This Pull Request reduces the repetition on constructors when obtaining the default prototype of an object.
Extracted the pattern from #1552, thanks to @Razican :)

It changes the following:

  • Implements the abstract operation GetPrototypeFromConstructor

  • Replaces the pattern:

let proto = new_target
        .as_object()
        .and_then(|obj| {
            obj.__get__(&PROTOTYPE.into(), obj.clone().into(), context)
                .map(|o| o.as_object())
                .transpose()
        })
        .transpose()?
        .unwrap_or_else(default_proto);

with

let proto = get_prototype_from_constructor(
                new_target,
                |intrinsics| intrinsics.object_object().prototype(),
                context,
            )?;

This also allows for an easy replacement of intrinsics with the intrinsic objects of a realm when we eventually implement multiple realms.

@github-actions
Copy link

github-actions bot commented Sep 7, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 80,685 80,685 0
Passed 32,968 32,981 +13
Ignored 15,818 15,818 0
Failed 31,899 31,886 -13
Panics 0 0 0
Conformance 40.86% 40.88% +0.02%
Fixed tests (13):
test/language/statements/function/13.2-12-s.js [strict mode] (previously Failed)
test/language/statements/function/13.2-12-s.js (previously Failed)
test/language/statements/function/13.2-20-s.js (previously Failed)
test/built-ins/Function/prototype/apply/S15.3.4.3_A1_T1.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/apply/S15.3.4.3_A1_T1.js (previously Failed)
test/built-ins/Function/prototype/apply/S15.3.4.3_A7_T9.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/apply/S15.3.4.3_A7_T9.js (previously Failed)
test/built-ins/Function/prototype/call/S15.3.4.4_A6_T9.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/call/S15.3.4.4_A6_T9.js (previously Failed)
test/built-ins/Function/prototype/call/S15.3.4.4_A1_T1.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/call/S15.3.4.4_A1_T1.js (previously Failed)
test/built-ins/String/prototype/slice/S15.5.4.13_A1_T5.js [strict mode] (previously Failed)
test/built-ins/String/prototype/slice/S15.5.4.13_A1_T5.js (previously Failed)

@Razican
Copy link
Member

Razican commented Sep 7, 2021

Should this be implemented as part of Context?

@raskad
Copy link
Member

raskad commented Sep 7, 2021

I'm not sure if the intrinsicDefaultProto should just be a FnOnce(&Context) -> JsObject. It is not probable, but I think this function could be misused with that definition. I think if we implement this, we should honor the 1. Assert: intrinsicDefaultProto is this specification's name of an intrinsic object. somewhat. Maybe we could use an enum as the argument and do the Context.standard_objects().some_object().prototype() in the function?

The spec defines this as an ordinary object internal method, so I think the place is good.

@jedel1043
Copy link
Member Author

I'm not sure if the intrinsicDefaultProto should just be a FnOnce(&Context) -> JsObject. It is not probable, but I think this function could be misused with that definition. I think if we implement this, we should honor the 1. Assert: intrinsicDefaultProto is this specification's name of an intrinsic object. somewhat. Maybe we could use an enum as the argument and do the Context.standard_objects().some_object().prototype() in the function?

The spec defines this as an ordinary object internal method, so I think the place is good.

I could pass a reference to StandardObjects instead of Context, and that way we avoid the boilerplate and the giant match of 42 variants

@jedel1043 jedel1043 force-pushed the get_proto_from_cons branch 2 times, most recently from 4248dc4 to bc5bfc9 Compare September 7, 2021 22:20
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I commented on a few places where we use object_object() while a specific StandardConstructor exists. I'm curious if changing those would result in actual changes.

boa/src/builtins/boolean/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/function/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/string/mod.rs Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member Author

Looks good to me.

I commented on a few places where we use object_object() while a specific StandardConstructor exists. I'm curious if changing those would result in actual changes.

Let's see if this works

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Nice work!

@jedel1043
Copy link
Member Author

Nice work!

Good catch!

@Razican
Copy link
Member

Razican commented Sep 8, 2021

I think it would be easier to implement it by using FnOnce(&StandardObjects) -> &StandardConstructor. I did this in #1552 in my latest changes, but it would allow passing StandardObjects::typed_array_object and things like that, so directly the function.

@jedel1043
Copy link
Member Author

I think it would be easier to implement it by using FnOnce(&StandardObjects) -> &StandardConstructor. I did this in #1552 in my latest changes, but it would allow passing StandardObjects::typed_array_object and things like that, so directly the function.

Yep, this is the best option.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Now it looks great! :) thanks!

@Razican Razican added this to the v0.13.0 milestone Sep 8, 2021
@Razican Razican added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Sep 8, 2021
@Razican Razican merged commit c91af15 into master Sep 8, 2021
@Razican Razican deleted the get_proto_from_cons branch September 8, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants