From 3d6b368514f2b72538c23a27f248684dd9cca227 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 7 Feb 2012 15:19:52 -0800 Subject: [PATCH] go/printer, gofmt: don't print incorrect programs Be careful when printing line comments with incorrect position information. Maintain additional state impliedSemi: when set, a comment containing a newline would imply a semicolon and thus placement must be delayed. Precompute state information pertaining to the next comment for faster checks (the printer is marginally faster now despite additional checks for each comment). No effect on existing src, misc sources. Fixes #1505. R=rsc CC=golang-dev https://golang.org/cl/5598054 --- src/cmd/fix/timefileinfo_test.go | 26 +++++ src/pkg/go/printer/nodes.go | 1 + src/pkg/go/printer/printer.go | 153 +++++++++++++++++++++-------- src/pkg/go/printer/printer_test.go | 75 ++++++++++++-- 4 files changed, 209 insertions(+), 46 deletions(-) diff --git a/src/cmd/fix/timefileinfo_test.go b/src/cmd/fix/timefileinfo_test.go index 76d5c1f7ff2..6573b854564 100644 --- a/src/cmd/fix/timefileinfo_test.go +++ b/src/cmd/fix/timefileinfo_test.go @@ -156,6 +156,32 @@ func main() { t2 := time.Now() dt := t2.Sub(t1) } +`, + }, + { + Name: "timefileinfo.5", // test for issues 1505, 2636 + In: `package main + +import ( + "fmt" + "time" +) + +func main() { + fmt.Println(time.SecondsToUTC(now)) // this comment must not introduce an illegal linebreak +} +`, + Out: `package main + +import ( + "fmt" + "time" +) + +func main() { + fmt.Println(time.Unix(now, 0).UTC( // this comment must not introduce an illegal linebreak + )) +} `, }, } diff --git a/src/pkg/go/printer/nodes.go b/src/pkg/go/printer/nodes.go index 5f3b4d4a740..25935fb42bb 100644 --- a/src/pkg/go/printer/nodes.go +++ b/src/pkg/go/printer/nodes.go @@ -76,6 +76,7 @@ func (p *printer) setComment(g *ast.CommentGroup) { } p.comments[0] = g p.cindex = 0 + p.nextComment() // get comment ready for use } type exprListMode uint diff --git a/src/pkg/go/printer/printer.go b/src/pkg/go/printer/printer.go index f1222d874b9..fe99e675eb0 100644 --- a/src/pkg/go/printer/printer.go +++ b/src/pkg/go/printer/printer.go @@ -12,7 +12,6 @@ import ( "go/token" "io" "os" - "path/filepath" "strconv" "strings" "text/tabwriter" @@ -52,11 +51,12 @@ type printer struct { fset *token.FileSet // Current state - output bytes.Buffer // raw printer result - indent int // current indentation - mode pmode // current printer mode - lastTok token.Token // the last token printed (token.ILLEGAL if it's whitespace) - wsbuf []whiteSpace // delayed white space + output bytes.Buffer // raw printer result + indent int // current indentation + mode pmode // current printer mode + impliedSemi bool // if set, a linebreak implies a semicolon + lastTok token.Token // the last token printed (token.ILLEGAL if it's whitespace) + wsbuf []whiteSpace // delayed white space // The (possibly estimated) position in the generated output; // in AST space (i.e., pos is set whenever a token position is @@ -73,6 +73,11 @@ type printer struct { cindex int // current comment index useNodeComments bool // if not set, ignore lead and line comments of nodes + // Information about p.comments[p.cindex]; set up by nextComment. + comment *ast.CommentGroup // = p.comments[p.cindex]; or nil + commentOffset int // = p.posFor(p.comments[p.cindex].List[0].Pos()).Offset; or infinity + commentNewline bool // true if the comment group contains newlines + // Cache of already computed node sizes. nodeSizes map[ast.Node]int @@ -89,6 +94,42 @@ func (p *printer) init(cfg *Config, fset *token.FileSet, nodeSizes map[ast.Node] p.cachedPos = -1 } +// commentsHaveNewline reports whether a list of comments belonging to +// an *ast.CommentGroup contains newlines. Because the position information +// may only be partially correct, we also have to read the comment text. +func (p *printer) commentsHaveNewline(list []*ast.Comment) bool { + // len(list) > 0 + line := p.lineFor(list[0].Pos()) + for i, c := range list { + if i > 0 && p.lineFor(list[i].Pos()) != line { + // not all comments on the same line + return true + } + if t := c.Text; len(t) >= 2 && (t[1] == '/' || strings.Contains(t, "\n")) { + return true + } + } + _ = line + return false +} + +func (p *printer) nextComment() { + for p.cindex < len(p.comments) { + c := p.comments[p.cindex] + p.cindex++ + if list := c.List; len(list) > 0 { + p.comment = c + p.commentOffset = p.posFor(list[0].Pos()).Offset + p.commentNewline = p.commentsHaveNewline(list) + return + } + // we should not reach here (correct ASTs don't have empty + // ast.CommentGroup nodes), but be conservative and try again + } + // no more comments + p.commentOffset = infinity +} + func (p *printer) internalError(msg ...interface{}) { if debug { fmt.Print(p.pos.String() + ": ") @@ -204,8 +245,7 @@ func (p *printer) writeItem(pos token.Position, data string, isLit bool) { } if debug { // do not update p.pos - use write0 - _, filename := filepath.Split(pos.Filename) - fmt.Fprintf(&p.output, "[%s:%d:%d]", filename, pos.Line, pos.Column) + fmt.Fprintf(&p.output, "/*%s*/", pos) } p.writeString(data, isLit) p.last = p.pos @@ -618,12 +658,13 @@ func (p *printer) writeCommentSuffix(needsLinebreak bool) (wroteNewline, dropped // func (p *printer) intersperseComments(next token.Position, tok token.Token) (wroteNewline, droppedFF bool) { var last *ast.Comment - for ; p.commentBefore(next); p.cindex++ { - for _, c := range p.comments[p.cindex].List { + for p.commentBefore(next) { + for _, c := range p.comment.List { p.writeCommentPrefix(p.posFor(c.Pos()), next, last, c, tok.IsKeyword()) p.writeComment(c) last = c } + p.nextComment() } if last != nil { @@ -735,22 +776,24 @@ func mayCombine(prev token.Token, next byte) (b bool) { // printed, followed by the actual token. // func (p *printer) print(args ...interface{}) { - for _, f := range args { - next := p.pos // estimated position of next item - data := "" - isLit := false - var tok token.Token + for _, arg := range args { + // information about the current arg + var data string + var isLit bool + var impliedSemi bool // value for p.impliedSemi after this arg - switch x := f.(type) { + switch x := arg.(type) { case pmode: // toggle printer mode p.mode ^= x + continue + case whiteSpace: if x == ignore { // don't add ignore's to the buffer; they // may screw up "correcting" unindents (see // LabeledStmt) - break + continue } i := len(p.wsbuf) if i == cap(p.wsbuf) { @@ -762,13 +805,27 @@ func (p *printer) print(args ...interface{}) { } p.wsbuf = p.wsbuf[0 : i+1] p.wsbuf[i] = x + if x == newline || x == formfeed { + // newlines affect the current state (p.impliedSemi) + // and not the state after printing arg (impliedSemi) + // because comments can be interspersed before the arg + // in this case + p.impliedSemi = false + } + p.lastTok = token.ILLEGAL + continue + case *ast.Ident: data = x.Name - tok = token.IDENT + impliedSemi = true + p.lastTok = token.IDENT + case *ast.BasicLit: data = x.Value isLit = true - tok = x.Kind + impliedSemi = true + p.lastTok = x.Kind + case token.Token: s := x.String() if mayCombine(p.lastTok, s[0]) { @@ -785,30 +842,40 @@ func (p *printer) print(args ...interface{}) { p.wsbuf[0] = ' ' } data = s - tok = x + // some keywords followed by a newline imply a semicolon + switch x { + case token.BREAK, token.CONTINUE, token.FALLTHROUGH, token.RETURN, + token.INC, token.DEC, token.RPAREN, token.RBRACK, token.RBRACE: + impliedSemi = true + } + p.lastTok = x + case token.Pos: if x.IsValid() { - next = p.posFor(x) // accurate position of next item + p.pos = p.posFor(x) // accurate position of next item } - tok = p.lastTok + continue + case string: // incorrect AST - print error message data = x isLit = true - tok = token.STRING + impliedSemi = true + p.lastTok = token.STRING + default: - fmt.Fprintf(os.Stderr, "print: unsupported argument %v (%T)\n", f, f) + fmt.Fprintf(os.Stderr, "print: unsupported argument %v (%T)\n", arg, arg) panic("go/printer type") } - p.lastTok = tok - p.pos = next + // data != "" - if data != "" { - wroteNewline, droppedFF := p.flush(next, tok) + next := p.pos // estimated/accurate position of next item + wroteNewline, droppedFF := p.flush(next, p.lastTok) - // intersperse extra newlines if present in the source - // (don't do this in flush as it will cause extra newlines - // at the end of a file) + // intersperse extra newlines if present in the source and + // if they don't cause extra semicolons (don't do this in + // flush as it will cause extra newlines at the end of a file) + if !p.impliedSemi { n := nlimit(next.Line - p.pos.Line) // don't exceed maxNewlines if we already wrote one if wroteNewline && n == maxNewlines { @@ -820,22 +887,25 @@ func (p *printer) print(args ...interface{}) { ch = '\f' // use formfeed since we dropped one before } p.writeByteN(ch, n) + impliedSemi = false } - - p.writeItem(next, data, isLit) } + + p.writeItem(next, data, isLit) + p.impliedSemi = impliedSemi } } -// commentBefore returns true iff the current comment occurs -// before the next position in the source code. +// commentBefore returns true iff the current comment group occurs +// before the next position in the source code and printing it does +// not introduce implicit semicolons. // -func (p *printer) commentBefore(next token.Position) bool { - return p.cindex < len(p.comments) && p.posFor(p.comments[p.cindex].List[0].Pos()).Offset < next.Offset +func (p *printer) commentBefore(next token.Position) (result bool) { + return p.commentOffset < next.Offset && (!p.impliedSemi || !p.commentNewline) } -// Flush prints any pending comments and whitespace occurring textually -// before the position of the next token tok. The Flush result indicates +// flush prints any pending comments and whitespace occurring textually +// before the position of the next token tok. The flush result indicates // if a newline was written or if a formfeed was dropped from the whitespace // buffer. // @@ -915,6 +985,9 @@ func (p *printer) printNode(node interface{}) error { // if there are no comments, use node comments p.useNodeComments = p.comments == nil + // get comments ready for use + p.nextComment() + // format node switch n := node.(type) { case ast.Expr: @@ -1068,6 +1141,8 @@ func (cfg *Config) fprint(output io.Writer, fset *token.FileSet, node interface{ if err = p.printNode(node); err != nil { return } + // print outstanding comments + p.impliedSemi = false // EOF acts like a newline p.flush(token.Position{Offset: infinity, Line: infinity}, token.EOF) // redirect output through a trimmer to eliminate trailing whitespace diff --git a/src/pkg/go/printer/printer_test.go b/src/pkg/go/printer/printer_test.go index 525fcc1595f..9adf48cda61 100644 --- a/src/pkg/go/printer/printer_test.go +++ b/src/pkg/go/printer/printer_test.go @@ -171,14 +171,14 @@ func TestLineComments(t *testing.T) { ` fset := token.NewFileSet() - ast1, err1 := parser.ParseFile(fset, "", src, parser.ParseComments) - if err1 != nil { - panic(err1) + f, err := parser.ParseFile(fset, "", src, parser.ParseComments) + if err != nil { + panic(err) // error in test } var buf bytes.Buffer fset = token.NewFileSet() // use the wrong file set - Fprint(&buf, fset, ast1) + Fprint(&buf, fset, f) nlines := 0 for _, ch := range buf.Bytes() { @@ -190,6 +190,7 @@ func TestLineComments(t *testing.T) { const expected = 3 if nlines < expected { t.Errorf("got %d, expected %d\n", nlines, expected) + t.Errorf("result:\n%s", buf.Bytes()) } } @@ -198,9 +199,11 @@ func init() { const name = "foobar" var buf bytes.Buffer if err := Fprint(&buf, fset, &ast.Ident{Name: name}); err != nil { - panic(err) + panic(err) // error in test } - if s := buf.String(); s != name { + // in debug mode, the result contains additional information; + // ignore it + if s := buf.String(); !debug && s != name { panic("got " + s + ", want " + name) } } @@ -211,7 +214,7 @@ func TestBadNodes(t *testing.T) { const res = "package p\nBadDecl\n" f, err := parser.ParseFile(fset, "", src, parser.ParseComments) if err == nil { - t.Errorf("expected illegal program") + t.Error("expected illegal program") // error in test } var buf bytes.Buffer Fprint(&buf, fset, f) @@ -219,3 +222,61 @@ func TestBadNodes(t *testing.T) { t.Errorf("got %q, expected %q", buf.String(), res) } } + +// Print and parse f with +func testComment(t *testing.T, f *ast.File, srclen int, comment *ast.Comment) { + f.Comments[0].List[0] = comment + var buf bytes.Buffer + for offs := 0; offs <= srclen; offs++ { + buf.Reset() + // Printing f should result in a correct program no + // matter what the (incorrect) comment position is. + if err := Fprint(&buf, fset, f); err != nil { + t.Error(err) + } + if _, err := parser.ParseFile(fset, "", buf.Bytes(), 0); err != nil { + t.Fatalf("incorrect program for pos = %d:\n%s", comment.Slash, buf.String()) + } + // Position information is just an offset. + // Move comment one byte down in the source. + comment.Slash++ + } +} + +// Verify that the printer produces always produces a correct program +// even if the position information of comments introducing newlines +// is incorrect. +func TestBadComments(t *testing.T) { + const src = ` +// first comment - text and position changed by test +package p +import "fmt" +const pi = 3.14 // rough circle +var ( + x, y, z int = 1, 2, 3 + u, v float64 +) +func fibo(n int) { + if n < 2 { + return n /* seed values */ + } + return fibo(n-1) + fibo(n-2) +} +` + + f, err := parser.ParseFile(fset, "", src, parser.ParseComments) + if err != nil { + t.Error(err) // error in test + } + + comment := f.Comments[0].List[0] + pos := comment.Pos() + if fset.Position(pos).Offset != 1 { + t.Error("expected offset 1") // error in test + } + + testComment(t, f, len(src), &ast.Comment{pos, "//-style comment"}) + testComment(t, f, len(src), &ast.Comment{pos, "/*-style comment */"}) + testComment(t, f, len(src), &ast.Comment{pos, "/*-style \n comment */"}) + testComment(t, f, len(src), &ast.Comment{pos, "/*-style comment \n\n\n */"}) +}