[Merged by Bors] - Implement `escape` and `unescape` #2768

jedel1043 · 2023-03-31T06:44:06Z

This Pull Request implements the escape and unescape functions.

Both are technically deprecated, but they're also part of the Additional ECMAScript Features for Web Browsers section, so it is preferable to have them in place.

github-actions · 2023-03-31T06:54:16Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	94,343	94,343	0
Passed	71,100	71,170	+70
Ignored	17,386	17,386	0
Failed	5,857	5,787	-70
Panics	0	0	0
Conformance	75.36%	75.44%	+0.07%

Fixed tests (70):

test/annexB/built-ins/escape/to-string-err-symbol.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/to-string-err-symbol.js (previously Failed)
test/annexB/built-ins/escape/length.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/length.js (previously Failed)
test/annexB/built-ins/escape/empty-string.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/empty-string.js (previously Failed)
test/annexB/built-ins/escape/unmodified.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/unmodified.js (previously Failed)
test/annexB/built-ins/escape/to-string-err.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/to-string-err.js (previously Failed)
test/annexB/built-ins/escape/escape-above-astral.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/escape-above-astral.js (previously Failed)
test/annexB/built-ins/escape/escape-above.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/escape-above.js (previously Failed)
test/annexB/built-ins/escape/name.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/name.js (previously Failed)
test/annexB/built-ins/escape/to-primitive-observe.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/to-primitive-observe.js (previously Failed)
test/annexB/built-ins/escape/argument_types.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/argument_types.js (previously Failed)
test/annexB/built-ins/escape/prop-desc.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/prop-desc.js (previously Failed)
test/annexB/built-ins/escape/not-a-constructor.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/not-a-constructor.js (previously Failed)
test/annexB/built-ins/escape/to-string-observe.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/to-string-observe.js (previously Failed)
test/annexB/built-ins/escape/escape-below.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/escape-below.js (previously Failed)
test/annexB/built-ins/escape/to-primitive-err.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/to-primitive-err.js (previously Failed)
test/annexB/built-ins/escape/argument_bigint.js [strict mode] (previously Failed)
test/annexB/built-ins/escape/argument_bigint.js (previously Failed)
test/annexB/built-ins/unescape/to-string-err-symbol.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/to-string-err-symbol.js (previously Failed)
test/annexB/built-ins/unescape/two-ignore-non-hex.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/two-ignore-non-hex.js (previously Failed)
test/annexB/built-ins/unescape/two.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/two.js (previously Failed)
test/annexB/built-ins/unescape/four-ignore-non-hex.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/four-ignore-non-hex.js (previously Failed)
test/annexB/built-ins/unescape/length.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/length.js (previously Failed)
test/annexB/built-ins/unescape/empty-string.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/empty-string.js (previously Failed)
test/annexB/built-ins/unescape/to-string-err.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/to-string-err.js (previously Failed)
test/annexB/built-ins/unescape/two-ignore-end-str.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/two-ignore-end-str.js (previously Failed)
test/annexB/built-ins/unescape/four-ignore-bad-u.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/four-ignore-bad-u.js (previously Failed)
test/annexB/built-ins/unescape/name.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/name.js (previously Failed)
test/annexB/built-ins/unescape/to-primitive-observe.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/to-primitive-observe.js (previously Failed)
test/annexB/built-ins/unescape/argument_types.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/argument_types.js (previously Failed)
test/annexB/built-ins/unescape/prop-desc.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/prop-desc.js (previously Failed)
test/annexB/built-ins/unescape/four.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/four.js (previously Failed)
test/annexB/built-ins/unescape/four-ignore-end-str.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/four-ignore-end-str.js (previously Failed)
test/annexB/built-ins/unescape/not-a-constructor.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/not-a-constructor.js (previously Failed)
test/annexB/built-ins/unescape/to-string-observe.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/to-string-observe.js (previously Failed)
test/annexB/built-ins/unescape/to-primitive-err.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/to-primitive-err.js (previously Failed)
test/annexB/built-ins/unescape/argument_bigint.js [strict mode] (previously Failed)
test/annexB/built-ins/unescape/argument_bigint.js (previously Failed)

codecov · 2023-03-31T07:02:57Z

Codecov Report

Merging #2768 (d0fe6d1) into main (540aa2b) will decrease coverage by 0.07%.
The diff coverage is 24.44%.

@@            Coverage Diff             @@
##             main    #2768      +/-   ##
==========================================
- Coverage   50.98%   50.92%   -0.07%     
==========================================
  Files         409      410       +1     
  Lines       40759    40849      +90     
==========================================
+ Hits        20781    20802      +21     
- Misses      19978    20047      +69

Impacted Files	Coverage Δ
boa_tester/src/edition.rs	`0.00% <ø> (ø)`
boa_engine/src/builtins/escape/mod.rs	`15.00% <15.00%> (ø)`
boa_engine/src/builtins/mod.rs	`100.00% <100.00%> (ø)`
boa_engine/src/context/intrinsics.rs	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Razican

It looks great to me, but I would put this behind a feature flag, since this should only be enabled for browsers:

When the ECMAScript host is a web browser the following additional properties of the standard built-in objects are defined.

jedel1043 · 2023-03-31T09:52:10Z

It looks great to me, but I would put this behind a feature flag, since this should only be enabled for browsers:

When the ECMAScript host is a web browser the following additional properties of the standard built-in objects are defined.

Thought about it, but I checked Node and Deno and both have these functions enabled, probably because of compatibility purposes... Though, we have plans for feature gating all builtins anyways, so maybe it should be fine to gate them, then add them to the default set of features?

