cmd/compile: generate closure for go/defer builtin calls #19710

Open
unixpickle opened this Issue Mar 25, 2017 · 10 comments

Comments

Projects
None yet
8 participants
@unixpickle

unixpickle commented Mar 25, 2017

Please answer these questions before submitting your issue. Thanks!

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

Go 1.8.
Also happens on play.golang.org.

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

Evaluate this code:

package main

import "fmt"

func main() {
	fmt.Println("should be 0:", len(foobar()))
}

func foobar() map[int]bool {
	m := map[int]bool{}
	for i := 0; i < 3; i++ {
		m[i] = true
		defer delete(m, i)
	}
	return m
}

What did you expect to see?

should be 0: 0

What did you see instead?

should be 0: 2

The defer statement is supposed to freeze its arguments the instant it is evaluated, but it does not when used with delete. Compare to this program, which works correctly:

package main

import "fmt"

func main() {
	fmt.Println("should be 0:", len(foobar()))
}

func foobar() map[int]bool {
	m := map[int]bool{}
	for i := 0; i < 3; i++ {
		m[i] = true
		defer myDelete(m, i)
	}
	return m
}

func myDelete(m map[int]bool, k int) {
	delete(m, k)
}

unixpickle added a commit to unixpickle/anynet that referenced this issue Mar 25, 2017

unixpickle added a commit to unixpickle/sgdstore that referenced this issue Mar 25, 2017

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Mar 25, 2017

Member

This is my interesting. Why you use defer delete?

Member

mattn commented Mar 25, 2017

This is my interesting. Why you use defer delete?

@unixpickle

This comment has been minimized.

Show comment
Hide comment
@unixpickle

unixpickle Mar 25, 2017

@mattn I have a map that I occasionally add temporary values to (for intermediate computations). I was trying to use defer+delete to remove those temporary entries automatically.

@mattn I have a map that I occasionally add temporary values to (for intermediate computations). I was trying to use defer+delete to remove those temporary entries automatically.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Mar 25, 2017

Member

Good news and bad news:

Bad news is this has been broken forever. (I tested Go 1.5, Go 1.6, Go 1.7, and Go 1.8) Because it's not a regression and has been broken forever, it doesn't qualify for a Go 1.8.1 cherry-pick.

Good news is that it's fixed at tip (what will be Go 1.9).

I haven't bisected to see where it was fixed and whether it was intentional. If it was fixed by accident, it likely still lacks a test.

/cc @griesemer @randall77 @mdempsky @josharian

Member

bradfitz commented Mar 25, 2017

Good news and bad news:

Bad news is this has been broken forever. (I tested Go 1.5, Go 1.6, Go 1.7, and Go 1.8) Because it's not a regression and has been broken forever, it doesn't qualify for a Go 1.8.1 cherry-pick.

Good news is that it's fixed at tip (what will be Go 1.9).

I haven't bisected to see where it was fixed and whether it was intentional. If it was fixed by accident, it likely still lacks a test.

/cc @griesemer @randall77 @mdempsky @josharian

@bradfitz bradfitz changed the title from defer+delete argument bug to cmd/compile: defer+delete argument bug Mar 25, 2017

@bradfitz bradfitz added this to the Go1.9 milestone Mar 25, 2017

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Mar 25, 2017

Contributor

Probably fixed by the order.go changes for mapdelete_fast*. Yes, needs a test. Should double-check go delete(x), and defer/delete of the other built-in functions.

Contributor

josharian commented Mar 25, 2017

Probably fixed by the order.go changes for mapdelete_fast*. Yes, needs a test. Should double-check go delete(x), and defer/delete of the other built-in functions.

@cespare

This comment has been minimized.

Show comment
Hide comment
@cespare

cespare Mar 25, 2017

Contributor

A bisect points at 5d6b7fc (CL 38172)

runtime: add mapdelete_fast*

Contributor

cespare commented Mar 25, 2017

A bisect points at 5d6b7fc (CL 38172)

runtime: add mapdelete_fast*

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Mar 25, 2017

Contributor

order.go, walk.go, and ssa.go are scattered with special handling of go/defer of built-in functions. I wonder whether instead we should just do a very early rewrite of

defer builtin(a, b, c)

to

defer func(a typa, b typb, c typc) {
  builtin(a, b, c)
}(a, b, c)

That would probably also simplify instrumentation of such cases, which are also the topic of scattered special handling.

Contributor

josharian commented Mar 25, 2017

order.go, walk.go, and ssa.go are scattered with special handling of go/defer of built-in functions. I wonder whether instead we should just do a very early rewrite of

defer builtin(a, b, c)

to

defer func(a typa, b typb, c typc) {
  builtin(a, b, c)
}(a, b, c)

That would probably also simplify instrumentation of such cases, which are also the topic of scattered special handling.

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Mar 25, 2017

Contributor

@josharian I was thinking the same thing.

Contributor

randall77 commented Mar 25, 2017

@josharian I was thinking the same thing.

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian May 15, 2017

Contributor

I think we should add a test for 1.9 and and leave the closurization of deferred built-ins for 1.10. (That will also enable us to simplify other things, like not lowering OPANIC in walk.go.)

I'll send a test CL.

Contributor

josharian commented May 15, 2017

I think we should add a test for 1.9 and and leave the closurization of deferred built-ins for 1.10. (That will also enable us to simplify other things, like not lowering OPANIC in walk.go.)

I'll send a test CL.

@gopherbot

This comment has been minimized.

Show comment
Hide comment

CL https://golang.org/cl/43497 mentions this issue.

gopherbot pushed a commit that referenced this issue May 15, 2017

test: add test of deferred delete
Updates #19710

Change-Id: I37d19a4a02b9010cb5f9062b3d141d5d65e12e01
Reviewed-on: https://go-review.googlesource.com/43497
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@josharian josharian modified the milestones: Go1.10, Go1.9 May 15, 2017

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian May 19, 2017

Contributor

Note to self: walkprintfunc does this now for defer println(...). It also has a minor naming bug: lookupN("print·%d", walkprintfunc_prgen) generates names like "".print·%d1·f. :P

Contributor

josharian commented May 19, 2017

Note to self: walkprintfunc does this now for defer println(...). It also has a minor naming bug: lookupN("print·%d", walkprintfunc_prgen) generates names like "".print·%d1·f. :P

@cherrymui cherrymui changed the title from cmd/compile: defer+delete argument bug to cmd/compile: generate closure for go/defer builtin calls Nov 29, 2017

@cherrymui cherrymui modified the milestones: Go1.10, Go1.11 Nov 29, 2017

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment