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
Closed

cmd/compile: label redefinition error position is wrong #26411

odeke-em opened this issue Jul 17, 2018 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@odeke-em
Copy link
Member

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
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 17, 2018
@mvdan
Copy link
Member

mvdan commented Jul 17, 2018

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

@gopherbot
Copy link

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
Copy link

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
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>
@golang golang locked and limited conversation to collaborators Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants