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

misc/wasm: Possible bug in _makeFuncWrapper #36585

Closed
albrow opened this issue Jan 15, 2020 · 4 comments
Closed

misc/wasm: Possible bug in _makeFuncWrapper #36585

albrow opened this issue Jan 15, 2020 · 4 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.
Milestone

Comments

@albrow
Copy link
Contributor

albrow commented Jan 15, 2020

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

$ go version
go version go1.13.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes, judging by the contents of the master branch.

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

go env Output
$ go env
O111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/alex/Library/Caches/go-build"
GOENV="/Users/alex/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/alex/programming/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/alex/.go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/alex/.go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/alex/programming/go-mod/0x-mesh/go.mod"
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=/var/folders/h1/vhbpm31925nd0whbv4qd8mr00000gn/T/go-build550535634=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Related to #36561, I came across another unexpected error
from the TypeScript compiler.

Screen Shot 2020-01-15 at 1 33 25 PM

this should usually be avoided in anonymous functions, due to its often unexpected behavior. It's possible that this is intended, but I wanted to point it out for two reasons:

  1. It looks like it could be a bug. At the very least it is not following JavaScript best practices.
  2. The current version doesn't compile in TypeScript so I wanted to get confirmation about the intended behavior so I can write a version that behaves the same way but does work in TypeScript.

As a quick reminder about the behavior of this inside anonymous functions:

  1. If you are not running in strict mode, this defaults to the global scope (e.g. window). In strict mode it defaults to undefined.
  2. You can set the value of this by using bind. In order to do this, you need a reference to the function. So if we have a function called myFunc, you could call it this way: myFunc.bind(myObj).call(). This would result in this being set to myObj.

It was difficult for me to find where the function was called, but if you are not calling it via bind, it means that this is always undefined. This might be a bug that should be addressed. On the other hand if everything still works, this could simply be removed:

            return function() {
-                const event = { id: id, this: this, args: arguments };
+                const event = { id: id, args: arguments };
                go._pendingEvent = event;
                go._resume();
                return event.result;
            };

If you are using bind, I would suggest simply passing in an argument to the function in order to adhere to JavaScript best practices. I'm not sure what the argument should be called since I don't have the full context of how this function is being used, but it would look something like this:

            return function(myObj) {
-                const event = { id: id, this: this, args: arguments };
+                const event = { id: id, myObj: myObj, args: arguments };
                go._pendingEvent = event;
                go._resume();
                return event.result;
            };

Maybe there is some reason the function can't be written that way? I'm not sure.

What did you expect to see?

Again, I don't expect this file to compile without making changes since it wasn't written with TypeScript in mind. I was surprised by this particular error and I think it points to a potential bug.

What did you see instead?

The TypeScript compiler complains about the usage of this inside an anonymous function.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 16, 2020
@ALTree
Copy link
Member

ALTree commented Jan 16, 2020

cc @neelance

@ALTree ALTree added this to the Backlog milestone Jan 16, 2020
@ALTree ALTree added the arch-wasm WebAssembly issues label Jan 16, 2020
@bradfitz bradfitz modified the milestones: Backlog, Go1.15 Jan 16, 2020
@fabioberger
Copy link

Any update on this @neelance?

@neelance
Copy link
Member

You don't have to use bind, it is much more common to assign the function to a property of some object and then call it as a method:

const fn = function () { console.log("the answer:", this.answer); };
const deepThought = { answer: 42 };
deepThought.calculateAnswer = fn;
deepThought.calculateAnswer();

Since the wrapper function is meant to capture this, there is no other way to write it.

@albrow
Copy link
Contributor Author

albrow commented Jan 24, 2020

Ahh you're right. I didn't know about this additional quirk of this. I still need to find a way to satisfy the TypeScript compiler but that's my problem, not yours.

@albrow albrow closed this as completed Jan 24, 2020
@golang golang locked and limited conversation to collaborators Jan 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

No branches or pull requests

6 participants