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.

Sign up for GitHub

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

Jump to bottom

syscall/js: expected panic on String method on null and undefined values #29536

Closed
sternix opened this issue Jan 3, 2019 · 10 comments
Closed

syscall/js: expected panic on String method on null and undefined values #29536

sternix opened this issue Jan 3, 2019 · 10 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@sternix
Copy link

sternix commented Jan 3, 2019 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.12beta1 freebsd/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/sternix/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="freebsd"
GOOS="freebsd"
GOPATH="/home/sternix/go"
GOPROXY=""
GORACE=""
GOROOT="/opt/go/1_12_b1/go"
GOTMPDIR=""
GOTOOLDIR="/opt/go/1_12_b1/go/pkg/tool/freebsd_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build182700344=/tmp/go-build -gno-record-gcc-switches

What did you do?

// +build js,wasm
  
package main

import (
        "fmt"
        "syscall/js"
)

func main() {
        null := js.Null()

        //panic: syscall/js: call of Value.Int on null
        //fmt.Printf("Int: %d\n",null.Int())

        //panic: syscall/js: call of Value.Float on null
        //fmt.Printf("Float: %f\n",null.Float())

        //panic: syscall/js: call of Value.Bool on null
        //fmt.Printf("Bool: %t\n",null.Bool())

        //String: null
        fmt.Printf("String: %s\n", null.String())

        undefined := js.Undefined()

        //panic: syscall/js: call of Value.Int on undefined
        //fmt.Printf("Int: %d\n",undefined.Int())

        //panic: syscall/js: call of Value.Float on undefined
        //fmt.Printf("Float: %f\n",undefined.Float())

        //panic: syscall/js: call of Value.Bool on undefined
        //fmt.Printf("Bool: %t\n",undefined.Bool())

        //String: undefined
        fmt.Printf("String: %s\n", undefined.String())
}

What did you expect to see?

a panic or string's zero value ""

What did you see instead?

"null" on null values
"undefined" on undefined values
@agnivade
Copy link
Contributor

agnivade commented Jan 4, 2019

/cc @neelance

@myitcv
Copy link
Member

myitcv commented Jan 4, 2019

Hmm, I could have sworn that at some point in the past I saw tests that covered this case, precisely because (in contrast to the current implementation of GopherJS) we want to push any implicit type conversions back onto the call (i.e. force them to do the equivalent of String(null) if that's what they want). But clearly I'm imagining those tests....

@katiehockman katiehockman changed the title syscall/js: Behavior of String method on null and undefined values syscall/js: expected panic on String method on null and undefined values Jan 4, 2019
@katiehockman
Copy link
Contributor

/cc @alexbrainman (feel free to redirect to more appropriate person)
I wasn't able to identify an owner of the syscall/js package in https://dev.golang.org/owners. Once one is identified, I can update the documentation.

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 4, 2019
@myitcv
Copy link
Member

myitcv commented Jan 4, 2019

@katiehockman - @neelance is the owner of the WASM port.

@katiehockman
Copy link
Contributor

Great, thanks @myitcv.

@katiehockman katiehockman added arch-wasm WebAssembly issues and removed arch-wasm WebAssembly issues labels Jan 4, 2019
@alexbrainman
Copy link
Member

/cc @alexbrainman (feel free to redirect to more appropriate person)

Sorry, but I do not know much about syscall/js package. I am pretty sure @neelance is your man.

Alex

@neelance
Copy link
Member

neelance commented Jan 6, 2019

The documentation of js.Value.String says

String returns the value v converted to string according to JavaScript type conversions.

So this is working as expected. @sternix Why would you want a different behavior?

@neelance neelance added the arch-wasm WebAssembly issues label Jan 6, 2019
@myitcv
Copy link
Member

myitcv commented Jan 6, 2019

Then I'm totally mistaken in my memory :) Indeed it's been like this since the start, so I'm clearly wrong!

@neelance my memory is clearly influenced by the discussion we had on this topic for GopherJS, over in gopherjs/gopherjs#617.

The conclusion we reached in that thread was:

Following discussion with @neelance and @shurcooL we propose the following: to make the semantics of $internalize and $externalize follow that of encoding/json (a far more precise way to define the semantics than the headline description of this issue)

Taking the string type as an example, both null and undefined would therefore $internalize to "":

https://play.golang.org/p/8LFBTyuwGe

We will also panic in case the value being $internalize-d is not appropriate, much like:

https://play.golang.org/p/TtfZEPl9MJ

Amongst the reasons we concluded on this being the "right" behaviour were:

  1. it's impossible to opt out of this automatic type conversion and therefore have type safe programs by default
  2. it's entirely possible (and easy) for the user to loosen up a strict default perform such a conversion explicitly

Particularly given point 1, I think we need to reconsider the current behaviour.

@sternix
Copy link
Author

sternix commented Jan 6, 2019

The documentation of js.Value.String says

String returns the value v converted to string according to JavaScript type conversions.

So this is working as expected. @sternix Why would you want a different behavior?

@neelance Thanks for reply, i expect String method acts like golang behavior, if this is expected behavior you can close issue.

@sternix
Copy link
Author

sternix commented Jan 10, 2019

i think issue #29642 is the way go

@sternix sternix closed this as completed Jan 10, 2019
@golang golang locked and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@neelance @agnivade @myitcv @gopherbot @alexbrainman @sternix @katiehockman