From 0b8cc192b6c99f27b2ebfbff40acd7cc0db11bd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= Date: Tue, 11 Jun 2024 10:22:40 -0400 Subject: [PATCH] Add 262 JSON tests (#662) * Add the `javy-test-macros` crate This commit introduces the `javy-test-macros` crate to the existing family of crates. This crate is intended to be a dev-dependency crate used to test javy through the embedding API. As of this commit, this crate exposes the bare minimum functionality to test the implementation of JSON.{parse,stringify} introduced in https://github.com/bytecodealliance/javy/pull/651. * Test the JSON.{parse, stringify} implementation This commit adds `javy-test-macros` as a development dependency in the `javy` crate and adds 262 test for Javy's custom implementation of `JSON.{parse,stringigy}`. The 262 suite is pulled in as a submodule. In order to test the custom implementation, this commit explicitly calls `config.override_json_parse_and_stringify` when creating the default runtime used in Javy. The override configuration was introduced in https://github.com/bytecodealliance/javy/pull/651. * chore: Checkout submodules in the CI workflow To pull-in 262 test suite. * chore: Remove `--test-threads=1` This was added for development purposes. * chore: Clippy fixes * chore: Define wasi runner in ci * Gate 262 tests under the json feature * Vet dependencies * chore: Remove stale TODO * Remove commented out test There's nothing in the spec stating that the key must be a string. * Fix Cargo.toml * Address review comments * Fix unbalanced double quote --- .github/workflows/ci.yml | 4 + .gitmodules | 3 + Cargo.lock | 15 +- Cargo.toml | 1 + Makefile | 2 +- crates/core/src/runtime.rs | 1 + crates/javy-test-macros/Cargo.toml | 16 ++ crates/javy-test-macros/README.md | 3 + crates/javy-test-macros/src/lib.rs | 174 ++++++++++++++++++++ crates/javy/Cargo.toml | 3 + crates/javy/src/apis/json.rs | 93 ++++++++--- crates/javy/src/json.rs | 3 +- crates/javy/src/lib.rs | 35 +++- crates/javy/src/serde/de.rs | 252 +++++++++++++++++++++++++---- crates/javy/src/serde/err.rs | 15 +- crates/javy/src/serde/ser.rs | 102 +++++------- crates/javy/test262 | 1 + crates/javy/tests/262.rs | 2 + supply-chain/config.toml | 4 - supply-chain/imports.lock | 10 +- 20 files changed, 606 insertions(+), 133 deletions(-) create mode 100644 crates/javy-test-macros/Cargo.toml create mode 100644 crates/javy-test-macros/README.md create mode 100644 crates/javy-test-macros/src/lib.rs create mode 160000 crates/javy/test262 create mode 100644 crates/javy/tests/262.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b233d4a..7fef738d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,8 @@ jobs: runs-on: macos-latest steps: - uses: actions/checkout@v4 + with: + submodules: true - uses: ./.github/actions/ci-shared-setup with: @@ -32,6 +34,8 @@ jobs: run: cargo build -p javy-core --release --target=wasm32-wasi - name: Test + env: + CARGO_TARGET_WASM32_WASI_RUNNER: wasmtime --dir=. run: cargo hack wasi test --workspace --exclude=javy-cli --each-feature -- --nocapture - name: Lint diff --git a/.gitmodules b/.gitmodules index fb6c51c3..094d793d 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,6 @@ [submodule "wpt/upstream"] path = wpt/upstream url = https://github.com/web-platform-tests/wpt +[submodule "crates/javy/test262"] + path = crates/javy/test262 + url = git@github.com:tc39/test262.git diff --git a/Cargo.lock b/Cargo.lock index 00035ac8..8b273f8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1517,6 +1517,7 @@ dependencies = [ "anyhow", "bitflags 2.5.0", "fastrand", + "javy-test-macros", "quickcheck", "rmp-serde", "rquickjs", @@ -1566,6 +1567,16 @@ dependencies = [ "once_cell", ] +[[package]] +name = "javy-test-macros" +version = "1.4.0" +dependencies = [ + "anyhow", + "proc-macro2", + "quote", + "syn 2.0.66", +] + [[package]] name = "jobserver" version = "0.1.31" @@ -2108,9 +2119,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.84" +version = "1.0.85" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6" +checksum = "22244ce15aa966053a896d1accb3a6e68469b97c7f33f284b99f0d576879fc23" dependencies = [ "unicode-ident", ] diff --git a/Cargo.toml b/Cargo.toml index 2857c284..708f174c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,7 @@ members = [ "crates/apis", "crates/core", "crates/cli", + "crates/javy-test-macros", ] resolver = "2" diff --git a/Makefile b/Makefile index 12af60b1..b642ae0e 100644 --- a/Makefile +++ b/Makefile @@ -22,7 +22,7 @@ docs: cargo doc --package=javy-core --open --target=wasm32-wasi test-javy: - cargo wasi test --package=javy --features json,messagepack -- --nocapture + CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime --dir=." cargo wasi test --package=javy --features json,messagepack -- --nocapture test-core: cargo wasi test --package=javy-core -- --nocapture diff --git a/crates/core/src/runtime.rs b/crates/core/src/runtime.rs index be5d42ce..15112bfe 100644 --- a/crates/core/src/runtime.rs +++ b/crates/core/src/runtime.rs @@ -7,6 +7,7 @@ pub(crate) fn new_runtime() -> Result { .text_encoding(true) .redirect_stdout_to_stderr(true) .javy_stream_io(true) + .override_json_parse_and_stringify(true) .javy_json(true); Runtime::new(std::mem::take(config)) diff --git a/crates/javy-test-macros/Cargo.toml b/crates/javy-test-macros/Cargo.toml new file mode 100644 index 00000000..a52c0766 --- /dev/null +++ b/crates/javy-test-macros/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "javy-test-macros" +version.workspace = true +authors.workspace = true +edition.workspace = true +license.workspace = true + +[lib] +proc-macro = true +doctest = false + +[dependencies] +anyhow = { workspace = true } +proc-macro2 = "1.0.85" +quote = "1.0.36" +syn = { version = "2.0.66", features = ["full"] } diff --git a/crates/javy-test-macros/README.md b/crates/javy-test-macros/README.md new file mode 100644 index 00000000..02da7ba2 --- /dev/null +++ b/crates/javy-test-macros/README.md @@ -0,0 +1,3 @@ +# Macro helpers for testing Javy + +See `src/lib.rs` for more details and usage. diff --git a/crates/javy-test-macros/src/lib.rs b/crates/javy-test-macros/src/lib.rs new file mode 100644 index 00000000..bbbf9e96 --- /dev/null +++ b/crates/javy-test-macros/src/lib.rs @@ -0,0 +1,174 @@ +use anyhow::bail; +/// Macros for testing Javy. +/// +/// Helper macros to define Test262 tests or tests that exercise different +/// configuration combinations. +/// +/// Currently only defining Test262 tests for JSON is supported. +/// +/// Usage +/// +/// ```rust +/// t262!(path = "path/to/262/directory") +/// ``` +use proc_macro::TokenStream; +use proc_macro2::Span; +use quote::quote; +use std::path::{Path, PathBuf}; +use syn::{parse_macro_input, Ident, LitStr, Result}; + +struct Config262 { + root: PathBuf, +} + +impl Config262 { + fn validate(&self) -> anyhow::Result<()> { + let path: Box = self.root.clone().into(); + if path.is_dir() { + Ok(()) + } else { + bail!("Invalid path") + } + } +} + +impl Default for Config262 { + fn default() -> Self { + Self { + root: PathBuf::new(), + } + } +} + +#[proc_macro] +pub fn t262(stream: TokenStream) -> TokenStream { + let mut config = Config262::default(); + + let config_parser = syn::meta::parser(|meta| { + if meta.path.is_ident("path") { + let lit: Option = Some(meta.value()?.parse()?); + + if let Some(s) = lit { + config.root = PathBuf::from(s.clone().value()); + } else { + return Err(meta.error("Expected String literal")); + } + config.validate().map_err(|e| meta.error(e)) + } else { + Err(meta.error("Unsupported property")) + } + }); + + parse_macro_input!(stream with config_parser); + + match expand(&config) { + Ok(tok) => tok, + Err(e) => e.into_compile_error().into(), + } +} + +// Should this test be ignored? +fn ignore(test_name: &str) -> bool { + [ + // A bit unfortunate, currently simd-json returns `0` for `'-0'`; + // I think this is a bug in simd-json itself. + "test_parse_text_negative_zero", + // Realms are not supported by QuickJS + "test_stringify_replacer_array_proxy_revoked_realm", + "test_stringify_value_bigint_cross_realm", + // TODO + // Currently the conversion between non-utf8 string encodings is lossy. + // There's probably a way to improve the interop. + "test_stringify_value_string_escape_unicode", + ] + .contains(&test_name) +} + +fn expand(config: &Config262) -> Result { + let harness = config.root.join("harness"); + let harness_str = harness.into_os_string().into_string().unwrap(); + let json_parse = config + .root + .join("test") + .join("built-ins") + .join("JSON") + .join("parse"); + + let json_stringify = config + .root + .join("test") + .join("built-ins") + .join("JSON") + .join("stringify"); + + let parse_tests = gen_tests(&json_parse, &harness_str, "parse"); + let stringify_tests = gen_tests(&json_stringify, &harness_str, "stringify"); + + Ok(quote! { + #parse_tests + #stringify_tests + } + .into()) +} + +fn gen_tests( + dir: &PathBuf, + harness_str: &String, + prefix: &'static str, +) -> proc_macro2::TokenStream { + let parse_dir = std::fs::read_dir(dir).expect("parse directory to be available"); + let spec = parse_dir.filter_map(|e| e.ok()).map(move |entry| { + let path = entry.path(); + let path_str = path.clone().into_os_string().into_string().unwrap(); + let name = path.file_stem().unwrap().to_str().unwrap(); + let name = name.replace('.', "_"); + let name = name.replace('-', "_"); + let test_name = Ident::new(&format!("test_{}_{}", prefix, name), Span::call_site()); + let ignore = ignore(&test_name.to_string()); + + let attrs = if ignore { + quote! { + #[ignore] + } + } else { + quote! {} + }; + + quote! { + #[test] + #attrs + #[allow(non_snake_case)] + fn #test_name() { + let mut config = ::javy::Config::default(); + config + .override_json_parse_and_stringify(true); + let runtime = ::javy::Runtime::new(config).expect("runtime to be created"); + let harness_path = ::std::path::PathBuf::from(#harness_str); + + let helpers = vec![ + harness_path.join("sta.js"), + harness_path.join("assert.js"), + harness_path.join("compareArray.js"), + harness_path.join("propertyHelper.js"), + harness_path.join("isConstructor.js") + ]; + runtime.context().with(|this| { + for helper in helpers { + let helper_contents = ::std::fs::read(helper).expect("helper path to exist"); + let r: ::javy::quickjs::Result<()> = this.eval_with_options(helper_contents, ::javy::quickjs::context::EvalOptions::default()).expect("helper evaluation to succeed"); + assert!(r.is_ok()); + } + + let test_contents = ::std::fs::read(&#path_str).expect("test file contents to be available"); + let r: ::javy::quickjs::Result<()> = this.eval_with_options(test_contents, ::javy::quickjs::context::EvalOptions::default()); + assert!(r.is_ok(), "{}", ::javy::val_to_string(this.clone(), this.catch()).unwrap()); + }); + + } + } + }); + + quote! { + #(#spec)* + } +} diff --git a/crates/javy/Cargo.toml b/crates/javy/Cargo.toml index 3e389997..b8c8c8ee 100644 --- a/crates/javy/Cargo.toml +++ b/crates/javy/Cargo.toml @@ -23,6 +23,9 @@ bitflags = "2.5.0" fastrand = "2.1.0" simd-json = { version = "0.13.10", optional = true, default-features = false, features = ["big-int-as-float", "serde_impl"] } +[dev-dependencies] +javy-test-macros = { path = "../javy-test-macros/" } + [features] export_alloc_fns = [] messagepack = ["rmp-serde", "serde-transcode"] diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index 1a2529e0..8ce64802 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -21,13 +21,18 @@ use crate::{ hold, hold_and_release, json, quickjs::{ context::Intrinsic, + function::This, prelude::{MutFn, Rest}, - qjs, Ctx, Function, Object, String as JSString, Value, + qjs, Ctx, Exception, Function, Object, String as JSString, Value, }, to_js_error, val_to_string, Args, }; -use anyhow::{anyhow, Result}; +use crate::serde::de::get_to_json; + +use simd_json::Error as SError; + +use anyhow::{anyhow, bail, Result}; use std::{ io::{Read, Write}, ptr::NonNull, @@ -52,20 +57,36 @@ impl Intrinsic for Json { } fn register<'js>(this: Ctx<'js>) -> Result<()> { + let global = this.globals(); + let json: Object = global.get("JSON")?; + let default_parse: Function = json.get("parse")?; + let default_stringify: Function = json.get("stringify")?; + let parse = Function::new( this.clone(), MutFn::new(move |cx: Ctx<'js>, args: Rest>| { - call_json_parse(hold!(cx.clone(), args)).map_err(|e| to_js_error(cx, e)) + call_json_parse(hold!(cx.clone(), args), default_parse.clone()) + .map_err(|e| to_js_error(cx, e)) }), )?; + // Explicitly set the function's name and length properties. + // In both the parse and the stringify case below, the spec tests + // assert that the name and length properties must be correctly set. + parse.set_length(2)?; + parse.set_name("parse")?; + let stringify = Function::new( this.clone(), MutFn::new(move |cx: Ctx<'js>, args: Rest>| { - call_json_stringify(hold!(cx.clone(), args)).map_err(|e| to_js_error(cx, e)) + call_json_stringify(hold!(cx.clone(), args), default_stringify.clone()) + .map_err(|e| to_js_error(cx, e)) }), )?; + stringify.set_name("stringify")?; + stringify.set_length(3)?; + let global = this.globals(); let json: Object = global.get("JSON")?; json.set("parse", parse)?; @@ -74,11 +95,14 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { Ok(()) } -fn call_json_parse(args: Args<'_>) -> Result { +fn call_json_parse<'a>(args: Args<'a>, default: Function<'a>) -> Result> { let (this, args) = args.release(); match args.len() { - 0 => Err(anyhow!("Error: \"undefined\" is not valid JSON")), + 0 => bail!(Exception::throw_syntax( + &this, + "\"undefined\" is not valid JSON" + )), 1 => { let val = args[0].clone(); // Fast path. Number and null are treated as identity. @@ -86,9 +110,23 @@ fn call_json_parse(args: Args<'_>) -> Result { return Ok(val); } + if val.is_symbol() { + bail!(Exception::throw_type(&this, "Expected string primitive")); + } + let mut string = val_to_string(this.clone(), args[0].clone())?; let bytes = unsafe { string.as_bytes_mut() }; - json::parse(this, bytes) + json::parse(this.clone(), bytes).map_err(|original| { + if original.downcast_ref::().is_none() { + return original; + } + + let e = match original.downcast_ref::() { + Some(e) => e.to_string(), + None => "JSON parse error".into(), + }; + anyhow!(Exception::throw_syntax(&this, &e)) + }) } _ => { // If there's more than one argument, defer to the built-in @@ -96,24 +134,37 @@ fn call_json_parse(args: Args<'_>) -> Result { // reviver argument. // // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#reviver. - let global = this.globals(); - let json: Object = global.get("JSON")?; - let parse: Function = json.get("parse")?; - - parse + default .call((args[0].clone(), args[1].clone())) - .map_err(|e| anyhow!("{e}")) + .map_err(|e| anyhow!(e)) } } } -fn call_json_stringify(args: Args<'_>) -> Result { +fn call_json_stringify<'a>(args: Args<'a>, default: Function<'a>) -> Result> { let (this, args) = args.release(); match args.len() { 0 => Ok(Value::new_undefined(this.clone())), 1 => { - let bytes = json::stringify(args[0].clone())?; + let arg = args[0].clone(); + let val: Value = if arg.is_object() { + if let Some(f) = get_to_json(&arg) { + f.call(( + This(arg.clone()), + JSString::from_str(arg.ctx().clone(), "")?.into_value(), + ))? + } else { + arg.clone() + } + } else { + arg.clone() + }; + if val.is_function() || val.is_undefined() || val.is_symbol() { + return Ok(Value::new_undefined(arg.ctx().clone())); + } + + let bytes = json::stringify(val.clone())?; let str = String::from_utf8(bytes)?; let str = JSString::from_str(this, &str)?; Ok(str.into_value()) @@ -123,18 +174,14 @@ fn call_json_stringify(args: Args<'_>) -> Result { // defer to the built-in JSON.stringify, which will take care of // validating invoking the replacer function and/or applying the // space argument. - let global = this.globals(); - let json: Object = global.get("JSON")?; - let stringify: Function = json.get("stringify")?; - if args.len() == 2 { - stringify + default .call((args[0].clone(), args[1].clone())) - .map_err(|e| anyhow!("{e}")) + .map_err(anyhow::Error::new) } else { - stringify + default .call((args[0].clone(), args[1].clone(), args[2].clone())) - .map_err(|e| anyhow!("{e}")) + .map_err(anyhow::Error::new) } } } diff --git a/crates/javy/src/json.rs b/crates/javy/src/json.rs index e621d1c6..bf266ea4 100644 --- a/crates/javy/src/json.rs +++ b/crates/javy/src/json.rs @@ -5,8 +5,9 @@ use anyhow::Result; /// Transcodes a byte slice containing a JSON encoded payload into a [Value]. pub fn parse<'js>(context: Ctx<'js>, bytes: &mut [u8]) -> Result> { let mut deserializer = simd_json::Deserializer::from_slice(bytes)?; - let mut serializer = Serializer::from_context(context)?; + let mut serializer = Serializer::from_context(context.clone())?; serde_transcode::transcode(&mut deserializer, &mut serializer)?; + Ok(serializer.value) } diff --git a/crates/javy/src/lib.rs b/crates/javy/src/lib.rs index 91ff3c96..09c79cab 100644 --- a/crates/javy/src/lib.rs +++ b/crates/javy/src/lib.rs @@ -103,8 +103,13 @@ macro_rules! hold { /// Handles a JavaScript error or exception and converts to [anyhow::Error]. pub fn from_js_error(ctx: Ctx<'_>, e: JSError) -> Error { if e.is_exception() { - let exception = ctx.catch().into_exception().unwrap(); - anyhow!("{exception}") + let val = ctx.catch(); + + if let Some(exception) = val.clone().into_exception() { + anyhow!("{exception}") + } else { + anyhow!(val_to_string(ctx, val).unwrap_or_else(|_| "Internal error".to_string())) + } } else { Into::into(e) } @@ -117,10 +122,28 @@ pub fn from_js_error(ctx: Ctx<'_>, e: JSError) -> Error { pub fn to_js_error(cx: Ctx, e: Error) -> JSError { match e.downcast::() { Ok(e) => e, - Err(e) => cx.throw(Value::from_exception( - Exception::from_message(cx.clone(), &e.to_string()) - .expect("creating an exception to succeed"), - )), + Err(e) => { + // In some cases the original error context is lost i.e. we can't + // retain the orginal JSError when invoking serde_transcode, + // particularly for json::stringify. The process of transcoding will + // report the Serializer error, which is totally implementation + // dependent, in this case particular to serde_json::Error. To + // workaround this, we identify the exception via its string + // representation. This is not ideal, but its also fine as it only + // happens in the transcoding case. + // + // Ref: https://github.com/sfackler/serde-transcode/issues/8 + if e.to_string() + .contains("JSError: Exception generated by QuickJS") + { + return JSError::Exception; + } + + cx.throw(Value::from_exception( + Exception::from_message(cx.clone(), &e.to_string()) + .expect("creating an exception to succeed"), + )) + } } } diff --git a/crates/javy/src/serde/de.rs b/crates/javy/src/serde/de.rs index 11be8ecd..f1f84c75 100644 --- a/crates/javy/src/serde/de.rs +++ b/crates/javy/src/serde/de.rs @@ -1,17 +1,30 @@ -use crate::quickjs::{object::ObjectIter, Array, Filter, Value}; +use crate::quickjs::{ + function::This, + object::ObjectIter, + qjs::{JS_GetClassID, JS_GetProperty}, + Array, Exception, Filter, String as JSString, Value, +}; use crate::serde::err::{Error, Result}; use crate::serde::{MAX_SAFE_INTEGER, MIN_SAFE_INTEGER}; -use crate::{from_js_error, to_string_lossy}; +use crate::to_string_lossy; use anyhow::anyhow; -use rquickjs::Null; +use rquickjs::{atom::PredefinedAtom, Function, Null}; use serde::de::{self, Error as SerError}; use serde::forward_to_deserialize_any; +// Class IDs, for internal, deserialization purposes only. +enum ClassId { + Number = 4, + String = 5, + Bool = 6, + BigInt = 33, +} + use super::as_key; impl SerError for Error { - fn custom(msg: T) -> Self { - Error::Custom(anyhow!(msg.to_string())) + fn custom(e: T) -> Self { + Error::Custom(anyhow!(e.to_string())) } } @@ -32,6 +45,8 @@ pub struct Deserializer<'js> { value: Value<'js>, map_key: bool, current_kv: Option<(Value<'js>, Value<'js>)>, + /// Stack to track circular dependencies. + stack: Vec>, } impl<'de> From> for Deserializer<'de> { @@ -40,6 +55,9 @@ impl<'de> From> for Deserializer<'de> { value, map_key: false, current_kv: None, + // We are probaby over allocating here. But it's probably fine to + // over allocate to avoid paying the cost of subsequent allocations. + stack: Vec::with_capacity(100), } } } @@ -78,6 +96,21 @@ impl<'js> Deserializer<'js> { } unreachable!() } + + /// When stringifying, circular dependencies are not allowed. This function + /// checks the current value stack to ensure that if the same value (tag and + /// bits) is found again a proper error is raised. + fn check_cycles(&self) -> Result<()> { + for val in self.stack.iter().rev() { + if self.value.eq(val) { + return Err(Error::from(Exception::throw_type( + val.ctx(), + "circular dependency", + ))); + } + } + Ok(()) + } } impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> { @@ -91,15 +124,39 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> { return self.deserialize_number(visitor); } + if get_class_id(&self.value) == ClassId::Number as u32 { + let value_of = get_valueof(&self.value); + if let Some(f) = value_of { + let v = f.call((This(self.value.clone()),))?; + self.value = v; + return self.deserialize_number(visitor); + } + } + if self.value.is_bool() { - let val = self.value.as_bool().unwrap(); - return visitor.visit_bool(val); + return visitor.visit_bool(self.value.as_bool().expect("value to be boolean")); + } + + if get_class_id(&self.value) == ClassId::Bool as u32 { + let value_of = get_valueof(&self.value); + if let Some(f) = value_of { + let v = f.call((This(self.value.clone()),))?; + return visitor.visit_bool(v); + } } if self.value.is_null() || self.value.is_undefined() { return visitor.visit_unit(); } + if get_class_id(&self.value) == ClassId::String as u32 { + let value_of = get_to_string(&self.value); + if let Some(f) = value_of { + let v = f.call(((This(self.value.clone())),))?; + self.value = v; + } + } + if self.value.is_string() { if self.map_key { self.map_key = false; @@ -119,32 +176,79 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> { } if self.value.is_array() { + self.check_cycles() + .map(|_| self.stack.push(self.value.clone()))?; let arr = self.value.as_array().unwrap().clone(); - let length = arr.len(); + // Retrieve the `length` property from the object itself rather than + // using the bindings `Array::len` given that according to the spec + // it's fine to return any value, not just a number from the + // `length` property. + let value: Value = arr.as_object().get(PredefinedAtom::Length)?; + let length: usize = if value.is_number() { + value.as_number().unwrap() as usize + } else { + let value_of: Function = value + .as_object() + .expect("length to be an object") + .get(PredefinedAtom::ValueOf)?; + value_of.call(())? + }; let seq_access = SeqAccess { de: self, length, seq: arr, index: 0, }; - return visitor.visit_seq(seq_access); + let result = visitor.visit_seq(seq_access); + self.stack.pop(); + return result; } if self.value.is_object() { - let filter = Filter::new().enum_only().symbol().string(); + ensure_supported(&self.value).and_then(|_| { + self.check_cycles() + .map(|_| self.stack.push(self.value.clone())) + })?; + + if let Some(f) = get_to_json(&self.value) { + let v: Value = f.call((This(self.value.clone()),))?; + + if v.is_undefined() { + self.value = Value::new_undefined(v.ctx().clone()); + } else { + self.value = v; + } + return self.deserialize_any(visitor); + } + + let filter = Filter::new().enum_only().string(); let obj = self.value.as_object().unwrap(); let properties: ObjectIter<'_, _, Value<'_>> = obj.own_props::, Value<'_>>(filter); + self.stack.push(self.value.clone()); let map_access = MapAccess { de: self, properties, }; - return visitor.visit_map(map_access); + + let result = visitor.visit_map(map_access); + self.stack.pop(); + return result; + } + + if get_class_id(&self.value) == ClassId::BigInt as u32 + || self.value.type_of() == rquickjs::Type::BigInt + { + if let Some(f) = get_to_json(&self.value) { + let v: Value = f.call((This(self.value.clone()),))?; + self.value = v; + return self.deserialize_any(visitor); + } } - Err(Error::Custom(anyhow!( - "Couldn't deserialize value: {:?}", - self.value + Err(Error::from(Exception::throw_type( + self.value.ctx(), + "Unsupported type", ))) } @@ -203,16 +307,40 @@ impl<'a, 'de> de::MapAccess<'de> for MapAccess<'a, 'de> { { loop { if let Some(kv) = self.properties.next() { - let (k, v) = kv.map_err(|e| from_js_error(self.de.value.ctx().clone(), e))?; - - // Entries with non-JSONable values are skipped to respect JSON.stringify's spec - if !is_jsonable(&v) { + let (k, v) = kv?; + + let to_json = get_to_json(&v); + let v = if let Some(f) = to_json { + f.call((This(v.clone()), k.clone()))? + } else { + v + }; + + // Entries with non-JSONable values are skipped to respect + // JSON.stringify's spec + if !ensure_supported(&v)? || k.is_symbol() { continue; } - self.de.value = k.clone(); + let class_id = get_class_id(&v); + + if class_id == ClassId::Bool as u32 || class_id == ClassId::Number as u32 { + let value_of = get_valueof(&v); + if let Some(f) = value_of { + let v = f.call((This(v.clone()),))?; + self.de.current_kv = Some((k.clone(), v)); + } + } else if class_id == ClassId::String as u32 { + let to_string = get_to_string(&v); + if let Some(f) = to_string { + let v = f.call((This(v.clone()),))?; + self.de.current_kv = Some((k.clone(), v)); + } + } else { + self.de.current_kv = Some((k.clone(), v)); + } + self.de.value = k; self.de.map_key = true; - self.de.current_kv = Some((k, v)); return seed.deserialize(&mut *self.de).map(Some); } else { @@ -245,17 +373,17 @@ impl<'a, 'de> de::SeqAccess<'de> for SeqAccess<'a, 'de> { T: de::DeserializeSeed<'de>, { if self.index < self.length { - self.de.value = self - .seq - .get(self.index) - .map(|v| { - if is_jsonable(&v) { - v - } else { - Null.into_value(self.seq.ctx().clone()) - } - }) - .map_err(|e| from_js_error(self.seq.ctx().clone(), e))?; + let el = self.seq.get(self.index)?; + let to_json = get_to_json(&el); + + if let Some(f) = to_json { + let index_value = JSString::from_str(el.ctx().clone(), &self.index.to_string()); + self.de.value = f.call((This(el.clone()), index_value))?; + } else if ensure_supported(&el)? { + self.de.value = el + } else { + self.de.value = Null.into_value(self.seq.ctx().clone()) + } self.index += 1; seed.deserialize(&mut *self.de).map(Some) } else { @@ -264,15 +392,71 @@ impl<'a, 'de> de::SeqAccess<'de> for SeqAccess<'a, 'de> { } } -fn is_jsonable(value: &Value<'_>) -> bool { - !matches!( +/// Checks if the value is an object and contains a single `toJSON` function. +pub(crate) fn get_to_json<'a>(value: &Value<'a>) -> Option> { + let f = unsafe { + JS_GetProperty( + value.ctx().as_raw().as_ptr(), + value.as_raw(), + PredefinedAtom::ToJSON as u32, + ) + }; + let f = unsafe { Value::from_raw(value.ctx().clone(), f) }; + + if f.is_function() { + Some(f.into_function().unwrap()) + } else { + None + } +} + +/// Checks if the value is an object and contains a `valueOf` function. +fn get_valueof<'a>(value: &Value<'a>) -> Option> { + if let Some(o) = value.as_object() { + let value_of = o.get("valueOf").ok(); + value_of.clone() + } else { + None + } +} + +/// Checks if the value is an object and contains a `valueOf` function. +fn get_to_string<'a>(value: &Value<'a>) -> Option> { + if let Some(o) = value.as_object() { + let value_of = o.get("toString").ok(); + value_of.clone() + } else { + None + } +} + +/// Gets the underlying class id of the value. +fn get_class_id(v: &Value) -> u32 { + unsafe { JS_GetClassID(v.as_raw()) } +} + +/// Ensures that the value can be stringified. +fn ensure_supported(value: &Value<'_>) -> Result { + let class_id = get_class_id(value); + if class_id == (ClassId::Bool as u32) || class_id == (ClassId::Number as u32) { + return Ok(true); + } + + if class_id == ClassId::BigInt as u32 { + return Err(Error::from(Exception::throw_type( + value.ctx(), + "BigInt not supported", + ))); + } + + Ok(!matches!( value.type_of(), rquickjs::Type::Undefined | rquickjs::Type::Symbol | rquickjs::Type::Function | rquickjs::Type::Uninitialized | rquickjs::Type::Constructor - ) + )) } #[cfg(test)] diff --git a/crates/javy/src/serde/err.rs b/crates/javy/src/serde/err.rs index 622ca6e1..665ccec6 100644 --- a/crates/javy/src/serde/err.rs +++ b/crates/javy/src/serde/err.rs @@ -1,5 +1,6 @@ use std::{error, fmt}; pub type Result = std::result::Result; +use crate::quickjs::Error as JSError; #[derive(Debug)] pub enum Error { @@ -11,7 +12,13 @@ impl error::Error for Error {} impl fmt::Display for Error { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { match self { - Error::Custom(e) => formatter.write_str(&e.to_string()), + Error::Custom(e) => { + if let Some(e) = e.downcast_ref::() { + formatter.write_str(&format!("JSError: {}", &e.to_string())) + } else { + formatter.write_str(&e.to_string()) + } + } } } } @@ -21,3 +28,9 @@ impl From for Error { Error::Custom(e) } } + +impl From for Error { + fn from(value: JSError) -> Self { + Error::Custom(anyhow::Error::new(value)) + } +} diff --git a/crates/javy/src/serde/ser.rs b/crates/javy/src/serde/ser.rs index 906b7db3..ba78cece 100644 --- a/crates/javy/src/serde/ser.rs +++ b/crates/javy/src/serde/ser.rs @@ -1,12 +1,12 @@ -use crate::from_js_error; -use crate::quickjs::{Array, Ctx, Error as JSError, Object, String as JSString, Value}; +use crate::quickjs::{ + qjs::{JS_DefinePropertyValue, JS_ValueToAtom, JS_PROP_C_W_E}, + Array, Ctx, Object, String as JSString, Value, +}; use crate::serde::err::{Error, Result}; use anyhow::anyhow; use serde::{ser, ser::Error as SerError, Serialize}; -use super::as_key; - /// `Serializer` is a serializer for [Value] values, implementing the `serde::Serializer` trait. /// /// This struct is responsible for converting Rust types into [Value] using the Serde @@ -109,8 +109,7 @@ impl<'a> ser::Serializer for &'a mut Serializer<'_> { } fn serialize_str(self, v: &str) -> Result<()> { - let js_string = JSString::from_str(self.context.clone(), v) - .map_err(|e: JSError| Error::custom(e.to_string()))?; + let js_string = JSString::from_str(self.context.clone(), v).map_err(Error::custom)?; self.value = Value::from(js_string); Ok(()) } @@ -152,8 +151,8 @@ impl<'a> ser::Serializer for &'a mut Serializer<'_> { } fn serialize_seq(self, _len: Option) -> Result { - let arr = Array::new(self.context.clone()).map_err(|e| Error::custom(e.to_string()))?; - self.value = Value::from(arr); + let arr = Array::new(self.context.clone()).map_err(Error::custom)?; + self.value = arr.into_value(); Ok(self) } @@ -170,7 +169,7 @@ impl<'a> ser::Serializer for &'a mut Serializer<'_> { } fn serialize_map(self, _len: Option) -> Result { - let obj = Object::new(self.context.clone()).map_err(|e| Error::custom(e.to_string()))?; + let obj = Object::new(self.context.clone()).map_err(Error::custom)?; self.value = Value::from(obj); Ok(self) } @@ -209,17 +208,17 @@ impl<'a> ser::Serializer for &'a mut Serializer<'_> { where T: ?Sized + Serialize, { - let obj = Object::new(self.context.clone()).map_err(|e| Error::custom(anyhow!("{e}")))?; + let obj = Object::new(self.context.clone()).map_err(Error::custom)?; value.serialize(&mut *self)?; obj.set(variant, self.value.clone()) - .map_err(|e| Error::custom(e.to_string()))?; + .map_err(Error::custom)?; self.value = Value::from(obj); Ok(()) } fn serialize_bytes(self, _: &[u8]) -> Result<()> { - Err(Error::custom(anyhow!("Cannot serialize bytes"))) + Err(Error::custom("Cannot serialize bytes")) } } @@ -237,10 +236,7 @@ impl<'a> ser::SerializeSeq for &'a mut Serializer<'_> { if let Some(v) = self.value.as_array() { return v .set(v.len(), element_serializer.value.clone()) - .map_err(|e| { - let e = from_js_error(element_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an array")) } @@ -264,10 +260,7 @@ impl<'a> ser::SerializeTuple for &'a mut Serializer<'_> { if let Some(v) = self.value.as_array() { return v .set(v.len(), element_serializer.value.clone()) - .map_err(|e| { - let e = from_js_error(element_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an array")) @@ -289,10 +282,9 @@ impl<'a> ser::SerializeTupleStruct for &'a mut Serializer<'_> { let mut field_serializer = Serializer::from_context(self.context.clone())?; value.serialize(&mut field_serializer)?; if let Some(v) = self.value.as_array() { - return v.set(v.len(), field_serializer.value.clone()).map_err(|e| { - let e = from_js_error(field_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + return v + .set(v.len(), field_serializer.value.clone()) + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an array")) @@ -315,10 +307,9 @@ impl<'a> ser::SerializeTupleVariant for &'a mut Serializer<'_> { value.serialize(&mut field_serializer)?; if let Some(v) = self.value.as_array() { - return v.set(v.len(), field_serializer.value.clone()).map_err(|e| { - let e = from_js_error(field_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + return v + .set(v.len(), field_serializer.value.clone()) + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an array")) @@ -349,13 +340,26 @@ impl<'a> ser::SerializeMap for &'a mut Serializer<'_> { { let mut map_serializer = Serializer::from_context(self.context.clone())?; value.serialize(&mut map_serializer)?; - let key = as_key(&self.key)?; + let atom = unsafe { JS_ValueToAtom(self.context.as_raw().as_ptr(), self.key.as_raw()) }; if let Some(o) = self.value.as_object() { - return o.set(key, map_serializer.value.clone()).map_err(|e| { - let e = from_js_error(map_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + // Use `JS_DefinePropertyValue` to keep the semantics of the object + // unchanged. + let result = unsafe { + JS_DefinePropertyValue( + self.context.as_raw().as_ptr(), + o.as_raw(), + atom, + map_serializer.value.as_raw(), + JS_PROP_C_W_E as i32, + ) + }; + + return if result != 0 { + Ok(()) + } else { + Err(Error::custom("Error while serializing object")) + }; } Err(Error::custom("Expected to be an object")) @@ -378,10 +382,9 @@ impl<'a> ser::SerializeStruct for &'a mut Serializer<'_> { value.serialize(&mut field_serializer)?; if let Some(o) = self.value.as_object() { - return o.set(key, field_serializer.value.clone()).map_err(|e| { - let e = from_js_error(field_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + return o + .set(key, field_serializer.value.clone()) + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an object")) @@ -404,10 +407,9 @@ impl<'a> ser::SerializeStructVariant for &'a mut Serializer<'_> { value.serialize(&mut field_serializer)?; if let Some(o) = self.value.as_object() { - return o.set(key, field_serializer.value.clone()).map_err(|e| { - let e = from_js_error(field_serializer.value.ctx().clone(), e); - Error::custom(e.to_string()) - }); + return o + .set(key, field_serializer.value.clone()) + .map_err(|e| Error::custom(e.to_string())); } Err(Error::custom("Expected to be an object")) @@ -629,24 +631,6 @@ mod tests { Ok(()) } - #[test] - fn test_map_with_invalid_key_type() { - // This is technically possible since msgpack supports maps - // with any other valid msgpack type. However, we try to enforce - // using `K: String` since it allow transcoding from json<->msgpack. - let rt = Runtime::default(); - rt.context().with(|cx| { - let mut serializer = ValueSerializer::from_context(cx.clone()).unwrap(); - - let mut map = BTreeMap::new(); - map.insert(42, "bar"); - map.insert(43, "titi"); - - let err = map.serialize(&mut serializer).unwrap_err(); - assert_eq!("map keys must be a string".to_string(), err.to_string()); - }); - } - #[test] fn test_map() { let rt = Runtime::default(); diff --git a/crates/javy/test262 b/crates/javy/test262 new file mode 160000 index 00000000..24965772 --- /dev/null +++ b/crates/javy/test262 @@ -0,0 +1 @@ +Subproject commit 249657722525cc8e43b1ddb91f8df0b4b011fcf6 diff --git a/crates/javy/tests/262.rs b/crates/javy/tests/262.rs new file mode 100644 index 00000000..f09e462f --- /dev/null +++ b/crates/javy/tests/262.rs @@ -0,0 +1,2 @@ +#[cfg(feature = "json")] +javy_test_macros::t262!(path = "crates/javy/test262"); diff --git a/supply-chain/config.toml b/supply-chain/config.toml index c6e219fd..ed2888aa 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -134,10 +134,6 @@ criteria = "safe-to-deploy" version = "0.1.8" criteria = "safe-to-deploy" -[[exemptions.cast]] -version = "0.3.0" -criteria = "safe-to-run" - [[exemptions.cc]] version = "1.0.98" criteria = "safe-to-deploy" diff --git a/supply-chain/imports.lock b/supply-chain/imports.lock index fd015d1a..2f623a40 100644 --- a/supply-chain/imports.lock +++ b/supply-chain/imports.lock @@ -377,8 +377,8 @@ user-login = "dtolnay" user-name = "David Tolnay" [[publisher.proc-macro2]] -version = "1.0.84" -when = "2024-05-25" +version = "1.0.85" +when = "2024-06-02" user-id = 3618 user-login = "dtolnay" user-name = "David Tolnay" @@ -1868,6 +1868,12 @@ criteria = "safe-to-deploy" delta = "2.4.2 -> 2.5.0" aggregated-from = "https://chromium.googlesource.com/chromium/src/+/main/third_party/rust/chromium_crates_io/supply-chain/audits.toml?format=TEXT" +[[audits.google.audits.cast]] +who = "George Burgess IV " +criteria = "safe-to-run" +version = "0.3.0" +aggregated-from = "https://chromium.googlesource.com/chromiumos/third_party/rust_crates/+/refs/heads/main/cargo-vet/audits.toml?format=TEXT" + [[audits.google.audits.clap]] who = "danakj@chromium.org" criteria = "safe-to-run"