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

cmd/compile: spurious error for missing argument for channel send within go func #33386

Closed
MaerF0x0 opened this issue Jul 31, 2019 · 6 comments

Comments

@MaerF0x0
Copy link

commented Jul 31, 2019

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

$ go version
go version go1.12.7 darwin/amd64

Does this issue reproduce with the latest release?

1.12.7 is the latest brew will install, lmk if I need the patch.

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

go env Output
$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/me/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/me/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.7/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_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=/var/folders/z3/089w1c5x2ngbzyfj0wb2lp440000gp/T/go-build853042324=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When attempting to send on a channel within a go func() {...} call , but missing the value to send go build is reporting 2 errors on other lines which are not the issue.

example code: https://play.golang.org/p/V7I7f9tc_17

package main

import (
	"fmt"
)

// Shows example with errors reported that distract from actual error
//./prog.go:12:5: expression in go must be function call
// ./prog.go:15:2: syntax error: unexpected }, expecting expression
func main() {
	send := make(chan int)
	go func() {
		// True error on line below. Missing value to send
		send <- 
	}()

	fmt.Println(<-send)
}

What did you expect to see?

a single error pointing to L14 of missing expression to send
Hypothetically

/prog.go:14:2: syntax error: unexpected \n, expecting expression for channel send

What did you see instead?

./prog.go:12:5: expression in go must be function call
./prog.go:15:2: syntax error: unexpected }, expecting expression

LMK if help wanted I'd love to contribute

@agnivade

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

The error on L15 is the correct one.

For eg., something like this is valid Go code.

func main() {
	send := make(chan int)
	go func() {
		// True error on line below. Missing value to send
		send <- 
	10 }()
	fmt.Println(<-send)
}

Leaving upto @griesemer and @mdempsky to see if something can be done about L12.

@agnivade agnivade added this to the Unplanned milestone Aug 1, 2019

@agnivade agnivade changed the title cmd/go build error message w/ missing argument for channel send within go func cmd/compile: spurious error for missing argument for channel send within go func Aug 1, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

As @agnivade points out, Go's syntax allows newlines between the <- token and the following expression. That's why we currently report at line 15 that we found a } when we expected an expression: we're still looking for an expression. So I think the proposed "unexpected \n" error message is inappropriate.

But we could maybe recognize cases like this somehow and emit a "missing expression" error instead?

As for the error on line 12, off hand I don't know what's causing that. I'm guessing error recovery gone wrong. @griesemer might have ideas on how to address that, and whether it's worthwhile.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I agree that the error on line 15 is correct. The error on line 12 is because the parser skips the closing '}' and then gets out of sync. Fix forthcoming.

The correct error message already says "expecting expression" which is close to "missing expression" (but a bit less definitive). I filed #33415 as a reminder.

@MaerF0x0

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@mdempsky / @griesemer what if the parser were to recognize it checked the next line for an expression and did not find one and then decrement the line number in the error message?

Again, for any of this let me know if you'd like a PR, I've been interested in contributing

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@MaerF0x0 It's not so simple in general. For one, there could be arbitrary many empty lines before the closing '}'. There could be comments, also arbitrary many. One could record the last valid token perhaps, and then use the position immediately after that token. But if that token is followed by comments than it's weird to get an error message before that comment. We provide the error when we see the first token that is unexpected. That is simple, sensible, consistent, and expected by many tests. A single slightly unfortunate case is not a good reason to change that. Generally, good error reporting by a recursive-descent parser is more of an art than a science. The fix here is simple I believe - I just need to test it a bit more before sending it out.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 1, 2019

Change https://golang.org/cl/188502 mentions this issue: cmd/compile/internal/syntax: better error recovery after missing expression

@gopherbot gopherbot closed this in dca0d03 Aug 27, 2019

tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
cmd/compile/internal/syntax: better error recovery after missing expr…
…ession

Don't skip closing parentheses of any kind after a missing
expression. They are likely part of the lexical construct
enclosing the expression.

Fixes golang#33386.

Change-Id: Ic0abc2037ec339a345ec357ccc724b7ad2a64c00
Reviewed-on: https://go-review.googlesource.com/c/go/+/188502
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/compile/internal/syntax: better error recovery after missing expr…
…ession

Don't skip closing parentheses of any kind after a missing
expression. They are likely part of the lexical construct
enclosing the expression.

Fixes golang#33386.

Change-Id: Ic0abc2037ec339a345ec357ccc724b7ad2a64c00
Reviewed-on: https://go-review.googlesource.com/c/go/+/188502
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.