Better output from `impl Debug for JsValue`. #1161

derekdreery · 2019-01-09T17:13:13Z

We can use JSON.stringify to get a better debug representation of a value. It's slow, but I think there's an acceptance that Debug (unlike maybe Display) is allowed to be slow to write better strings, given that it's used mostly before deployment.

There's also a new method for testing whether something is an array in javascript. I can remove this if you don't think it's useful.

derekdreery · 2019-01-09T17:20:19Z

This should have some tests I think (it doesn't yet).

alexcrichton · 2019-01-10T19:31:08Z

Thanks for this!

Reading this over, I wonder if we perhaps want to actually just move the entire Debug implementation into JS? The code size footprint in Rust is actually pretty large we've seen in the past with float parsing and whatnot, and if we're hopping back and forth to JS to test types and whatnot it may actually just be best to have stringification happen directly in JS.

derekdreery · 2019-01-10T22:43:06Z

Good idea! I'll experiment.

derekdreery · 2019-01-10T23:40:36Z

I've done an implementation in JS. See what you think.

There's still a TODO around no-std. This patch actually regresses to doing nothing. Can we do something to get the string in the case that we don't have an allocator?

fitzgen

I like this approach!

fitzgen · 2019-01-10T23:55:03Z

src/lib.rs

@@ -298,11 +298,32 @@ impl JsValue {
        unsafe { __wbindgen_is_object(self.idx) == 1 }
    }

+    /// Tests whether `Array.isArray(self)`.
+    #[inline]
+    pub fn is_array(&self) -> bool {


This seems to be unused? If so, then we should remove it.

Yeah if it were to be included it should be a separate PR. I'll remove.

fitzgen · 2019-01-11T01:19:10Z

crates/cli-support/src/js/mod.rs

+            Ok(String::from(
+                "
+                function(i, len_ptr) {
+                    const debug_str = val => {


I tried to come up with something that supported as rich of debug string output as this but was a little more terse. Not sure I really ended up on an improvement over what you have here, but for posterity:

function debug(val) { switch(typeof val) { case "undefined": case "number": case "boolean": case "symbol": return String(val); case "function": return `Function(${val.name || ""})`; case "object": if (val === null) { return "null"; } const s = Object.prototype.toString.call(val); if (s === "[object Object]" || s === "[object Array]") { try { return JSON.stringify(val); } catch (_) {} } return s; default: return Object.prototype.toString.call(val); } }

This is cool. I did think about doing a big switch. If JSON.stringify can throw, I should catch it and just fall through like you do. If we can capture the function name that would be cool also. I'll try to merge the 2 impls.

Also since I'm assuming there's no stability guarantees on the output here we can always refine later.

get name of the function if possible

if JSON.stringify can throw, catch and ignore any exceptions.

get toString from Object.prototype

If JSON.stringify can throw

This is also why I did Object.prototype.toString.call(val) rather than val.toString() or String(val) etc.

fitzgen · 2019-01-11T01:23:16Z

There's still a TODO around no-std. This patch actually regresses to doing nothing. Can we do something to get the string in the case that we don't have an allocator?

I don't think there is an easy way, unfortunately.

derekdreery · 2019-01-11T11:20:42Z

I don't think there is an easy way, unfortunately.

I think I probably need to restore the old impl then for the no_std case.

alexcrichton · 2019-01-11T17:41:39Z

I think it's fine to in general not worry much about the no_std use case, I don't think that it's really operational at all and likely won't make its way to the next major release of wasm-bindgen. (I personally think I was mistaken to add it originally!)

fitzgen · 2019-01-11T18:24:51Z

I think it's fine to in general not worry much about the no_std use case, I don't think that it's really operational at all and likely won't make its way to the next major release of wasm-bindgen. (I personally think I was mistaken to add it originally!)

Agreed, FWIW.

derekdreery · 2019-01-11T19:25:17Z

I've added suggestings from @fitzgen, and also rebased.

derekdreery · 2019-01-12T10:47:53Z

I think this is ready for review now. I've tried to cover all the cases in the tests. I found quite a few problems, which I guess is why tests!!!!

derekdreery · 2019-01-12T14:37:15Z

Not sure where the failure's from.

fitzgen · 2019-01-14T17:20:53Z

https://travis-ci.com/rustwasm/wasm-bindgen/jobs/169969977#L894-L906

---- wasm::api::debug_output output ----
    JS exception that was thrown:
        TypeError: getObject(...).values is not a function
            at module.exports.__wbg_values_c7388d2dffc70125 (/home/travis/build/rustwasm/wasm-bindgen/target/wasm32-unknown-unknown/wbg-tmp/wasm-bindgen-test.js:4348:42)
            at js_sys::Array::values::h413e65c06a2161cc (wasm-function[8094]:101)
            at wasm::api::debug_output::h53891ce08362a31e (wasm-function[2663]:96)
            at core::ops::function::FnOnce::call_once::h155aa4ffc67eec53 (wasm-function[1035]:22)
            at wasm_bindgen_test::__rt::Context::execute_sync::_$u7b$$u7b$closure$u7d$$u7d$::h3886c0814e7f4942 (wasm-function[3976]:22)
            at _$LT$futures..future..lazy..Lazy$LT$F$C$$u20$R$GT$$GT$::get::hc1c1b592d7026e96 (wasm-function[654]:395)
            at _$LT$futures..future..lazy..Lazy$LT$F$C$$u20$R$GT$$u20$as$u20$futures..future..Future$GT$::poll::hc9d74cbdf88e301e (wasm-function[859]:38)
            at _$LT$wasm_bindgen_test..__rt..TestFuture$LT$F$GT$$u20$as$u20$futures..future..Future$GT$::poll::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h89214d033d0eec01 (wasm-function[4309]:71)
            at wasm_bindgen::convert::closures::invoke0_mut::h34f83a9aa7fb5334 (wasm-function[7341]:166)
            at Function.cbarg0 (/home/travis/build/rustwasm/wasm-bindgen/target/wasm32-unknown-unknown/wbg-tmp/wasm-bindgen-test.js:4275:25)

I believe that node doesn't support Array.prototype.values yet, so maybe that is what is going on here.

And fix loads of bugs.

fitzgen

LGTM, just a couple nitpicks below about the __wbg_is_array stuff that still hasn't gotten removed.

fitzgen · 2019-01-17T20:34:26Z

crates/cli-support/src/js/mod.rs

@@ -299,6 +299,18 @@ impl<'a> Context<'a> {
            ))
        })?;

+        self.bind("__wbindgen_is_array", &|me| {


Nitpick: remove this

fitzgen · 2019-01-17T20:34:36Z

src/lib.rs

@@ -474,9 +489,11 @@ externs! {
    fn __wbindgen_symbol_new(ptr: *const u8, len: usize) -> u32;
    fn __wbindgen_is_symbol(idx: u32) -> u32;
    fn __wbindgen_is_object(idx: u32) -> u32;
+    fn __wbindgen_is_array(idx: u32) -> u32;


Nitpick: remove this

yep sorry missed this last time.

derekdreery force-pushed the debug_output branch from c29b7cd to 8e732f2 Compare January 9, 2019 17:31

derekdreery mentioned this pull request Jan 10, 2019

Custom Debug implementations for js_sys::* #1157

Closed

derekdreery force-pushed the debug_output branch from 54fb81c to 7d5d443 Compare January 10, 2019 23:37

fitzgen reviewed Jan 11, 2019

View reviewed changes

derekdreery force-pushed the debug_output branch from d19c580 to f50e715 Compare January 11, 2019 19:23

derekdreery added 10 commits January 16, 2019 10:16

Use JSON serialization to get better debug printouts.

34bab51

Use toString where JSON.stringify didn't work

5b51d27

Redo as js function + fix tests

126efd5

Make clear it's a JsValue.

36cb504

Fix tests again.

7276575

Implement suggestings from @fitzgen.

b60d82a

Resolve what to do with no_std.

fab9d1d

Remove Array.isArray test.

f3dd386

Add a proper test.

5bffc28

And fix loads of bugs.

Add better support for errors.

5f2ba3f

derekdreery force-pushed the debug_output branch from 02f28c2 to 5f2ba3f Compare January 16, 2019 10:46

derekdreery changed the title ~~Use JSON serialization to get better debug printouts.~~ Better output from impl Debug for JsValue. Jan 16, 2019

Better recognition of error type.

b0906fd

Update node to support Array.values in tests.

7fe7606

alexcrichton assigned fitzgen Jan 17, 2019

fitzgen approved these changes Jan 17, 2019

View reviewed changes

derekdreery added 2 commits January 17, 2019 21:07

remove array check

867b13b

remove is array function

f8680a0

fitzgen approved these changes Jan 17, 2019

View reviewed changes

alexcrichton merged commit ba732a8 into rustwasm:master Jan 18, 2019

derekdreery deleted the debug_output branch January 18, 2019 11:55

fitzgen added the TWiRaWA Nominate this PR for inclusion in the next issue of This Week in Rust and WebAssembly label Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better output from `impl Debug for JsValue`. #1161

Better output from `impl Debug for JsValue`. #1161

derekdreery commented Jan 9, 2019

derekdreery commented Jan 9, 2019

alexcrichton commented Jan 10, 2019

derekdreery commented Jan 10, 2019

derekdreery commented Jan 10, 2019

fitzgen left a comment

fitzgen Jan 10, 2019

derekdreery Jan 11, 2019

fitzgen Jan 11, 2019

derekdreery Jan 11, 2019 •

edited

Loading

fitzgen Jan 11, 2019

fitzgen commented Jan 11, 2019

derekdreery commented Jan 11, 2019

alexcrichton commented Jan 11, 2019

fitzgen commented Jan 11, 2019

derekdreery commented Jan 11, 2019

derekdreery commented Jan 12, 2019

derekdreery commented Jan 12, 2019

fitzgen commented Jan 14, 2019

fitzgen left a comment

fitzgen Jan 17, 2019

fitzgen Jan 17, 2019

derekdreery Jan 17, 2019

Better output from impl Debug for JsValue. #1161

Better output from impl Debug for JsValue. #1161

Conversation

derekdreery commented Jan 9, 2019

derekdreery commented Jan 9, 2019

alexcrichton commented Jan 10, 2019

derekdreery commented Jan 10, 2019

derekdreery commented Jan 10, 2019

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Jan 10, 2019

Choose a reason for hiding this comment

derekdreery Jan 11, 2019

Choose a reason for hiding this comment

fitzgen Jan 11, 2019

Choose a reason for hiding this comment

derekdreery Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

fitzgen Jan 11, 2019

Choose a reason for hiding this comment

fitzgen commented Jan 11, 2019

derekdreery commented Jan 11, 2019

alexcrichton commented Jan 11, 2019

fitzgen commented Jan 11, 2019

derekdreery commented Jan 11, 2019

derekdreery commented Jan 12, 2019

derekdreery commented Jan 12, 2019

fitzgen commented Jan 14, 2019

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Jan 17, 2019

Choose a reason for hiding this comment

fitzgen Jan 17, 2019

Choose a reason for hiding this comment

derekdreery Jan 17, 2019

Choose a reason for hiding this comment

Better output from `impl Debug for JsValue`. #1161

Better output from `impl Debug for JsValue`. #1161

derekdreery Jan 11, 2019 •

edited

Loading