From 241b23606c4c1d37071669c6e07c1918501835ff Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 9 Oct 2012 17:08:09 -0700 Subject: [PATCH] go/printer: idempotent comment formatting Also: - Refactored testing framework to permit easier idempotency testing. - Applied gofmt -w src misc This CL depends on CL 6639044 being applied first. Formatting is not idempotent for all files: In those files the comment position has changed (due to missing precise location information) and/or the comment formatting cannot/is not aware of independent code re-formatting. In general it is very hard to make format idempotent when running it in one pass only. Leaving that aside for now. Fixes #1835. R=r, rsc CC=golang-dev https://golang.org/cl/6624051 --- src/pkg/go/printer/printer.go | 33 +++- src/pkg/go/printer/printer_test.go | 150 ++++++++++++------- src/pkg/go/printer/testdata/comments2.golden | 79 ++++++++++ src/pkg/go/printer/testdata/comments2.input | 79 ++++++++++ 4 files changed, 277 insertions(+), 64 deletions(-) create mode 100644 src/pkg/go/printer/testdata/comments2.golden create mode 100644 src/pkg/go/printer/testdata/comments2.input diff --git a/src/pkg/go/printer/printer.go b/src/pkg/go/printer/printer.go index a027d32da89..516c37161c1 100644 --- a/src/pkg/go/printer/printer.go +++ b/src/pkg/go/printer/printer.go @@ -449,11 +449,17 @@ func commonPrefix(a, b string) string { return a[0:i] } +// stripCommonPrefix removes a common prefix from /*-style comment lines (unless no +// comment line is indented, all but the first line have some form of space prefix). +// The prefix is computed using heuristics such that is is likely that the comment +// contents are nicely laid out after re-printing each line using the printer's +// current indentation. +// func stripCommonPrefix(lines []string) { - if len(lines) < 2 { + if len(lines) <= 1 { return // at most one line - nothing to do } - // len(lines) >= 2 + // len(lines) > 1 // The heuristic in this function tries to handle a few // common patterns of /*-style comments: Comments where @@ -479,7 +485,7 @@ func stripCommonPrefix(lines []string) { for i, line := range lines[1 : len(lines)-1] { switch { case isBlank(line): - lines[1+i] = "" // range starts at line 1 + lines[1+i] = "" // range starts with lines[1] case first: prefix = commonPrefix(line, line) first = false @@ -570,9 +576,9 @@ func stripCommonPrefix(lines []string) { } // Remove the common prefix from all but the first and empty lines. - for i, line := range lines[1:] { - if len(line) != 0 { - lines[1+i] = line[len(prefix):] // range starts at line 1 + for i, line := range lines { + if i > 0 && line != "" { + lines[i] = line[len(prefix):] } } } @@ -612,6 +618,19 @@ func (p *printer) writeComment(comment *ast.Comment) { // for /*-style comments, print line by line and let the // write function take care of the proper indentation lines := split(text) + + // The comment started in the first column but is going + // to be indented. For an idempotent result, add indentation + // to all lines such that they look like they were indented + // before - this will make sure the common prefix computation + // is the same independent of how many times formatting is + // applied (was issue 1835). + if pos.IsValid() && pos.Column == 1 && p.indent > 0 { + for i, line := range lines[1:] { + lines[1+i] = " " + line + } + } + stripCommonPrefix(lines) // write comment lines, separated by formfeed, @@ -1140,7 +1159,7 @@ func (p *trimmer) Write(data []byte) (n int, err error) { // ---------------------------------------------------------------------------- // Public interface -// A Mode value is a set of flags (or 0). They coontrol printing. +// A Mode value is a set of flags (or 0). They control printing. type Mode uint const ( diff --git a/src/pkg/go/printer/printer_test.go b/src/pkg/go/printer/printer_test.go index ab9e9b2ec8c..36d1bf74d3f 100644 --- a/src/pkg/go/printer/printer_test.go +++ b/src/pkg/go/printer/printer_test.go @@ -6,7 +6,9 @@ package printer import ( "bytes" + "errors" "flag" + "fmt" "go/ast" "go/parser" "go/token" @@ -25,33 +27,28 @@ var update = flag.Bool("update", false, "update golden files") var fset = token.NewFileSet() -func lineString(text []byte, i int) string { - i0 := i - for i < len(text) && text[i] != '\n' { - i++ - } - return string(text[i0:i]) -} - type checkMode uint const ( export checkMode = 1 << iota rawFormat + idempotent ) -func runcheck(t *testing.T, source, golden string, mode checkMode) { - // parse source - prog, err := parser.ParseFile(fset, source, nil, parser.ParseComments) +// format parses src, prints the corresponding AST, verifies the resulting +// src is syntactically correct, and returns the resulting src or an error +// if any. +func format(src []byte, mode checkMode) ([]byte, error) { + // parse src + f, err := parser.ParseFile(fset, "", src, parser.ParseComments) if err != nil { - t.Error(err) - return + return nil, fmt.Errorf("parse: %s\n%s", err, src) } // filter exports if necessary if mode&export != 0 { - ast.FileExports(prog) // ignore result - prog.Comments = nil // don't print comments that are not in AST + ast.FileExports(f) // ignore result + f.Comments = nil // don't print comments that are not in AST } // determine printer configuration @@ -60,17 +57,72 @@ func runcheck(t *testing.T, source, golden string, mode checkMode) { cfg.Mode |= RawFormat } - // format source + // print AST var buf bytes.Buffer - if err := cfg.Fprint(&buf, fset, prog); err != nil { - t.Error(err) + if err := cfg.Fprint(&buf, fset, f); err != nil { + return nil, fmt.Errorf("print: %s", err) } - res := buf.Bytes() - // formatted source must be valid + // make sure formated output is syntactically correct + res := buf.Bytes() if _, err := parser.ParseFile(fset, "", res, 0); err != nil { + return nil, fmt.Errorf("re-parse: %s\n%s", err, buf.Bytes()) + } + + return res, nil +} + +// lineAt returns the line in text starting at offset offs. +func lineAt(text []byte, offs int) []byte { + i := offs + for i < len(text) && text[i] != '\n' { + i++ + } + return text[offs:i] +} + +// diff compares a and b. +func diff(aname, bname string, a, b []byte) error { + var buf bytes.Buffer // holding long error message + + // compare lengths + if len(a) != len(b) { + fmt.Fprintf(&buf, "\nlength changed: len(%s) = %d, len(%s) = %d", aname, len(a), bname, len(b)) + } + + // compare contents + line := 1 + offs := 1 + for i := 0; i < len(a) && i < len(b); i++ { + ch := a[i] + if ch != b[i] { + fmt.Fprintf(&buf, "\n%s:%d:%d: %s", aname, line, i-offs+1, lineAt(a, offs)) + fmt.Fprintf(&buf, "\n%s:%d:%d: %s", bname, line, i-offs+1, lineAt(b, offs)) + fmt.Fprintf(&buf, "\n\n") + break + } + if ch == '\n' { + line++ + offs = i + 1 + } + } + + if buf.Len() > 0 { + return errors.New(buf.String()) + } + return nil +} + +func runcheck(t *testing.T, source, golden string, mode checkMode) { + src, err := ioutil.ReadFile(source) + if err != nil { + t.Error(err) + return + } + + res, err := format(src, mode) + if err != nil { t.Error(err) - t.Logf("\n%s", res) return } @@ -89,23 +141,19 @@ func runcheck(t *testing.T, source, golden string, mode checkMode) { return } - // compare lengths - if len(res) != len(gld) { - t.Errorf("len = %d, expected %d (= len(%s))", len(res), len(gld), golden) + // formatted source and golden must be the same + if err := diff(source, golden, res, gld); err != nil { + t.Error(err) + return } - // compare contents - for i, line, offs := 0, 1, 0; i < len(res) && i < len(gld); i++ { - ch := res[i] - if ch != gld[i] { - t.Errorf("%s:%d:%d: %s", source, line, i-offs+1, lineString(res, offs)) - t.Errorf("%s:%d:%d: %s", golden, line, i-offs+1, lineString(gld, offs)) - t.Error() - return - } - if ch == '\n' { - line++ - offs = i + 1 + if mode&idempotent != 0 { + // formatting golden must be idempotent + // (This is very difficult to achieve in general and for now + // it is only checked for files explicitly marked as such.) + res, err = format(gld, mode) + if err := diff(golden, fmt.Sprintf("format(%s)", golden), gld, res); err != nil { + t.Errorf("golden is not idempotent: %s", err) } } } @@ -142,15 +190,16 @@ type entry struct { // Use go test -update to create/update the respective golden files. var data = []entry{ - {"empty.input", "empty.golden", 0}, + {"empty.input", "empty.golden", idempotent}, {"comments.input", "comments.golden", 0}, {"comments.input", "comments.x", export}, - {"linebreaks.input", "linebreaks.golden", 0}, - {"expressions.input", "expressions.golden", 0}, - {"expressions.input", "expressions.raw", rawFormat}, + {"comments2.input", "comments2.golden", idempotent}, + {"linebreaks.input", "linebreaks.golden", idempotent}, + {"expressions.input", "expressions.golden", idempotent}, + {"expressions.input", "expressions.raw", rawFormat | idempotent}, {"declarations.input", "declarations.golden", 0}, {"statements.input", "statements.golden", 0}, - {"slow.input", "slow.golden", 0}, + {"slow.input", "slow.golden", idempotent}, } func TestFiles(t *testing.T) { @@ -248,7 +297,7 @@ func testComment(t *testing.T, f *ast.File, srclen int, comment *ast.Comment) { } } -// Verify that the printer produces always produces a correct program +// Verify that the printer produces a correct program // even if the position information of comments introducing newlines // is incorrect. func TestBadComments(t *testing.T) { @@ -421,21 +470,8 @@ func TestX(t *testing.T) { package p func _() {} ` - // parse original - f, err := parser.ParseFile(fset, "src", src, parser.ParseComments) + _, err := format([]byte(src), 0) if err != nil { - t.Fatal(err) + t.Error(err) } - - // pretty-print original - var buf bytes.Buffer - if err = (&Config{Mode: UseSpaces, Tabwidth: 8}).Fprint(&buf, fset, f); err != nil { - t.Fatal(err) - } - - // parse pretty printed original - if _, err := parser.ParseFile(fset, "", buf.Bytes(), 0); err != nil { - t.Fatalf("%s\n%s", err, buf.Bytes()) - } - } diff --git a/src/pkg/go/printer/testdata/comments2.golden b/src/pkg/go/printer/testdata/comments2.golden new file mode 100644 index 00000000000..d3b50bf3e05 --- /dev/null +++ b/src/pkg/go/printer/testdata/comments2.golden @@ -0,0 +1,79 @@ +// Copyright 2012 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. + +// This is a package for testing comment placement by go/printer. +// +package main + +// Test cases for idempotent comment formatting (was issue 1835). +/* +c1a +*/ +/* + c1b +*/ +/* foo +c1c +*/ +/* foo + c1d +*/ +/* +c1e +foo */ +/* + c1f + foo */ + +func f() { + /* + c2a + */ + /* + c2b + */ + /* foo + c2c + */ + /* foo + c2d + */ + /* + c2e + foo */ + /* + c2f + foo */ +} + +func g() { + /* + c3a + */ + /* + c3b + */ + /* foo + c3c + */ + /* foo + c3d + */ + /* + c3e + foo */ + /* + c3f + foo */ +} + +// Test case taken literally from issue 1835. +func main() { + /* + prints test 5 times + */ + for i := 0; i < 5; i++ { + println("test") + } +} diff --git a/src/pkg/go/printer/testdata/comments2.input b/src/pkg/go/printer/testdata/comments2.input new file mode 100644 index 00000000000..6f8c85c94aa --- /dev/null +++ b/src/pkg/go/printer/testdata/comments2.input @@ -0,0 +1,79 @@ +// Copyright 2012 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. + +// This is a package for testing comment placement by go/printer. +// +package main + +// Test cases for idempotent comment formatting (was issue 1835). +/* +c1a +*/ +/* + c1b +*/ +/* foo +c1c +*/ +/* foo + c1d +*/ +/* +c1e +foo */ +/* + c1f + foo */ + +func f() { +/* +c2a +*/ +/* + c2b +*/ +/* foo +c2c +*/ +/* foo + c2d +*/ +/* +c2e +foo */ +/* + c2f + foo */ +} + +func g() { +/* +c3a +*/ +/* + c3b +*/ +/* foo +c3c +*/ +/* foo + c3d +*/ +/* +c3e +foo */ +/* + c3f + foo */ +} + +// Test case taken literally from issue 1835. +func main() { +/* +prints test 5 times +*/ + for i := 0; i < 5; i++ { + println("test") + } +} \ No newline at end of file