boa

boa copied to clipboard

This Pull Request closes #1975. It's still a work in progress, but tries to go in that direction.

It changes the following:

• Adds a new TryFromJs trait, that can be derived using a new boa_derive crate.
• Adds a new try_js_into() function that, similarly to the standard library TryInto trait

Things to think about:

• Should the boa_derive crate be re-exported in boa_engine using a derive feature, similar to how it's done in serde?
• The current implementation only converts perfectly valid values. So, if we try to convert a big integer into an i8, or any floating point number to an f32. So, you cannot derive TryFromJs for structures that contain an f32 for example (you can still manually implement the trait, though, and decide in favour of a loss of precision). Should we also provide some traits for transparent loss of precision?
• Currently, you cannot convert between types, so if the JS struct has an integer, you cannot cast it to a boolean, for example. Should we provide a TryConvertJs trait, for example to force conversions?
• Currently we only have basic types and object conversions. Should add Array to Vec conversion, for example, right? Should we also add TypedArray conversions? What about Map and Set? Does this step over the fine grained APIs that we were creating?

Note that this still requires a bunch of documentation, tests, and validation from the dev team and from the users that requested this feature. I'm particularly interested in @lastmjs's thoughts on this API.

I already added an usage example in boa_examples/src/bin/derive.rs.

Test262 conformance changes

Test result	main count	PR count	difference
Total	94,277	94,277	0
Passed	71,100	71,100	0
Ignored	17,324	17,324	0
Failed	5,853	5,853	0
Panics	0	0	0
Conformance	75.42%	75.42%	0.00%

After some thoughts coming from the feedback of our users, this would need an extra attribute for each member, so that users can override the transformation function for a particular field. Similarly to deserialize_with in serde. I'll try to implement that next when I have some time.

We will also need to provide conversions for Vec, arrays, slices, maps, sets and so on, at least for the stuff in std.

I have added conversion overrides with attributes in the derive. This should make it easy to implement specific conversions for types that either do not implement TryFromJs or for types where we want to be a bit more flexible.

The example in boa_examples/src/bin/derive.rs uses this override for one of the fields.

Since this was a feature requested by @lastmjs, I would like to get some feedback and see if he likes this approach.

This still requires some clean-up, since the coding style of this is not 100% idiomatic.

What if we want to override these traits for the primitive types, like u128? The solution we've moved to is just implementing our own traits, TryIntoVmValue and TryFromVmValue. We implement the trait on all of our types, and in the implementations we either use the builtin boa conversions, or if necessary our own custom conversion.

For our use case, I'm thinking that overriding specific fields isn't really that useful. It's the specific types that we are concerned about. We only need one implementation per type as far as I know, so for every conceivable struct or enum, we might want to override the implementations for certain types, especially the primitives.

So thinking of a hypothetical scenario, we could swap out our TryIntoVmValue and TryFromVmValue for Boa's, and if we could override the implementation just on a few specific types, then all would work well. Unfortunately I'm not sure how possible that is, so we might just need to keep our custom traits.

What if we want to override these traits for the primitive types, like u128? The solution we've moved to is just implementing our own traits, TryIntoVmValue and TryFromVmValue. We implement the trait on all of our types, and in the implementations we either use the builtin boa conversions, or if necessary our own custom conversion.

If just a few types will be overridden, couldn't you just use an intermediary newtype, and implement TryFromJs/TryIntoJs for it? Or is it too intrusive for your specific API?

I'm setting this as "ready for review", since there are a few things that can be implemented, but, I don't have the needed time right now, and this is already useful as-is. Things for the future:

• Conversion back from Rust to JS (which currently would require to use a native object or the Into trait)
• Conversions of TypedArray. I tried to implement this, but it's not so simple to get the buffer and convert it to the proper vector in Rust. This also requires some decisions:
  • Do we want to clone the buffer? (we are doing this for BigInt, btw
  • Do we want to "take" the buffer and leave the JS object without it? This would mean that subsequent calls inside JavaScript would get different results.

I think this is the minimal implementation that we can provide without making sacrifices in the long run.

I think this is ready for review. Could we merge it as-is?

In the future, we could use this trait to convert Vec<u8> and others from ArrayBuffer or TypedArray. It would be nice to also have a similar trait for the Rust to JS conversions, but I guess this can already be reviewed and merged.

Does it make sense to put the macros in boa_macros instead of adding the new crate? In boa_engine we already depend on boa_macros anyway.

Does it make sense to put the macros in boa_macros instead of adding the new crate? In boa_engine we already depend on boa_macros anyway.

Indeed, this was started when boa_macros didn't exist. I have now moved it into boa_macros, and re-exported the derive in value::TryFromJs, so now, importing the trait automatically imports the derive too.

I'm unable to reproduce the tarpaulin error locally :/

I'm unable to reproduce the tarpaulin error locally :/

Me neither. My only guess is that it may be caused by the cyclic dependency between boa_macros and boa_engine. I would suggest we test if it works when we remove the boa_engine dev-dependency from boa_macros.

Seconding the cyclical dependency guess raskad mentioned above. I ran into something similar with cyclical dependencies when adding Trace and Finalize.

Would it be worthwhile to move the derive macro into a derive_macro folder under boa_engine and then re-export the value for a public use in boa_macros? I think Bevy does something similar to this where crates own their derive macros.

I'm pretty sure derive crates don't test their implementations internally. For example, serde has a serde_test crate on its workspace precisely for this reason. If we want to test, we can either:

• Create a boa_test crate for integration tests (We could also move all integration tests there, in order to speedup compilation times).

• Move the boa_derive tests to boa_engine.

I'm tending towards the first option, because we have a LOT of integration tests in a single, massive file and we could benefit from an exclusive crate for tests.

I'm pretty sure derive crates don't test their implementations internally. For example, serde has a serde_test crate on its workspace precisely for this reason. If we want to test, we can either:

• Create a boa_test crate for integration tests (We could also move all integration tests there, in order to speedup compilation times).

• Move the boa_derive tests to boa_engine.

I'm tending towards the first option, because we have a LOT of integration tests in a single, massive file and we could benefit from an exclusive crate for tests.

I guess this is the best option. I would say that for now, I can just move these tests to boa_engine, and then we can create another PR to add all the tests into a crate.

Rebased :) feel free to review it and see if we can already merge it as-is (and improve it later), or if you'd like me to add some more stuff

