cmd/compile, cmd/compile/internal/syntax: print relative column info

This change enables printing of relative column information if a
prior line directive specified a valid column. If there was no
line directive, or the line directive didn't specify a column
(or the -C flag is specified), no column information is shown in
file positions.

Implementation: Column values (and line values, for that matter)
that are zero are interpreted as "unknown". A line directive that
doesn't specify a column records that as a zero column in the
respective PosBase data structure. When computing relative columns,
a relative value is zero of the base's column value is zero.
When formatting a position, a zero column value is not printed.

To make this work without special cases, the PosBase for a file
is given a concrete (non-0:0) position 1:1 with the PosBase's
line and column also being 1:1. In other words, at the position
1:1 of a file, it's relative positions are starting with 1:1 as
one would expect.

In the package syntax, this requires self-recursive PosBases for
file bases, matching what cmd/internal/src.PosBase was already
doing. In src.PosBase, file and inlining bases also need to be
based at 1:1 to indicate "known" positions.

This change completes the cmd/compiler part of the issue below.

Fixes #22662.

Change-Id: I6c3d2dee26709581fba0d0261b1d12e93f1cba1a
Reviewed-on: https://go-review.googlesource.com/97375Reviewed-by: Matthew Dempsky <mdempsky@google.com>

src/cmd/compile/internal/gc/noder.go

