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

proposal: syscall/js: stricter conversion methods #29642

Closed
myitcv opened this issue Jan 9, 2019 · 23 comments
Closed

proposal: syscall/js: stricter conversion methods #29642

myitcv opened this issue Jan 9, 2019 · 23 comments
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Jan 9, 2019

This proposal is based on work originally done for GopherJS in gopherjs/gopherjs#617.

Problem

Currently, any syscall.js.Value can have String() called on it, and that is guaranteed to succeed, because JavaScript's String(thing) function can take anything as an argument (this is where it is used). For example:

func main() {
	var v js.Value

	v = eval(`global.X = undefined`)
	fmt.Printf("global.X: %v\n", v.String())

	v = eval(`global.X = null`)
	fmt.Printf("global.X: %v\n", v.String())

	v = eval(`global.X = 5`)
	fmt.Printf("global.X: %v\n", v.String())

	v = eval(`global.X = new Number(5)`)
	fmt.Printf("global.X: %v\n", v.String())

	v = eval(`global.X = [1, 2, 3]`)
	fmt.Printf("global.X: %v\n", v.String())
}

func eval(s string, args ...interface{}) js.Value {
	v := fmt.Sprintf(s, args...)
	fmt.Printf("> %v\n", v)
	return js.Global().Call("eval", v)
}

gives:

> global.X = undefined
global.X: undefined
> global.X = null
global.X: null
> global.X = 5
global.X: 5
> global.X = new Number(5)
global.X: 5
> global.X = [1, 2, 3]
global.X: 1,2,3

By contrast, the Int(), Float(), and Bool() methods are much stricter. They require a js.Value with a JavaScript type of number, number and boolean respectively. Consider the following example for Int():

func main() {
	var v js.Value

	v = eval(`global.X = 5`)
	fmt.Printf("global.X: %v\n", v.Int())

	v = eval(`global.X = new Number(5)`)
	fmt.Printf("global.X:%v\n", v.Int())
}

gives:

> global.X = 5
global.X: 5
> global.X = new Number(5)
panic: syscall/js: call of Value.Int on object
...

I believe that the strictness of Int(), Float() and Bool() methods is correct; with this strictness, I can make strong assertions about the fact that only a number, number or boolean can have been the source value. I discuss below whether panic-ing is the correct approach when the source type is incorrect.

By contrast, I believe the current implementation of String() will cause issues. Precisely because I can't make any assertions about the source value: I can't opt-out of this automatic conversion that happens along the way. Instead everywhere I call String() I need to defensively code to ensure that I actually have a string value on the JavaScript side before treating it as such on the Go side:

func main() {
	var v js.Value

	v = eval(`global.X = "hello"`)
	if v.Type() == js.TypeString {
		fmt.Printf("global.X: %v\n", v.String())
	} else {
		fmt.Printf("global.X: not a string\n")
	}

	v = eval(`global.X = 5`)
	if v.Type() == js.TypeString {
		fmt.Printf("global.X: %v\n", v.String())
	} else {
		fmt.Printf("global.X: not a string\n")
	}
}

gives:

> global.X = "hello"
global.X: hello
> global.X = 5
global.X: not a string

However, the current implementation of String() is useful in one important respect (thanks to @neelance for reminding me of this): fmt. It allows a js.Value value to implement fmt.Stringer. Having the JavaScript String() representation of a value makes sense to me.

That said, we can't just change String() to be as strict as Int() et al.

Potential solution

My proposal is that we leave String() as is for fmt reasons, remove Int(), Float() and Bool(), and choose one of the following options (there could be others):

Option 1: define AsX() methods that have the same strict semantics as Int() et al currently do:

func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

That is, unless a method saw the a boolean, number, number or string (respectively) JavaScript typed value, it would panic.

Option 2: same as option 1, but instead of panic-ing, return an error:

func (v Value) AsBool() (bool, error)
func (v Value) AsFloat() (float64, error)
func (v Value) AsInt() (int, error)
func (v Value) AsString() (string, error)

Option 3: switch to more of an encoding parse style:

func (v Value) AsBool(*bool) error
func (v Value) AsFloat(*float) error
func (v Value) AsInt(*int) error
func (v Value) AsString(*string) error

This would also have the added benefit of being slightly more future proof in case we decided to relax the very strict semantics of requiring a boolean, number, number or string, and instead allowed undefined and null to be interpreted as the zero value (encoding/json behaves in this way).

