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: label redefinition error position is wrong #26411

Closed
odeke-em opened this issue Jul 17, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@odeke-em
Copy link
Member

commented Jul 17, 2018

I was just studying the syntax checker and parser and noticed that for label reuse errors in

name := s.Label.Value
labels := ls.labels
if labels == nil {
labels = make(map[string]*label)
ls.labels = labels
} else if alt := labels[name]; alt != nil {
ls.err(s.Pos(), "label %s already defined at %s", name, alt.lstmt.Label.Pos().String())

while printing the error, we use a label's parsed statement position, of which that position is until the last token that delimits a label aka ":" instead of actually where the label starts from. However, for the original label, we actually use the label's start position. I believe this issue might affect people whose code editors jump to line numbers/positions on errors.

A simple exhibit below https://play.golang.org/p/Oi-4doqbEN1 or inlined below:

package main

func main() {
foo:
foo:
} 

gives the error

prog.go:4:1: label foo defined and not used
prog.go:5:4: label foo already defined at prog.go:4:1

but notice the error message
prog.go:5:4: label foo already defined at prog.go:4:1

the position for the first label is correctly reported as 4:1 but for the redefined one as 5:4 yet it should be 5:1

I am filling this issue for the purposes of asking if this is the intended behavior and if not, for purposes of tracking the bug. This issue is not a regression and exists also in Go1.10

The fix seems straightforward to me, which is to use s.Label.Pos() instead of s.Pos() aka

diff --git a/src/cmd/compile/internal/syntax/branches.go b/src/cmd/compile/internal/syntax/branches.go
index a03e2734d2..cdeec863bc 100644
--- a/src/cmd/compile/internal/syntax/branches.go
+++ b/src/cmd/compile/internal/syntax/branches.go
@@ -77,7 +77,7 @@ func (ls *labelScope) declare(b *block, s *LabeledStmt) *label {
                labels = make(map[string]*label)
                ls.labels = labels
        } else if alt := labels[name]; alt != nil {
-               ls.err(s.Pos(), "label %s already defined at %s", name, alt.lstmt.Label.Pos().String())
+               ls.err(s.Label.Pos(), "label %s already defined at %s", name, alt.lstmt.Label.Pos())
                return alt
        }
        l := &label{b, s, false}

which will then correctly print

$ go tool compile branches.go 
prog.go:4:1: label foo defined and not used
prog.go:5:1: label foo already defined at prog.go:4:1

/cc @griesemer

@odeke-em odeke-em added this to the Go1.12 milestone Jul 17, 2018

@odeke-em odeke-em changed the title cmd/compile: label reuse error position is wrong cmd/compile: label redefinition error position is wrong Jul 17, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

@odeke-em why not send that patch as a CL, along with a test?

@gopherbot

This comment has been minimized.

Copy link

commented Jul 18, 2018

Change https://golang.org/cl/124595 mentions this issue: cmd/compile: fix label redefinition error column numbers

@odeke-em odeke-em self-assigned this Oct 16, 2018

@gopherbot gopherbot closed this in 0b63086 Oct 16, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Oct 16, 2018

Change https://golang.org/cl/142720 mentions this issue: cmd/compile: simplified test case (cleanup)

gopherbot pushed a commit that referenced this issue Oct 16, 2018

cmd/compile: simplified test case (cleanup)
Follow-up on https://golang.org/cl/124595; no semantic changes.

Updates #26411.

Change-Id: Ic1c4622dbf79529ff61530f9c25ec742c2abe5ca
Reviewed-on: https://go-review.googlesource.com/c/142720
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.