@@ -78,13 +78,13 @@ func (p *noder) makeSrcPosBase(b0 *syntax.PosBase) *src.PosBase {
 	b1, ok := p.basemap[b0]
 	if !ok {
 		fn := b0.Filename()
-		if p0 := b0.Pos(); p0.IsKnown() {
+		if b0.IsFileBase() {
+			b1 = src.NewFileBase(fn, absFilename(fn))
+		} else {
 			// line directive base
+			p0 := b0.Pos()
 			p1 := src.MakePos(p.makeSrcPosBase(p0.Base()), p0.Line(), p0.Col())
 			b1 = src.NewLinePragmaBase(p1, fn, fileh(fn), b0.Line(), b0.Col())
-		} else {
-			// file base
-			b1 = src.NewFileBase(fn, absFilename(fn))
 		}
 		p.basemap[b0] = b1
 	}

src/cmd/compile/internal/syntax/parser.go

@@ -85,8 +85,8 @@ func (p *parser) init(file *PosBase, r io.Reader, errh ErrorHandler, pragh Pragm
 
 // updateBase sets the current position base to a new line base at pos.
 // The base's filename, line, and column values are extracted from text
-// which is positioned at (line, col) (only needed for error messages).
-func (p *parser) updateBase(pos Pos, line, col uint, text string) {
+// which is positioned at (tline, tcol) (only needed for error messages).
+func (p *parser) updateBase(pos Pos, tline, tcol uint, text string) {
 	i, n, ok := trailingDigits(text)
 	if i == 0 {
 		return // ignore (not a line directive)
@@ -95,38 +95,39 @@ func (p *parser) updateBase(pos Pos, line, col uint, text string) {
 
 	if !ok {
 		// text has a suffix :xxx but xxx is not a number
-		p.errorAt(p.posAt(line, col+i), "invalid line number: "+text[i:])
+		p.errorAt(p.posAt(tline, tcol+i), "invalid line number: "+text[i:])
 		return
 	}
 
+	var line, col uint
 	i2, n2, ok2 := trailingDigits(text[:i-1])
 	if ok2 {
 		//line filename:line:col
 		i, i2 = i2, i
-		n, n2 = n2, n
-		if n2 == 0 || n2 > PosMax {
-			p.errorAt(p.posAt(line, col+i2), "invalid column number: "+text[i2:])
+		line, col = n2, n
+		if col == 0 || col > PosMax {
+			p.errorAt(p.posAt(tline, tcol+i2), "invalid column number: "+text[i2:])
 			return
 		}
-		text = text[:i2-1] // lop off :col
+		text = text[:i2-1] // lop off ":col"
 	} else {
 		//line filename:line
-		n2 = colbase // use start of line for column
+		line = n
 	}
 
-	if n == 0 || n > PosMax {
-		p.errorAt(p.posAt(line, col+i), "invalid line number: "+text[i:])
+	if line == 0 || line > PosMax {
+		p.errorAt(p.posAt(tline, tcol+i), "invalid line number: "+text[i:])
 		return
 	}
 
 	// If we have a column (//line filename:line:col form),
 	// an empty filename means to use the previous filename.
-	filename := text[:i-1] // lop off :line
+	filename := text[:i-1] // lop off ":line"
 	if filename == "" && ok2 {
 		filename = p.base.Filename()
 	}
 
-	p.base = NewLineBase(pos, filename, n, n2)
+	p.base = NewLineBase(pos, filename, line, col)
 }
 
 func commentText(s string) string {

src/cmd/compile/internal/syntax/pos.go

@@ -31,10 +31,26 @@ func (pos Pos) Base() *PosBase { return pos.base }
 func (pos Pos) Line() uint     { return uint(pos.line) }
 func (pos Pos) Col() uint      { return uint(pos.col) }
 
-func (pos Pos) RelFilename() string { b := pos.Base(); return b.Filename() }
-func (pos Pos) RelLine() uint       { b := pos.Base(); return b.Line() + (pos.Line() - b.Pos().Line()) }
+func (pos Pos) RelFilename() string { return pos.base.Filename() }
+
+func (pos Pos) RelLine() uint {
+	b := pos.base
+	if b.Line() == 0 {
+		// base line is unknown => relative line is unknown
+		return 0
+	}
+	return b.Line() + (pos.Line() - b.Pos().Line())
+}
+
 func (pos Pos) RelCol() uint {
-	b := pos.Base()
+	b := pos.base
+	if b.Col() == 0 {
+		// base column is unknown => relative column is unknown
+		// (the current specification for line directives requires
+		// this to apply until the next PosBase/line directive,
+		// not just until the new newline)
+		return 0
+	}
 	if pos.Line() == b.Pos().Line() {
 		// pos on same line as pos base => column is relative to pos base
 		return b.Col() + (pos.Col() - b.Pos().Col())
@@ -43,13 +59,34 @@ func (pos Pos) RelCol() uint {
 }
 
 func (pos Pos) String() string {
-	s := fmt.Sprintf("%s:%d:%d", pos.RelFilename(), pos.RelLine(), pos.RelCol())
-	if bpos := pos.Base().Pos(); bpos.IsKnown() {
-		s += fmt.Sprintf("[%s:%d:%d]", bpos.RelFilename(), pos.Line(), pos.Col())
+	rel := position_{pos.RelFilename(), pos.RelLine(), pos.RelCol()}
+	abs := position_{pos.Base().Pos().RelFilename(), pos.Line(), pos.Col()}
+	s := rel.String()
+	if rel != abs {
+		s += "[" + abs.String() + "]"
 	}
 	return s
 }
 
+// TODO(gri) cleanup: find better name, avoid conflict with position in error_test.go
+type position_ struct {
+	filename  string
+	line, col uint
+}
+
+func (p position_) String() string {
+	if p.line == 0 {
+		if p.filename == "" {
+			return "<unknown position>"
+		}
+		return p.filename
+	}
+	if p.col == 0 {
+		return fmt.Sprintf("%s:%d", p.filename, p.line)
+	}
+	return fmt.Sprintf("%s:%d:%d", p.filename, p.line, p.col)
+}
+
 // A PosBase represents the base for relative position information:
 // At position pos, the relative position is filename:line:col.
 type PosBase struct {
@@ -59,9 +96,12 @@ type PosBase struct {
 }
 
 // NewFileBase returns a new PosBase for the given filename.
-// The PosBase position is unknown in this case.
+// A file PosBase's position is relative to itself, with the
+// position being filename:1:1.
 func NewFileBase(filename string) *PosBase {
-	return &PosBase{filename: filename}
+	base := &PosBase{MakePos(nil, linebase, colbase), filename, linebase, colbase}
+	base.pos.base = base
+	return base
 }
 
 // NewLineBase returns a new PosBase for a line directive "line filename:line:col"
@@ -73,6 +113,13 @@ func NewLineBase(pos Pos, filename string, line, col uint) *PosBase {
 	return &PosBase{pos, filename, sat32(line), sat32(col)}
 }
 
+func (base *PosBase) IsFileBase() bool {
+	if base == nil {
+		return false
+	}
+	return base.pos.base == base
+}
+
 func (base *PosBase) Pos() (_ Pos) {
 	if base == nil {
 		return

src/cmd/internal/src/pos.go

@@ -69,14 +69,28 @@ func (p *Pos) SetBase(base *PosBase) { p.base = base }
 func (p Pos) RelFilename() string { return p.base.Filename() }
 
 // RelLine returns the line number relative to the position's base.
-func (p Pos) RelLine() uint { b := p.base; return b.Line() + p.Line() - b.Pos().Line() }
+func (p Pos) RelLine() uint {
+	b := p.base
+	if b.Line() == 0 {
+		// base line is unknown => relative line is unknown
+		return 0
+	}
+	return b.Line() + (p.Line() - b.Pos().Line())
+}
 
 // RelCol returns the column number relative to the position's base.
 func (p Pos) RelCol() uint {
-	b := p.Base()
+	b := p.base
+	if b.Col() == 0 {
+		// base column is unknown => relative column is unknown
+		// (the current specification for line directives requires
+		// this to apply until the next PosBase/line directive,
+		// not just until the new newline)
+		return 0
+	}
 	if p.Line() == b.Pos().Line() {
 		// p on same line as p's base => column is relative to p's base
-		return b.Col() + p.Col() - b.Pos().Col()
+		return b.Col() + (p.Col() - b.Pos().Col())
 	}
 	return p.Col()
 }
@@ -93,10 +107,10 @@ func (p Pos) String() string {
 }
 
 // Format formats a position as "filename:line" or "filename:line:column",
-// controlled by the showCol flag. A position relative to a line directive
-// is always formatted without column information. In that case, if showOrig
-// is set, the original position (again controlled by showCol) is appended
-// in square brackets: "filename:line[origfile:origline:origcolumn]".
+// controlled by the showCol flag and if the column is known (!= 0).
+// For positions relative to line directives, the original position is
+// shown as well, as in "filename:line[origfile:origline:origcolumn] if
+// showOrig is set.
 func (p Pos) Format(showCol, showOrig bool) string {
 	if !p.IsKnown() {
 		return "<unknown line number>"
@@ -107,9 +121,6 @@ func (p Pos) Format(showCol, showOrig bool) string {
 		return format(p.Filename(), p.Line(), p.Col(), showCol)
 	}
 
-	// TODO(gri): Column information should be printed if a line
-	// directive explicitly specified a column, per issue #22662.
-
 	// base is relative
 	// Print the column only for the original position since the
 	// relative position's column information may be bogus (it's
@@ -118,7 +129,7 @@ func (p Pos) Format(showCol, showOrig bool) string {
 	// that's provided via a line directive).
 	// TODO(gri) This may not be true if we have an inlining base.
 	// We may want to differentiate at some point.
-	s := format(p.RelFilename(), p.RelLine(), 0, false)
+	s := format(p.RelFilename(), p.RelLine(), p.RelCol(), showCol)
 	if showOrig {
 		s += "[" + format(p.Filename(), p.Line(), p.Col(), showCol) + "]"
 	}
@@ -126,11 +137,11 @@ func (p Pos) Format(showCol, showOrig bool) string {
 }
 
 // format formats a (filename, line, col) tuple as "filename:line" (showCol
-// is false) or "filename:line:column" (showCol is true).
+// is false or col == 0) or "filename:line:column" (showCol is true and col != 0).
 func format(filename string, line, col uint, showCol bool) string {
 	s := filename + ":" + strconv.FormatUint(uint64(line), 10)
-	// col == colMax is interpreted as unknown column value
-	if showCol && col < colMax {
+	// col == 0 and col == colMax are interpreted as unknown column values
+	if showCol && 0 < col && col < colMax {
 		s += ":" + strconv.FormatUint(uint64(col), 10)
 	}
 	return s
@@ -141,8 +152,6 @@ func format(filename string, line, col uint, showCol bool) string {
 
 // A PosBase encodes a filename and base position.
 // Typically, each file and line directive introduce a PosBase.
-// A nil *PosBase is a ready to use file PosBase for an unnamed
-// file with line numbers starting at 1.
 type PosBase struct {
 	pos         Pos    // position at which the relative position is (line, col)
 	filename    string // file name used to open source file, for error messages
@@ -155,17 +164,16 @@ type PosBase struct {
 // NewFileBase returns a new *PosBase for a file with the given (relative and
 // absolute) filenames.
 func NewFileBase(filename, absFilename string) *PosBase {
-	if filename != "" {
 	base := &PosBase{
 		filename:    filename,
 		absFilename: absFilename,
 		symFilename: FileSymPrefix + absFilename,
+		line:        1,
+		col:         1,
 		inl:         -1,
 	}
-		base.pos = MakePos(base, 0, 0)
+	base.pos = MakePos(base, 1, 1)
 	return base
-	}
-	return nil
 }
 
 // NewLinePragmaBase returns a new *PosBase for a line directive of the form
@@ -180,8 +188,8 @@ func NewLinePragmaBase(pos Pos, filename, absFilename string, line, col uint) *P
 // index. If old == nil, the resulting PosBase has no filename.
 func NewInliningBase(old *PosBase, inlTreeIndex int) *PosBase {
 	if old == nil {
-		base := &PosBase{inl: inlTreeIndex}
-		base.pos = MakePos(base, 0, 0)
+		base := &PosBase{line: 1, col: 1, inl: inlTreeIndex}
+		base.pos = MakePos(base, 1, 1)
 		return base
 	}
 	copy := *old

src/cmd/internal/src/pos_test.go

@@ -39,23 +39,23 @@ func TestPos(t *testing.T) {
 		relLine, relCol uint
 	}{
 		{Pos{}, "<unknown line number>", "", 0, 0, "", 0, 0},
-		{MakePos(nil, 2, 3), ":2:3", "", 2, 3, "", 2, 3},
+		{MakePos(nil, 2, 3), ":2:3", "", 2, 3, "", 0, 0},
 		{MakePos(f0, 2, 3), ":2:3", "", 2, 3, "", 2, 3},
 		{MakePos(f1, 1, 1), "f1:1:1", "f1", 1, 1, "f1", 1, 1},
-		{MakePos(f2, 7, 10), "f2:17[:7:10]", "", 7, 10, "f2", 17, 10},
-		{MakePos(f3, 12, 7), "f3:102[f1:12:7]", "f1", 12, 7, "f3", 102, 7},
-		{MakePos(f4, 25, 1), "f4:115[f3:25:1]", "f3", 25, 1, "f4", 115, 1},
+		{MakePos(f2, 7, 10), "f2:17[:7:10]", "", 7, 10, "f2", 17, 0 /* line base doesn't specify a column */},
+		{MakePos(f3, 12, 7), "f3:102:7[f1:12:7]", "f1", 12, 7, "f3", 102, 7},
+		{MakePos(f4, 25, 1), "f4:115:1[f3:25:1]", "f3", 25, 1, "f4", 115, 1},
 
 		// line directives with non-1 columns
-		{MakePos(f5, 5, 5), "f5:10[f1:5:5]", "f1", 5, 5, "f5", 10, 1},
-		{MakePos(f5, 5, 10), "f5:10[f1:5:10]", "f1", 5, 10, "f5", 10, 6},
-		{MakePos(f5, 6, 10), "f5:11[f1:6:10]", "f1", 6, 10, "f5", 11, 10},
+		{MakePos(f5, 5, 5), "f5:10:1[f1:5:5]", "f1", 5, 5, "f5", 10, 1},
+		{MakePos(f5, 5, 10), "f5:10:6[f1:5:10]", "f1", 5, 10, "f5", 10, 6},
+		{MakePos(f5, 6, 10), "f5:11:10[f1:6:10]", "f1", 6, 10, "f5", 11, 10},
 
 		// positions from issue #19392
-		{MakePos(fc, 4, 1), "c.go:10[p.go:4:1]", "p.go", 4, 1, "c.go", 10, 1},
-		{MakePos(ft, 7, 1), "t.go:20[p.go:7:1]", "p.go", 7, 1, "t.go", 20, 1},
-		{MakePos(fv, 10, 1), "v.go:30[p.go:10:1]", "p.go", 10, 1, "v.go", 30, 1},
-		{MakePos(ff, 13, 1), "f.go:40[p.go:13:1]", "p.go", 13, 1, "f.go", 40, 1},
+		{MakePos(fc, 4, 1), "c.go:10:1[p.go:4:1]", "p.go", 4, 1, "c.go", 10, 1},
+		{MakePos(ft, 7, 1), "t.go:20:1[p.go:7:1]", "p.go", 7, 1, "t.go", 20, 1},
+		{MakePos(fv, 10, 1), "v.go:30:1[p.go:10:1]", "p.go", 10, 1, "v.go", 30, 1},
+		{MakePos(ff, 13, 1), "f.go:40:1[p.go:13:1]", "p.go", 13, 1, "f.go", 40, 1},
 	} {
 		pos := test.pos
 		if got := pos.String(); got != test.string {
@@ -134,10 +134,10 @@ func TestLico(t *testing.T) {
 		string    string
 		line, col uint
 	}{
-		{0, ":0:0", 0, 0},
-		{makeLico(0, 0), ":0:0", 0, 0},
+		{0, ":0", 0, 0},
+		{makeLico(0, 0), ":0", 0, 0},
 		{makeLico(0, 1), ":0:1", 0, 1},
-		{makeLico(1, 0), ":1:0", 1, 0},
+		{makeLico(1, 0), ":1", 1, 0},
 		{makeLico(1, 1), ":1:1", 1, 1},
 		{makeLico(2, 3), ":2:3", 2, 3},
 		{makeLico(lineMax, 1), fmt.Sprintf(":%d:1", lineMax), lineMax, 1},

test/fixedbugs/issue22662b.go

+// run
+
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Verify the impact of line directives on error positions and position formatting.
+
+package main
+
+import (
+	"io/ioutil"
+	"log"
+	"os"
+	"os/exec"
+	"runtime"
+	"strings"
+)
+
+// Each of these tests is expected to fail (missing package clause)
+// at the position determined by the preceeding line directive.
+var tests = []struct {
+	src, pos string
+}{
+	{"//line :10\n", ":10:"},                   // no filename means no filename
+	{"//line :10:4\n", "filename:10:4"},        // no filename means use existing filename
+	{"//line foo.go:10\n", "foo.go:10:"},       // no column means don't print a column
+	{"//line foo.go:10:4\n", "foo.go:10:4:"},   // column means print a column
+	{"//line foo.go:10:4\n\n", "foo.go:11:1:"}, // relative columns start at 1 after newline
+
+	{"/*line :10*/", ":10:"},
+	{"/*line :10:4*/", "filename:10:4"},
+	{"/*line foo.go:10*/", "foo.go:10:"},
+	{"/*line foo.go:10:4*/", "foo.go:10:4:"},
+	{"/*line foo.go:10:4*/\n", "foo.go:11:1:"},
+}
+
+func main() {
+	if runtime.GOOS == "nacl" {
+		return // no file system available on builders
+	}
+
+	f, err := ioutil.TempFile("", "issue22662b.go")
+	if err != nil {
+		log.Fatal(err)
+	}
+	f.Close()
+	defer os.Remove(f.Name())
+
+	for _, test := range tests {
+		if err := ioutil.WriteFile(f.Name(), []byte(test.src), 0660); err != nil {
+			log.Fatal(err)
+		}
+
+		out, err := exec.Command("go", "tool", "compile", f.Name()).CombinedOutput()
+		if err == nil {
+			log.Fatalf("expected compiling\n---\n%s\n---\nto fail", test.src)
+		}
+
+		errmsg := strings.Replace(string(out), f.Name(), "filename", -1) // use "filename" instead of actual (long) filename
+		if !strings.HasPrefix(errmsg, test.pos) {
+			log.Fatalf("%q: got %q; want position %q", test.src, errmsg, test.pos)
+		}
+	}
+}