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

JsValue from primitives #1993

Closed
wants to merge 6 commits into from

Conversation

lastmjs
Copy link
Contributor

@lastmjs lastmjs commented Mar 31, 2022

This Pull Request fixes/closes #1994 and #1991 and depends on #1971.

It changes the following:

  • Adds many primitive conversions, From for JsValue where T are integers
  • Makes some opinions about i128, u128, i64, and u64 when converting to JsValue, always converting to BigInt

@lastmjs lastmjs changed the title Js value from primitives JsValue from primitives Mar 31, 2022
@jedel1043
Copy link
Member

jedel1043 commented Mar 31, 2022

This change reduces our conformance by a very large amount. The reason is the conversion for i64, IIRC.

The range of an integer index in javascript is, surprisingly, [0 to 2^53), which is the max integer that is representable by an f64 without loss of precision. When we parse an integer, we use i32 to store it for performance reasons, and fallback to f64 in case the integer exceeds the max value of an i32, because converting from f64 to u64 is faster than converting from JsBigInt to u64.
However, eagerly converting it to JsBigInt automatically makes it unusable as an integer index, and it's instead converted to a string key. This unfortunately breaks indexing for big integer values.

@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #1993 (39e2799) into main (6baf455) will decrease coverage by 0.03%.
The diff coverage is 3.33%.

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
- Coverage   45.89%   45.85%   -0.04%     
==========================================
  Files         206      206              
  Lines       17150    17170      +20     
==========================================
+ Hits         7871     7874       +3     
- Misses       9279     9296      +17     
Impacted Files Coverage Δ
boa_engine/src/bigint.rs 37.95% <0.00%> (-1.15%) ⬇️
boa_engine/src/value/mod.rs 51.09% <0.00%> (-0.76%) ⬇️
boa_engine/src/value/conversions.rs 19.64% <4.54%> (-5.36%) ⬇️
boa_engine/src/builtins/string/mod.rs 62.74% <0.00%> (+0.21%) ⬆️
...ine/src/syntax/parser/expression/assignment/mod.rs 30.84% <0.00%> (+1.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6baf455...39e2799. Read the comment docs.

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!

@raskad raskad added this to the v0.15.0 milestone Apr 3, 2022
@raskad raskad added enhancement New feature or request API labels Apr 3, 2022
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.

As already mentioned in multiple comments, I don't think it's the way forward. This already silently heap-allocates with a TON of overhead many Rust stack primitives. you can already do these conversions explicitly, and there are talks for adding easier boilerplating with a trait and maybe some derives.

@Razican Razican removed this from the v0.15.0 milestone Jun 1, 2022
@lastmjs
Copy link
Contributor Author

lastmjs commented Dec 16, 2022

I'm going to close this pull request in response to the comments.

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

Successfully merging this pull request may close these issues.

Implement From<T> for JsValue where T are integers
5 participants