Razican · 2023-03-31T10:33:17Z

It looks great to me, but I would put this behind a feature flag, since this should only be enabled for browsers:

When the ECMAScript host is a web browser the following additional properties of the standard built-in objects are defined.

Thought about it, but I checked Node and Deno and both have these functions enabled, probably because of compatibility purposes... Though, we have plans for feature gating all builtins anyways, so maybe it should be fine to gate them, then add them to the default set of features?

I would feature gate them and add them to the default set, but in the future, I can imagine having:

default: base + built-ins + browser (anex B) + intl
base: basic language, no built-ins
built-ins: non-anex-B built-ins
intl: intl module

Then, I would like to have one feature for each feature in test262, ideally, or at least one feature per built-in + async/await + generators... in case users would like to block some functionality. They should also be able to create a context with no features and cherry pick the built-ins at runtime, for example.

But this is out of this PR, of course. I would still add the feature gate, though :)

raskad · 2023-04-01T16:13:56Z

Right now this is missing Escape::init(&intrinsics); and Unescape::init(&intrinsics); in Intrinsics::new(). The functions are just empty objects, that is why only the negative tests are passing. After that there are two tests in unescape that are failing, but I'm currently not sure why.

jedel1043 · 2023-04-02T00:24:41Z

Right now this is missing Escape::init(&intrinsics); and Unescape::init(&intrinsics); in Intrinsics::new(). The functions are just empty objects, that is why only the negative tests are passing. After that there are two tests in unescape that are failing, but I'm currently not sure why.

Remind me to not push changes at 4am again 🙃 Thanks for the catch!

jedel1043 · 2023-04-03T19:10:35Z

Ok, all tests are passing for me and I gated the functions behind an annex-b feature.

boa_engine/src/context/intrinsics.rs

HalidOdat

Looks good to me! :)

raskad · 2023-04-03T22:18:36Z

bors r+

This Pull Request implements the [`escape`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/escape) and [`unescape`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape) functions. Both are technically deprecated, but they're also part of the [Additional ECMAScript Features for Web Browsers](https://tc39.es/ecma262/#sec-additional-ecmascript-features-for-web-browsers) section, so it is preferable to have them in place.

bors · 2023-04-03T22:37:13Z

Pull request successfully merged into main.

Build succeeded:

Implement escape and unescape

b8bae39

jedel1043 added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Mar 31, 2023

jedel1043 added this to the v0.17.0 milestone Mar 31, 2023

jedel1043 requested review from Razican, jasonwilliams, HalidOdat, RageKnify, raskad and nekevss March 31, 2023 06:44

Razican reviewed Mar 31, 2023

View reviewed changes

Initialize intrinsic functions

74dbec3

jedel1043 marked this pull request as draft April 2, 2023 17:05

Fix edge cases

d31c725

jedel1043 marked this pull request as ready for review April 3, 2023 18:49

Gate escape and unescape

1a337a6

jedel1043 requested a review from Razican April 3, 2023 19:10

HalidOdat reviewed Apr 3, 2023

View reviewed changes

boa_engine/src/context/intrinsics.rs Outdated Show resolved Hide resolved

Razican reviewed Apr 3, 2023

View reviewed changes

boa_engine/src/context/intrinsics.rs Show resolved Hide resolved

Gate missing intrinsic objects

5b049be

HalidOdat reviewed Apr 3, 2023

View reviewed changes

boa_engine/src/context/intrinsics.rs Outdated Show resolved Hide resolved

Fix annotation

d0fe6d1

jedel1043 requested a review from Razican April 3, 2023 21:20

jedel1043 requested a review from HalidOdat April 3, 2023 21:20

HalidOdat approved these changes Apr 3, 2023

View reviewed changes

raskad approved these changes Apr 3, 2023

View reviewed changes

bors bot changed the title ~~Implement escape and unescape~~ [Merged by Bors] - Implement escape and unescape Apr 3, 2023

bors bot closed this Apr 3, 2023

bors bot deleted the escape-unescape branch April 3, 2023 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Implement `escape` and `unescape` #2768

[Merged by Bors] - Implement `escape` and `unescape` #2768

jedel1043 commented Mar 31, 2023

github-actions bot commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

Razican left a comment

jedel1043 commented Mar 31, 2023

Razican commented Mar 31, 2023

raskad commented Apr 1, 2023

jedel1043 commented Apr 2, 2023

jedel1043 commented Apr 3, 2023

HalidOdat left a comment •

edited

Loading

raskad commented Apr 3, 2023

bors bot commented Apr 3, 2023

[Merged by Bors] - Implement escape and unescape #2768

[Merged by Bors] - Implement escape and unescape #2768

Conversation

jedel1043 commented Mar 31, 2023

github-actions bot commented Mar 31, 2023 • edited Loading

Test262 conformance changes

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

Razican left a comment

Choose a reason for hiding this comment

jedel1043 commented Mar 31, 2023

Razican commented Mar 31, 2023

raskad commented Apr 1, 2023

jedel1043 commented Apr 2, 2023

jedel1043 commented Apr 3, 2023

HalidOdat left a comment • edited Loading

Choose a reason for hiding this comment

raskad commented Apr 3, 2023

bors bot commented Apr 3, 2023

[Merged by Bors] - Implement `escape` and `unescape` #2768

[Merged by Bors] - Implement `escape` and `unescape` #2768

github-actions bot commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

HalidOdat left a comment •

edited

Loading