Notes

  • The As method name prefix above is of course not set in stone
  • I think the syscall/js docs should also spell out any conversions that happen along the way too. For example, when converting a string value JavaScript <-> Go we end up performing a UTF16 <-> UTF8 conversion. This point is not tied to this proposal however, I just mention it in passing given the main focus of the proposal/issue is conversion JavaScript <-> Go

cc @neelance

@neelance
Copy link
Member

What about only adding MustString() with the strict semantics? It is a bit inconsistent, but I'm not sure that it is worth to clutter the other function names just because of this. It would also be good to stay similar to the reflect package.

@bradfitz I'd like your opinion on this if you don't mind.

@myitcv
Copy link
Member Author

myitcv commented Jan 10, 2019

Thanks @neelance.

It would also be good to stay similar to the reflect package.

The encoding/json analogy seems more appropriate to my mind (hence option 3 above), not least because there is some conversion required from the JavaScript value.

What about only adding MustString() with the strict semantics?

Yes, I think an inconsistency of this sort in a brand new API would be a shame.

In a Slack discussion we also came up with To as a prefix. So, taking option 1:

func (v Value) ToBool() bool
func (v Value) ToFloat() float64
func (v Value) ToInt() int
func (v Value) ToString() string

@justinclift
Copy link

justinclift commented Jan 11, 2019

Avoiding the panic sounds important to me, just from the perspective of being able to catch when a problem occurs - using standard "if err != nil" approach - for handling.

I'd steer clear of option 1. 😄

@justinclift
Copy link

Name wise.. the To* versions sound better than the As* versions (to me).

@Yaoir
Copy link

Yaoir commented Jan 11, 2019

Out of the three options, I definitely prefer option 2. It's a little more complicated than option 1, but avoids panics. Also, compare the situation to converting strings to other types in Go. The strconv package Parse_Type_() functions like ParseInt() and ParseBool() accept a string and return an error if the string couldn't be converted to that type.

@flimzy
Copy link
Contributor

flimzy commented Jan 13, 2019

I definitely prefer option 2, or a slight variation, 2b, if the only type of error is a type mismatch:

func (v Value) AsBool() (bool, bool)
func (v Value) AsFloat() (float64, bool)
func (v Value) AsInt() (int, bool)
func (v Value) AsString() (string, bool)

I'm now in favor of the panic method, as described here.

@johanbrandhorst
Copy link
Member

Re: the bikeshed; Rust has chosen as_x for these methods:

https://docs.rs/wasm-bindgen/0.2.11/wasm_bindgen/struct.JsValue.html

@neelance
Copy link
Member

I think we should not return an error value (or ok value), since it is functionally equivalent to a simple check of the Type() and it puts a burden on each and every use.

@myitcv
Copy link
Member Author

myitcv commented Jan 14, 2019

@neelance on the question of returning an error/bool vs panic, I'm not sure it's that clear cut.

A number of JavaScript APIs allow fields of effectively union type. e.g. string | []string. For such fields you'd have to implement error handling via a defer, which works but isn't very pleasant.

To my mind, if we have to make a decision one way or the other here it should be somewhat guided by how often such a pattern would need to be handled in practice. I don't have any gut feel/numbers on that.

Alternatively, don't force a decision between the two, instead add additional API methods for the error/bool handling, effectively a hybrid of options 1 and 2.

// from option 1
func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

// from option 2
func (v Value) AsBoolErr() (bool, error)
func (v Value) AsFloatErr() (float64, error)
func (v Value) AsIntErr() (int, error)
func (v Value) AsStringErr() (string, error)

@neelance
Copy link
Member

A number of JavaScript APIs allow fields of effectively union type. e.g. string | []string. For such fields you'd have to implement error handling via a defer, which works but isn't very pleasant.

I don't understand this, please post example code.

@myitcv
Copy link
Member Author

myitcv commented Jan 14, 2019

Actually, I'm wrong here. Because you'd almost certainly use Type() in such situations.

So I agree, the panic approach should suffice.

@myitcv
Copy link
Member Author

myitcv commented Jan 20, 2019

Think we're waiting on @bradfitz to opine on this... Anyone else?

Would it be possible to get this in for 1.12, given that WASM is still experimental?

@neelance
Copy link
Member