It seems that tarpaulin is failing somehow, but I cannot reproduce it in a Mac

I have rebased this and updated the code for syn 2.0, which is now much cleaner :)

@HalidOdat, @raskad, @jedel1043, @RageKnify, @nekevss, @jasonwilliams did you have the chance to run cargo tarpaulin locally to see if you can reproduce the error?

error: Broken pipe (os error 32)
warning: build failed, waiting for other jobs to finish...
error: could not compile `boa_cli` due to 3 previous errors
Mar 25 19:08:10.059 ERROR cargo_tarpaulin: Failed to compile tests!
error[E0308]: mismatched types
   --> boa_cli/src/main.rs:196:29
    |
196 |     boa_parser::Parser::new(Source::from_bytes(&src))
    |     ----------------------- ^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `boa_parser::Source`, found struct `boa_engine::Source`
    |     |
    |     arguments to this function are incorrect
    |
    = note: struct `boa_engine::Source` and struct `boa_parser::Source` have similar names, but are actually distinct types
note: struct `boa_engine::Source` is defined in crate `boa_parser`
   --> /home/runner/work/boa/boa/boa_parser/src/source.rs:12:1
    |
12  | pub struct Source<'path, R> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: struct `boa_parser::Source` is defined in crate `boa_parser`
   --> /home/runner/work/boa/boa/boa_parser/src/source.rs:12:1
    |
12  | pub struct Source<'path, R> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `boa_parser` are being used?
note: associated function defined here
   --> /home/runner/work/boa/boa/boa_parser/src/parser/mod.rs:121:12
    |
121 |     pub fn new(source: Source<'a, R>) -> Self {
    |            ^^^


Error: "Failed to compile tests!\nerror[E0308]: mismatched types\n   --> boa_cli/src/main.rs:196:29\n    |\n196 |     boa_parser::Parser::new(Source::from_bytes(&src))\n    |     ----------------------- ^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `boa_parser::Source`, found struct `boa_engine::Source`\n    |     |\n    |     arguments to this function are incorrect\n    |\n    = note: struct `boa_engine::Source` and struct `boa_parser::Source` have similar names, but are actually distinct types\nnote: struct `boa_engine::Source` is defined in crate `boa_parser`\n   --> /home/runner/work/boa/boa/boa_parser/src/source.rs:12:1\n    |\n12  | pub struct Source<'path, R> {\n    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^\nnote: struct `boa_parser::Source` is defined in crate `boa_parser`\n   --> /home/runner/work/boa/boa/boa_parser/src/source.rs:12:1\n    |\n12  | pub struct Source<'path, R> {\n    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^\n    = note: perhaps two different versions of crate `boa_parser` are being used?\nnote: associated function defined here\n   --> /home/runner/work/boa/boa/boa_parser/src/parser/mod.rs:121:12\n    |\n121 |     pub fn new(source: Source<'a, R>) -> Self {\n    |            ^^^\n\n"

I cannot reproduce it from my machine. I think it's probably a caching issue.

Let's see if this works...

Oh, so it isn't caching, it's the tarpaulin action itself apparently...

So neither the cache nor the action are the cause...

Codecov Report

Merging #2276 (5bff2ff) into main (6474dda) will decrease coverage by 0.50%. The diff coverage is 3.81%.

@@            Coverage Diff             @@
##             main    #2276      +/-   ##
==========================================
- Coverage   51.47%   50.98%   -0.50%     
==========================================
  Files         405      409       +4     
  Lines       40347    40759     +412     
==========================================
+ Hits        20768    20780      +12     
- Misses      19579    19979     +400     

Impacted Files	Coverage Δ
boa_engine/src/value/conversions/serde_json.rs	74.19% <ø> (ø)
boa_engine/src/value/conversions/try_from_js.rs	0.00% <0.00%> (ø)
boa_engine/src/value/mod.rs	65.77% <ø> (ø)
boa_examples/src/bin/derive.rs	0.00% <0.00%> (ø)
boa_macros/src/lib.rs	0.00% <0.00%> (ø)
boa_engine/src/value/conversions/mod.rs	58.92% <56.25%> (ø)

... and 22 files 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.

Huh, removing the cyclic test dep fixed it... I see two possible explanations:

• A Tarpaulin bug on certain environments with cyclic dependencies
• Removing the tests triggered a recompilation

Was about to check if re adding the tests fixed the error but I had to go out, so feel free to uncomment the commented out code, otherwise I'll do that in a couple of hours.

I'll try to move the tests to a new crate.

It seems this solves the issue :) Should we merge this?

It seems this solves the issue :) Should we merge this?

Sounds good!

bors r+

:clock1: Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

bors r+

Pull request successfully merged into main.

Build succeeded:

• Build and Test (macos-latest)
• Build and Test (windows-latest)
• Coverage
• Misc