Thanks for reporting this and providing concrete details, @myitcv.

I'll share some quick thoughts that come to me immediately after reading this, but I'll need more time to think about it further and perhaps I'll change my mind.

To my mind this is a bug because the compiler generated code is failing to properly internalise an external value which we have defined to be of type string.

In my mind, it's not so clear whether it's a bug or not. It may, or may not be. It really depends on the following. Given this struct:

type S struct {
	*js.Object

	A string `js:"A"`
}

I know that A string `js:"A"` is really just a shortcut to accessing the JavaScript object's A property.

And when you do:

o := js.Global.Get("Object").New()
s := S{Object: o}

Which is translated to something like var s = new Object(); in JavaScript.

It's very expected to me that s.A (JavaScript syntax) will be undefined.

The question is whether GopherJS should do a lightweight mapping/rewriting that is JavaScript-like, which it seems it currently does, or if it should do something more/different that is more Go-like (as you're expecting).

I think @neelance's opinion on this is important, since it's a question of scope for this feature.

Also, we should try to improve documentation of the feature so it's possible to determine correct behavior from reading the spec.

(By the way, there's a good chance I overlooked something in your comment and repeated something you've already covered. Sorry if so, just let me know if that's the case.)

@shurcooL I've just pushed up a PR that formalises roughly what I'm proposing. Namely to tighten up the existing internalize function.

The PR is intended, for now at least, to serve as a starting point for discussion.

The question is whether GopherJS should do a lightweight mapping/rewriting that is JavaScript-like, which it seems it currently does, or if it should do something more/different that is more Go-like (as you're expecting).

Possibly another way to look at this is: There simply is no way to map all JS values to Go values. undefined in JS has the following properties (non-exhaustive list):

• It is equal (==) to undefined
• It stringifies to "undefined"

Go has no equivalent data type. So at most one of these behaviors can be copied. At present, the latter. I suspect that's somewhat by "accident" (by virtue of the fact that for all non-undefined values, stringification makes perfect sense)

But I think a strong case can be made that the former makes more sense. The closest Go offers to undefined (apart from nil, which doesn't apply here), is the zero value.

For a little comparison, I amended @myitcv 's playground here with int, float64 and bool types. I'm not sure if it really helps, but:

• An undefined JS value converted to int is treated as 0 in GopherJS.
• An undefined JS value converted to float64 is treated as NaN in GopherJS.
• An undefined JS value converted to bool is treated as false in GopherJS.

The int and bool cases makes sense to me, and I think is an argument in favor of treating an uninitialized string as its zero value.

The float64 case gives me pause, though. While NaN makes some kind of sense to me, it's not the zero value of a float64, which I probably would have expected. (And if we change string behavior to reflect the zero value, I'd expect us to do the same change for float64, and probably any other data types that don't correspond to their GopherJS zero values).

So I guess TL;DR; I'm in favor of treating all undfined JS values as their Go zero values. But I think this is more a question of the spec (and thus, @neelance 's territory), than a clear cut-and-dried bug to be fixed.

There simply is no way to map all JS values to Go values.
Go has no equivalent data type [to JavaScript's undefined].

@flimzy, what about js.Undefined?

What I meant was that Go has no datatype that can be either a valid string, or undefined.

Of course a *js. Object can do that, but a string cannot.

Mirroring Go's zero values sounds appropriate to me. Honestly even after ~7 years of writing JavaScript full time undefined is pretty much useless, I've always == null to check for null / undefined.

I haven't been using Gopher enough to know if there's any down side to that approach but it SGTM. I can see introducing a lot of bugs if you didn't know undefined == "undefined" on the Go side.

@shurcooL I've just pushed up a PR that formalises roughly what I'm proposing. Namely to tighten up the existing internalize function.

The PR is intended, for now at least, to serve as a starting point for discussion.

I'm trying to understand what underlying issue you're referring to. Let me ask the following questions.

Do we agree that:

type S struct {
	*js.Object

	A string `js:"A"`
}

s := S{Object: js.Global.Get("Object").New()}

fmt.Println(s.A)

Is completely equivalent to:

type S struct {
	*js.Object

	A string `js:"A"`
}

s := S{Object: js.Global.Get("Object").New()}

fmt.Println(s.Get("A").String())

Basically, js struct field tag is just a shorthand for accessing fields in a JS object, which can be done more directly/explicitly with Object.Get method.

If so, then it looks to me (but I could be wrong, please tell me) that your PR/issue can be simplified to:

got := js.Undefined.String() // "undefined"
want := ""

got != want

Basically, you're suggesting that the undefined JS value should internalize to "" string rather than "undefined" string, and the js field tag syntax is not really relevant here. Is that right?

See this playground.

Basically, js struct field tag is just a shorthand for accessing fields in a JS object, which can be done more directly/explicitly with Object.Get method

Indeed, because they ultimately transpile down to the same Javascript:

$internalize(s.A, $String)

The issue I'm trying to address here is the value conversion semantics between the Javascript and Go worlds; specifically in this issue the direction Javascript -> Go.

It's my understanding that $internalize defines these semantics (and has a counterpart $externalize for the other direction).

The example presented in the description highlights what to my mind is a gap in the spec/implementation that doesn't cover internalising of undefined or null. But it is actually a more generic point about tightening up/making safe this $internalize function for all of Go's types. Consider (playground):

package main

import (
	"fmt"

	"github.com/gopherjs/gopherjs/js"
)

func main() {
	o := js.Global.Get("Object").New()
	o.Set("A", 5)

	str := o.Get("A").String()

	fmt.Printf("%q", str)
	js.Global.Get("console").Call("log", "\"%s\"", str)
}

This code should, to my mind, throw a runtime error (indeed I've just updated my PR to show what I believe the semantics should be in this case)

I'm pretty sure I disagree with that last change/suggestion. That violates the way JS is meant to work, and would put a huge burden on the GopherJS coder to ensure runtime type safety before internalizing data.

It's quite common that a JS lib will return a number or a string (or a stringable object). This change would require the coder to always check the JS data type, then do manual stringification.

IMO its better the way it is now: If you care about the internal JS type, you can explicitly check. But in the common case, you can just internalize normally.

That violates the way JS is meant to work, and would put a huge burden on the GopherJS coder to ensure runtime type safety before internalizing data.

To my mind the whole point of the $internalize function is to provide certainty over the resulting value, precisely because the semantics around stringification and automatic type conversion in Javascript are so loose/hard to follow/reason/etc....

The flip side is that if we don't do this then the fuzzy world of automatic type conversions leaks into the GopherJS world and makes it harder (impossible?) to easily reason about the resulting GopherJS code. If I have a string (Go type) value then I expect it to be a string. Otherwise I have to start reasoning along the lines of "well if it's actually an array value under the covers then strings.Join will do XYZ instead of ABC." And what's worse is you force the GopherJS programmer to then have knowledge of the fact that a given string value came from an external object. It all starts to fall apart a bit...

I would much prefer the burden of having to "shim" some external Javascript code in such a way where I'm forced to be explicit about the conversions than rely on automatic conversions that allow my code to behave in ways.

For example it just feels wrong to me that this should be possible:

https://gopherjs.github.io/playground/#/v4sp4QU3If

I can see and appreciate your view. It's one of the principal reasons I started with GopherJS: I hate the entire JS typing system.

I'm still not sure I am in favor of such changes. But even if I was, this type of change would break a lot of existing code.

Would it be possible to do this type of "strict" internalization in a separate package? It might even be possible to mimic the GopherJS struct tags. I haven't thought much about implementation of such a package yet. It's just a brainstorm for now.

But even if I was, this type of change would break a lot of existing code.

If we were to make this change then this is clearly a point that we would need to be understood more, but I'm slightly more optimistic in thinking it wouldn't actually break as often as we might think (but clearly some numbers would help steer this conversation 👍 )

Furthermore, if we were to make this change but as a developer you wanted the original (i.e. current at the time of writing) behaviour then you could simply:

js.Global.Call("String", o.Get("A")).String()

i.e. make explicit the fact that you want to use Javascript's automatic conversion.

I guess I thought you were proposing that both forms of internalization would change. If you're only suggesting a change to the struct conversion, that bothers me less, but mainly because I personally don't use that much. I've found it to be too wrought with difficulties to be generally useful, so I tend toward the converter methods.

I think (and might be wrong here) that every "form" ultimately transpiles to a call to $internalize... On the basis I'm proposing changing that function, then all "forms" are ultimately impacted. But as I demonstrated above, you can easily revert to the previous behaviour if required/desired.

But as I demonstrated above, you can easily revert to the previous behaviour if required/desired.

Oh right, I see what you mean now. So the same potential for breakage still exists.

The flip side is that if we don't do this then the fuzzy world of automatic type conversions leaks into the GopherJS world and makes it harder (impossible?) to easily reason about the resulting GopherJS code. If I have a string (Go type) value then I expect it to be a string. Otherwise I have to start reasoning along the lines of "well if it's actually an array value under the covers then strings.Join will do XYZ instead of ABC." And what's worse is you force the GopherJS programmer to then have knowledge of the fact that a given string value came from an external object. It all starts to fall apart a bit...

I would much prefer the burden of having to "shim" some external Javascript code in such a way where I'm forced to be explicit about the conversions than rely on automatic conversions that allow my code to behave in ways.

For example it just feels wrong to me that this should be possible:

https://gopherjs.github.io/playground/#/v4sp4QU3If

To clarify, it's not that the string can now be a dynamic javascript invocation. Here, I've modified your example to make that more clear:

https://gopherjs.github.io/playground/#/cj-JaeDcnP

Object.String is documented as:

// String returns the object converted to string according to JavaScript type conversions.

So once you internalize a JavaScript object into a Go variable, it becomes a normal Go string and behaves as such. This doesn't seem that bad to me, since you can always avoid implicitly internalizing things that aren't strings to strings, if you prefer not to do that.

It's not yet clear to me whether making internalization conversion rules stricter would be helpful, but it might be.

I can see it might be more impactful for js struct field tag situation.

I definitely agree that we should tighten up and document the internalization logic for undefined and null values.

// String returns the object converted to string according to JavaScript type conversions.

Thanks @shurcooL, I had forgotten about these docs. Useful to have this as a reference for this discussion because the change I'm proposing would also have an impact for these methods.

Specifically, in the case of String(), the docs would need to be changed to something like:

// String returns the underlying Javascript value as a string. If the type of the 
// underlying Javascript value is not string, then a runtime error is thrown. 
// If you want non-string values to be converted to string values you need to 
// perform that conversion first using js.Global.Call("String", o).String()

Again, the current behaviour can easily be achieved by performing the type conversion explicitly, as the docs try to show.

But the key is that by forcing the developer to be explicit about where these type conversions are happening we:

• reduce the number of surprises
• reduce the potential for errors
• make things more readable and therefore understandable
• ...

I wonder if we can focus on just the nil/undefined/zero-value cases in this issue (in accordance with the title), and open a separate issue to discuss the merits of strict type checking of JS types (i.e. If the type of the underlying Javascript value is not string, then a runtime error is thrown.).

@flimzy that's a good point. I wonder however whether we end up having to take the two at the same time (and correspondingly change the title to reflect that) because they are so interrelated. At least discussing the two together in order that we work out where we want to end up... we might phase that as two separate changes in PR terms.

I've just updated the related PR at #618 with the proposed docs change for js.Object.String() and a more explicit test for the internalising of a string value.

From all of our discussion above, it seems there are two (related) issues/questions related to internalization of values from JS to Go:

• How should null, undefined internalize to string, int, bool, float64, etc.? What value would it have for all of Go types?
• Should we add restrictions on what JS types can be internalized to what Go types. Can a non-string JavaScript type be internalized into a Go string, or should that cause a run-time error?

I'd like to hear from @neelance on the above. That would make moving forward on this easier.

Thanks @shurcooL - great summary. The only one change I would request is that null also be added alongside undefined in the first bullet. To my mind, both null and undefined are the only two primitive values that need to be interpreted in Go terms when it comes to talking about zero values in the context of $internalize.

Fair enough. I've added it now. The reason I skipped it originally is because I thought it was obvious that we'd want to internalize nil as zero value in Go, but it's still helpful to consider it when thinking about undefined.

I just ran across another data point for this discussion, when using time.Time: https://gopherjs.github.io/playground/#/V4PEKAnGjl

An uninitialized JS value mapped to time.Time results in a JS runtime error. I would expect a time.Time zero value.

I feel like having *js.Object in a struct is a clear signal that you are now dealing with javascript. The more go-like this becomes, the more mysterious *js.Object will be.

My current use case is to create data objects in gopherjs that are then being passed to websocket connects etc so that I am both the producer and consumer of the data. In other words I already know what is going in and out so for me it doesn't matter at all. More generally I always fall on the side of trying to make implementations simpler so I don't see the need to make internalization stricter.

@myitcv I know there's still some discussion/planning in the other proposals about structs with *js.Object, but from what I can tell, we're pretty much agreed on this one being a good idea.

I'm tempted to move it to "NeedsFix" stage, but I'd really like to see a final and complete table of possible/impossible conversions (in both directions). From looking over this thread, I don't quite see that, not all in one place. I think we need that to be able to to implement this and test the implementation works as expected.

Sorry, I should've communicated earlier what's needed to make progress on this, but I didn't get around to it until now.

If you ask for a string of an "undefined" object, that it is returned as the string "undefined" is not an issue in my mind. Most of the code a person writes in gopherjs will adhere naturally to types. The intersection with external javascript libraries is unpredictable, I suppose, so it should be loose unless you expect the gopherjs user to make sure any old js library they are interacting with has a strict sense of types.

@shurcooL 👍 - will work on the details.

Made a bit of progress on this today.

PR in #618 is updated.

@shurcooL as requested, as part of flushing out the details for $internalize and $externalize I'm maintaining a human-readable version of the conversions in the godoc for github.com/gopherjs/gopherjs/js. The docs in that package will need more work... so this is all WIP for now.

Another random data point I came across today, where GopherJS fails: pouchdb/pouchdb#6655

The PouchDB API is a case where strings and Date objects are, at times, used rather interchangeably. It's probably well beyond the scope of GopherJS to handle this situation gracefully, but it seems relevant to the spirit of the discussion just the same.

@neelance @shurcooL et al.

Based on where things currently stand in #618, I've hit on something of an issue.

Per the TODO list in the PR description, my choice of string as one of the first types was not entirely unintentional. Investigating step 2, the failure in the reflect tests, has uncovered the following.

The *js.Object String() method has, in the current (read master) implementation, effectively been overloaded to do two things:

1. convert a *js.Object value into a Go string value, according to Javascript String() type conversion semantics
2. act as a fmt.Stringer for *js.Object values

As you're aware, the compiler handles these methods as special cases; String() gets emitted as $internalize with a target type of string.

Under this (#617) proposal, use 1 is effectively hardened (within $internalize) to:

1. convert a *js.Object value into a Go string value if that value is either null, undefined, a string primitive of an object with constructor String; panic for all other values

This brings this use of String() in direct conflict with use 2. Consider:

o := js.Global.Get("Object").New()
fmt.Printf("%v", o)

o is of type *js.Object. But in this situation its Javascript value is an instance of Object. Hence calling String() will, per this proposal, panic.

Possible solutions

I'm not flush with ideas on this one right now, so more than anything adding this comment to get your thoughts.

However, here's what came immediately to mind.

Keep *js.Object.String() for use 2.

Create *js.Object.AsString()` and friends as follows for use case 1 to give us a method set of:

func (o *Object) AsBool() bool
func (o *Object) AsFloat() float64
func (o *Object) AsInt() int
func (o *Object) AsInt64() int64
func (o *Object) AsInterface() interface{}
func (o *Object) AsString() string
func (o *Object) AsUint64() uint64
func (o *Object) AsUnsafe() uintptr
func (o *Object) Call(name string, args ...interface{}) *Object
func (o *Object) Delete(key string)
func (o *Object) Get(key string) *Object
func (o *Object) Index(i int) *Object
func (o *Object) Invoke(args ...interface{}) *Object
func (o *Object) Length() int
func (o *Object) New(args ...interface{}) *Object
func (o *Object) Set(key string, value interface{})
func (o *Object) SetIndex(i int, value interface{})
func (o *Object) String() string

notice we drop *js.Object.Bool() and friends

These As* methods are then explicitly for the purposes of asserting the type of the *js.Object value.

Would welcome your thoughts.

I wish I could easily find where we discussed it last. But I raised the question of how fmt package is supposed to interact with *js.Object, and I think I'm okay with it being "unsupported" aka "undefined behavior" aka "if it panics, that's fine".

In general, I think we should be more strict and less forgiving in order to make positive progress. fmt package is well defined on Go types. To make it work with JavaScript object, we either have to do a lot of work ourselves (not very realistic given our limited time and resources), or just take the easy short term solution of saying "it's not supported, so you must covert your JS objects to Go types first, and you need to do that in a proper way."

I'll admit, it's hard for me to keep all the context related to this topic in my memory, and I don't have the bandwidth to dig more deeply into this right now. I hope you can make progress as is @myitcv, but let me know if you're blocked and need more to proceed.

@shurcooL

I wish I could easily find where we discussed it last

I think you're referring to the discussion in #625

In general, I think we should be more strict and less forgiving in order to make positive progress

Completely agree, indeed that was the motivation for this issue in the first place.

... or just take the easy short term solution of saying "it's not supported, so you must covert your JS objects to Go types first, and you need to do that in a proper way."

I understand the sentiment of this; we don't want to blow the scope of this piece of work unnecessarily. But at the moment I don't think the "do nothing" option is viable, i.e. I think we have to split the role of String() via a solution like As*. Reason being as follows.

Consider:

package main

import (
	"fmt"

	"honnef.co/go/js/dom"
)

func main() {
	el := dom.GetWindow().Document().QuerySelector("script")
	scr := el.(*dom.HTMLScriptElement)
	fmt.Printf("%v\n", scr)
}

This totally innocuous-looking code will in fact panic:

%!v(PANIC=runtime error: tried to internalize non-string value of type object/HTMLScriptElement)

In this situation, we have a *js.Object-special struct type, *dom.HTMLScriptElement. So the user cannot do anything more to "convert" the Javascript object: they are using a package API and type.

For this reason I don't think we can ignore or sidestep this issue.

More details on the As* proposal

I have a working commit (will be pushed up shortly), all tests passing, where the current (read master) String() et al methods have been renamed to As* equivalents. It's not a intrusive as I first thought it might be.

So by way of a simple example, using AsString() on a valid string primitive:

x := js.Global.Call("eval", `"hello"`)
fmt.Printf("%#v\n", x.AsString())

we get:

output: "hello"

Whereas when used with a number primitive:

x := js.Global.Call("eval", `5`)
fmt.Printf("%#v\n", x.AsString())

gives a runtime error:

Error: runtime error: tried to internalize non-string value of type number/Number

Making these changes therefore allowed me to addresses the question of how a *js.Object value should be formatted via new String() and GoString() methods (cc @slimsag via #625). This is covered in some tests but here are some examples:

inputs := []string{
	`undefined`,
	`null`,
	`5`,
	`"hello"`,
	`new Object()`,
	`Object({hello: "world", Age: 5})`,
	`new Array(1,2,3)`,
}

for _, v := range inputs {
	fmt.Printf("eval(%v) => %v\n", v, js.Global.Call("eval", v))
}

gives:

eval(undefined) => *js.Object[undefined]
eval(null) => *js.Object[null]
eval(5) => *js.Object[number/Number]
eval("hello") => *js.Object[string/String]
eval(new Object()) => *js.Object[object/Object]
eval(Object({hello: "world", Age: 5})) => *js.Object[object/Object]
eval(new Array(1,2,3)) => *js.Object[object/Array]

The thinking being, undefined and null are just that until someone tries to internalize either, at which point the definition of internalization for the target type kicks in. For everything else, we simply annotate with the source type, a combination of typeof and the constructor name to keep things simple.

The As* methods (well only AsString() and AsBool() for now given that I'm at step 1) are now, therefore, simply documented to internalize the *js.Object value. The package-level documentation in github.com/gopherjs/gopherjs/js covers the semantics of internalization and externalization.

Implications of this approach: docs/understanding

With *js.Object.String() no longer overloaded, we have the dual benefit of simpler docs and therefore simpler understanding.

Implications of this approach: refactoring

With the renaming of methods there is some refactoring required in packages that import github.com/gopherjs/gopherjs/js. Where code used to use Bool(), we will get a compile error (same for all other methods except String()); easy fix to then use AsBool() et al.

String() is slightly more complicated. As a starting point, all uses of String() should be changed to AsString(); this will then flush out cases at runtime where non-string values are trying to be internalized. This is a fix that people would have had to make in any case, irrespective of this As* addition.

In some very rare cases people might actually want the new *js.Object.String() method... but that will be very rare and one can easily revert to that method from AsString() where required

To help make this as smooth as possible we can:

1. use gorename to help make this refactor painless (flipping between commits of github.com/gopherjs/gopherjs)
2. use guru to statically consider each of the uses of the now-renamed AsString()
3. run tests to ensure we have concluded all the cases from point 2 correctly

Indeed I followed exactly these steps to fix honnef.co/go/js/dom locally.

Back to the motivating example

Taking the example from the beginning of this post:

func main() {
	el := dom.GetWindow().Document().QuerySelector("script")
	scr := el.(*dom.HTMLScriptElement)
	fmt.Printf("%v\n", scr)
}

If we look at the output under this proposal we get:

*js.Object[object/HTMLScriptElement]

Incidentally, I'm not entirely sure I agree with dom.HTMLScriptElement ultimately embedding (via *dom.BasicHTMLElement, which embeds *dom.BasicElement, which embeds *dom.BasicNode) *js.Object. It promotes the *js.Object.String() is promoted to *dom.HTMLScriptElement. But that's for a different discussion

Feedback

I realise there's a lot going on here. Thoughts/questions/corrections on the above welcomed. Indeed if it helps to have a quick Hangout or similar to run through this then I'm very happy to run a session.

My main concern here is of becoming too strict. The fast and loose typing of JS simply doesn't translate to Go's typing system easily, and runtime panics when one tries to convert between number/string, for example, seems very dangerous in some situations.

I don't have a specific solution in mind (I already mentioned the idea of a tag for lax conversion rules in struct conversions, which seemed popular, but since this isn't about structs....)

I suppose one possible solution might be a add an additional set of To* functions (ToString, ToBool, et al), which do a JS-style loose conversion?

@flimzy

I suppose one possible solution might be a add an additional set of To* functions (ToString, ToBool, et al), which do a JS-style loose conversion?

You will always have the option of doing this yourself as the caller. For example:

v := js.Global.Call("eval", "5")
// s := v.AsString() - runtime exception
s := js.Global.Call("String", v).AsString()

My main concern here is of becoming too strict.

The problem about not being strict is that we lose any sorts of ability to reason about the execution of our Go code... which is a problem.

The canonical example of where this is a problem is in this comment

I think we've discussed this before, and you already offered that work-around. My apologies for backtracking. Maybe it would be worth mentioning that in the new documentation, for those who want to retain the old/JS-style conversion behavior.

Maybe it would be worth mentioning that in the new documentation, for those who want to retain the old/JS-style conversion behavior.

That is an excellent suggestion, thank you!