I think it is too late in the cycle to get this into 1.12. Also, we don't seem to have a consensus yet.

@andybons
Copy link
Member

Definitely too late for 1.12. Sorry.

@martin-juhlin
Copy link

Adding MustString() or do nothing at all is the best solution.

Using To*/As* indicate that a conversion of some sort is taking place. If the type is a float with value 3.1415, and I invoking ToInt(), I expect it to return 3, not panic.

We are actually getting current value. Get is a better prefix as in GetBool(), GetInt() etc, but in Go we don't prefix getters with Get, therefor the methods should be namned Bool(), Int() etc

option 2 and 3 is also hard to use. Example a = b + c, in current form it can be written as a = b.Int()+ c.Int(). For option 2 and 3 that result in multiple lines.

@myitcv
Copy link
Member Author

myitcv commented Feb 5, 2019

@martin-juhlin

Using To*/As* indicate that a conversion of some sort is taking place.

syscall/js is the interface between JavaScript values and Go values. For string values, for example, a conversion happens to/from UTF8/UTF16. To my mind this package should be a type-safe interface that is limited to/concerned with conversion between these two "worlds". Things like float to integer conversion should happen elsewhere and explicitly so, either in the JavaScript world before they get passed through syscall/js, or in the Go world after the value has passed through.

The problem with MustString() is that it creates an inconsistent API. If I am writing an interface to a JavaScript library/some such, then I end up using the following methods:

// option "Richard"
func (v Value) Bool() bool
func (v Value) Float() float64
func (v Value) Int() int
func (v Value) MustString() string

despite there being a String() method. It just feels like having to say to people "use Int(), Bool() and Float(), but remember to use MustString() for string values" is an unnecessary caveat and a somewhat inevitable pitfall.

Instead, having a consistent API is much clearer and less susceptible to user error:

// option 1
func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

@neelance
Copy link
Member

neelance commented Mar 3, 2019

I would like to propose an alternative, inspired by https://golang.org/pkg/reflect/#Value.String: For non-string values instead of using JS's type conversion we use the following string representations:

<undefined>
<null>
<boolean: true>
<number: 42>
<symbol: abc>
<object>
<function>

This has the following properties:

  • No panic, thus compatible with fmt.Stringer
  • No JS conversion semantics in the Go API
  • No unexpected type conversion of the number 42 to the string "42" which may hide a bug
  • No cluttered names
  • No inconsistency with the reflect package

The downside of the inconsistency between String() and Int() etc. remains, but this tradeoff already has its precedence in the reflect package.

If one is processing external data and strict type checking is required, then explicit checking of the Type() is necessary anyways, since it is bad Go style to rely on recovering a panic for catching bad external input.

@myitcv
Copy link
Member Author

myitcv commented Mar 3, 2019

@neelance and I discussed this further offline.

I think the conclusion we reached is that what I'm after is better suited to a package/approach that sits atop syscall/js. I'm going to experiment with my React wrapper and report back.

@bradfitz
Copy link
Contributor

Do you have a report, @myitcv?

@myitcv
Copy link
Member Author

myitcv commented Mar 21, 2019

Haven't had a chance yet @bradfitz. So I'd suggest we either put this on hold or close it. We can always resurrect the issue later if needs be

@andybons
Copy link
Member

Closing per discussion with @golang/proposal-review

@neelance
Copy link
Member

@andybons What about my proposal to change the results of String() for non-string values? It still seems like a strict improvement to me and I haven't yet seen any arguments against it. I was planning to send a CL in the next few days.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169757 mentions this issue: syscall/js: improve Value.String() for non-string values

gopherbot pushed a commit that referenced this issue Mar 28, 2019
This change modifies Value.String() to use the following
representations for non-string values:
  <undefined>
  <null>
  <boolean: true>
  <number: 42>
  <symbol>
  <object>
  <function>

It avoids JavaScript conversion semantics in the Go API and lowers the
risk of hidden bugs by unexpected conversions, e.g. the conversion
of the number 42 to the string "42". See discussion in #29642.

This is a breaking change, which are still allowed for syscall/js.
The impact should be small since it only affects uses of
Value.String() with non-string values, which should be uncommon.

Updates #29642.

Change-Id: I2c27be6e24befe8cb713031fbf66f7b6041e7148
Reviewed-on: https://go-review.googlesource.com/c/go/+/169757
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants