Repository: mvdan/gofumpt
Pull request #196 (2022-02-20 13:16:09)

sync gofmt.go with Go 1.18rc1
(see commit message)




Patch:
diff --git a/README.md b/README.md
index 5b83645..46c93e9 100644
--- a/README.md
+++ b/README.md
@@ -2,11 +2,13 @@
 
 	go install mvdan.cc/gofumpt@latest
 
-Enforce a stricter format than `gofmt`, while being backwards compatible. That
-is, `gofumpt` is happy with a subset of the formats that `gofmt` is happy with.
+Enforce a stricter format than `gofmt`, while being backwards compatible.
+That is, `gofumpt` is happy with a subset of the formats that `gofmt` is happy with.
 
-The tool is a modified fork of `gofmt`, so it can be used as a drop-in
-replacement. Running `gofmt` after `gofumpt` should be a no-op. For example:
+The tool is a modified fork of `gofmt` as of Go 1.18rc1.
+It can be used as a drop-in replacement to format your Go code,
+and running `gofmt` after `gofumpt` should produce no changes.
+For example:
 
 	gofumpt -l -w .
 
@@ -385,7 +387,7 @@ var _ = map[string]string{
 
 </details>
 
-Interfaces should not have leading or trailing empty lines
+Field lists should not have leading or trailing empty lines
 
 <details><summary><i>example</i></summary>
 
@@ -396,6 +398,12 @@ type Person interface {
 
 	Age() int
 
+}
+
+type ZeroFields struct {
+
+	// No fields are needed here.
+
 }
 ```
 
@@ -405,6 +413,10 @@ type Person interface {
 
 	Age() int
 }
+
+type ZeroFields struct {
+	// No fields are needed here.
+}
 ```
 
 </details>
diff --git a/go.mod b/go.mod
index 4a5aa21..1d67bb0 100644
--- a/go.mod
+++ b/go.mod
@@ -7,6 +7,7 @@ require (
 	github.com/google/go-cmp v0.5.6
 	github.com/rogpeppe/go-internal v1.8.1
 	golang.org/x/mod v0.5.1
+	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
 	golang.org/x/sys v0.0.0-20211213223007-03aa0b5f6827
 	golang.org/x/tools v0.1.8
 )
diff --git a/go.sum b/go.sum
index 9da648a..b131430 100644
--- a/go.sum
+++ b/go.sum
@@ -24,6 +24,7 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn
 golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
 golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
diff --git a/gofmt.go b/gofmt.go
index 10ad6cb..309a44f 100644
--- a/gofmt.go
+++ b/gofmt.go
@@ -6,6 +6,7 @@ package main
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"flag"
 	"fmt"
@@ -23,6 +24,7 @@ import (
 	"runtime/pprof"
 	"strings"
 
+	"golang.org/x/sync/semaphore"
 	exec "golang.org/x/sys/execabs"
 
 	gformat "mvdan.cc/gofumpt/format"
@@ -56,26 +58,30 @@ const (
 	printerMode = printer.UseSpaces | printer.TabIndent | printerNormalizeNumbers
 
 	// printerNormalizeNumbers means to canonicalize number literal prefixes
-	// and exponents while printing. See https://golang.org/doc/go1.13#gofumpt.
+	// and exponents while printing. See https://golang.org/doc/go1.13#gofmt.
 	//
-	// This value is defined in go/printer specifically for go/format and cmd/gofumpt.
+	// This value is defined in go/printer specifically for go/format and cmd/gofmt.
 	printerNormalizeNumbers = 1 << 30
 )
 
+// fdSem guards the number of concurrently-open file descriptors.
+//
+// For now, this is arbitrarily set to 200, based on the observation that many
+// platforms default to a kernel limit of 256. Ideally, perhaps we should derive
+// it from rlimit on platforms that support that system call.
+//
+// File descriptors opened from outside of this package are not tracked,
+// so this limit may be approximate.
+var fdSem = make(chan bool, 200)
+
 var (
 	fileSet    = token.NewFileSet() // per process FileSet
-	exitCode   = 0
 	parserMode parser.Mode
 
 	// walkingVendorDir is true if we are explicitly walking a vendor directory.
 	walkingVendorDir bool
 )
 
-func report(err error) {
-	scanner.PrintError(os.Stderr, err)
-	exitCode = 2
-}
-
 func usage() {
 	fmt.Fprintf(os.Stderr, `usage: gofumpt [flags] [path ...]
 	-version  show version and exit
@@ -101,7 +107,7 @@ func initParserMode() {
 func isGoFile(f fs.DirEntry) bool {
 	// ignore non-Go files
 	name := f.Name()
-	return !f.IsDir() && !strings.HasPrefix(name, ".") && strings.HasSuffix(name, ".go")
+	return !strings.HasPrefix(name, ".") && strings.HasSuffix(name, ".go") && !f.IsDir()
 }
 
 var rxCodeGenerated = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.$`)
@@ -120,29 +126,157 @@ func isGenerated(file *ast.File) bool {
 	return false
 }
 
-// If in == nil, the source is the contents of the file with the given filename.
-func processFile(filename string, in io.Reader, out io.Writer, stdin, explicit bool) error {
-	var perm os.FileMode = 0o644
-	if in == nil {
-		f, err := os.Open(filename)
-		if err != nil {
-			return err
-		}
-		defer f.Close()
-		fi, err := f.Stat()
-		if err != nil {
-			return err
+// A sequencer performs concurrent tasks that may write output, but emits that
+// output in a deterministic order.
+type sequencer struct {
+	maxWeight int64
+	sem       *semaphore.Weighted   // weighted by input bytes (an approximate proxy for memory overhead)
+	prev      <-chan *reporterState // 1-buffered
+}
+
+// newSequencer returns a sequencer that allows concurrent tasks up to maxWeight
+// and writes tasks' output to out and err.
+func newSequencer(maxWeight int64, out, err io.Writer) *sequencer {
+	sem := semaphore.NewWeighted(maxWeight)
+	prev := make(chan *reporterState, 1)
+	prev <- &reporterState{out: out, err: err}
+	return &sequencer{
+		maxWeight: maxWeight,
+		sem:       sem,
+		prev:      prev,
+	}
+}
+
+// exclusive is a weight that can be passed to a sequencer to cause
+// a task to be executed without any other concurrent tasks.
+const exclusive = -1
+
+// Add blocks until the sequencer has enough weight to spare, then adds f as a
+// task to be executed concurrently.
+//
+// If the weight is either negative or larger than the sequencer's maximum
+// weight, Add blocks until all other tasks have completed, then the task
+// executes exclusively (blocking all other calls to Add until it completes).
+//
+// f may run concurrently in a goroutine, but its output to the passed-in
+// reporter will be sequential relative to the other tasks in the sequencer.
+//
+// If f invokes a method on the reporter, execution of that method may block
+// until the previous task has finished. (To maximize concurrency, f should
+// avoid invoking the reporter until it has finished any parallelizable work.)
+//
+// If f returns a non-nil error, that error will be reported after f's output
+// (if any) and will cause a nonzero final exit code.
+func (s *sequencer) Add(weight int64, f func(*reporter) error) {
+	if weight < 0 || weight > s.maxWeight {
+		weight = s.maxWeight
+	}
+	if err := s.sem.Acquire(context.TODO(), weight); err != nil {
+		// Change the task from "execute f" to "report err".
+		weight = 0
+		f = func(*reporter) error { return err }
+	}
+
+	r := &reporter{prev: s.prev}
+	next := make(chan *reporterState, 1)
+	s.prev = next
+
+	// Start f in parallel: it can run until it invokes a method on r, at which
+	// point it will block until the previous task releases the output state.
+	go func() {
+		if err := f(r); err != nil {
+			r.Report(err)
 		}
-		in = f
-		perm = fi.Mode().Perm()
+		next <- r.getState() // Release the next task.
+		s.sem.Release(weight)
+	}()
+}
+
+// AddReport prints an error to s after the output of any previously-added
+// tasks, causing the final exit code to be nonzero.
+func (s *sequencer) AddReport(err error) {
+	s.Add(0, func(*reporter) error { return err })
+}
+
+// GetExitCode waits for all previously-added tasks to complete, then returns an
+// exit code for the sequence suitable for passing to os.Exit.
+func (s *sequencer) GetExitCode() int {
+	c := make(chan int, 1)
+	s.Add(0, func(r *reporter) error {
+		c <- r.ExitCode()
+		return nil
+	})
+	return <-c
+}
+
+// A reporter reports output, warnings, and errors.
+type reporter struct {
+	prev  <-chan *reporterState
+	state *reporterState
+}
+
+// reporterState carries the state of a reporter instance.
+//
+// Only one reporter at a time may have access to a reporterState.
+type reporterState struct {
+	out, err io.Writer
+	exitCode int
+}
+
+// getState blocks until any prior reporters are finished with the reporter
+// state, then returns the state for manipulation.
+func (r *reporter) getState() *reporterState {
+	if r.state == nil {
+		r.state = <-r.prev
+	}
+	return r.state
+}
+
+// Warnf emits a warning message to the reporter's error stream,
+// without changing its exit code.
+func (r *reporter) Warnf(format string, args ...interface{}) {
+	fmt.Fprintf(r.getState().err, format, args...)
+}
+
+// Write emits a slice to the reporter's output stream.
+//
+// Any error is returned to the caller, and does not otherwise affect the
+// reporter's exit code.
+func (r *reporter) Write(p []byte) (int, error) {
+	return r.getState().out.Write(p)
+}
+
+// Report emits a non-nil error to the reporter's error stream,
+// changing its exit code to a nonzero value.
+func (r *reporter) Report(err error) {
+	if err == nil {
+		panic("Report with nil error")
 	}
+	st := r.getState()
+	scanner.PrintError(st.err, err)
+	st.exitCode = 2
+}
 
-	src, err := io.ReadAll(in)
+func (r *reporter) ExitCode() int {
+	return r.getState().exitCode
+}
+
+// If info == nil, we are formatting stdin instead of a file.
+// If in == nil, the source is the contents of the file with the given filename.
+func processFile(filename string, info fs.FileInfo, in io.Reader, r *reporter, explicit bool) error {
+	src, err := readFile(filename, info, in)
 	if err != nil {
 		return err
 	}
 
-	file, sourceAdj, indentAdj, err := parse(fileSet, filename, src, stdin)
+	fileSet := token.NewFileSet()
+	fragmentOk := false
+	if info == nil {
+		// If we are formatting stdin, we accept a program fragment in lieu of a
+		// complete source file.
+		fragmentOk = true
+	}
+	file, sourceAdj, indentAdj, err := parse(fileSet, filename, src, fragmentOk)
 	if err != nil {
 		return err
 	}
@@ -175,15 +309,21 @@ func processFile(filename string, in io.Reader, out io.Writer, stdin, explicit b
 	if !bytes.Equal(src, res) {
 		// formatting has changed
 		if *list {
-			fmt.Fprintln(out, filename)
+			fmt.Fprintln(r, filename)
 		}
 		if *write {
+			if info == nil {
+				panic("-w should not have been allowed with stdin")
+			}
 			// make a temporary backup before overwriting original
+			perm := info.Mode().Perm()
 			bakname, err := backupFile(filename+".", src, perm)
 			if err != nil {
 				return err
 			}
+			fdSem <- true
 			err = os.WriteFile(filename, res, perm)
+			<-fdSem
 			if err != nil {
 				os.Rename(bakname, filename)
 				return err
@@ -198,40 +338,96 @@ func processFile(filename string, in io.Reader, out io.Writer, stdin, explicit b
 			if err != nil {
 				return fmt.Errorf("computing diff: %s", err)
 			}
-			fmt.Fprintf(out, "diff -u %s %s\n", filepath.ToSlash(filename+".orig"), filepath.ToSlash(filename))
-			out.Write(data)
+			fmt.Fprintf(r, "diff -u %s %s\n", filepath.ToSlash(filename+".orig"), filepath.ToSlash(filename))
+			r.Write(data)
 		}
 	}
 
 	if !*list && !*write && !*doDiff {
-		_, err = out.Write(res)
+		_, err = r.Write(res)
 	}
 
 	return err
 }
 
-func visitFile(path string, f fs.DirEntry, err error) error {
-	if !walkingVendorDir && filepath.Base(path) == "vendor" {
-		return filepath.SkipDir
+// readFile reads the contents of filename, described by info.
+// If in is non-nil, readFile reads directly from it.
+// Otherwise, readFile opens and reads the file itself,
+// with the number of concurrently-open files limited by fdSem.
+func readFile(filename string, info fs.FileInfo, in io.Reader) ([]byte, error) {
+	if in == nil {
+		fdSem <- true
+		var err error
+		f, err := os.Open(filename)
+		if err != nil {
+			return nil, err
+		}
+		in = f
+		defer func() {
+			f.Close()
+			<-fdSem
+		}()
 	}
-	if err != nil || !isGoFile(f) {
-		return err
+
+	// Compute the file's size and read its contents with minimal allocations.
+	//
+	// If we have the FileInfo from filepath.WalkDir, use it to make
+	// a buffer of the right size and avoid ReadAll's reallocations.
+	//
+	// If the size is unknown (or bogus, or overflows an int), fall back to
+	// a size-independent ReadAll.
+	size := -1
+	if info != nil && info.Mode().IsRegular() && int64(int(info.Size())) == info.Size() {
+		size = int(info.Size())
 	}
-	if err := processFile(path, nil, os.Stdout, false, false); err != nil {
-		report(err)
+	if size+1 <= 0 {
+		// The file is not known to be regular, so we don't have a reliable size for it.
+		var err error
+		src, err := io.ReadAll(in)
+		if err != nil {
+			return nil, err
+		}
+		return src, nil
+	}
+
+	// We try to read size+1 bytes so that we can detect modifications: if we
+	// read more than size bytes, then the file was modified concurrently.
+	// (If that happens, we could, say, append to src to finish the read, or
+	// proceed with a truncated buffer — but the fact that it changed at all
+	// indicates a possible race with someone editing the file, so we prefer to
+	// stop to avoid corrupting it.)
+	src := make([]byte, size+1)
+	n, err := io.ReadFull(in, src)
+	if err != nil && err != io.ErrUnexpectedEOF {
+		return nil, err
 	}
-	return nil
+	if n < size {
+		return nil, fmt.Errorf("error: size of %s changed during reading (from %d to %d bytes)", filename, size, n)
+	} else if n > size {
+		return nil, fmt.Errorf("error: size of %s changed during reading (from %d to >=%d bytes)", filename, size, len(src))
+	}
+	return src[:n], nil
 }
 
-func main() {
-	// call gofumptMain in a separate function
+func main() { os.Exit(main1()) }
+
+func main1() int {
+	// Arbitrarily limit in-flight work to 2MiB times the number of threads.
+	//
+	// The actual overhead for the parse tree and output will depend on the
+	// specifics of the file, but this at least keeps the footprint of the process
+	// roughly proportional to GOMAXPROCS.
+	maxWeight := (2 << 20) * int64(runtime.GOMAXPROCS(0))
+	s := newSequencer(maxWeight, os.Stdout, os.Stderr)
+
+	// call gofmtMain in a separate function
 	// so that it can use defer and have them
 	// run before the exit.
-	gofumptMain()
-	os.Exit(exitCode)
+	gofmtMain(s)
+	return s.GetExitCode()
 }
 
-func gofumptMain() {
+func gofmtMain(s *sequencer) {
 	// Ensure our parsed files never start with base 1,
 	// to ensure that using token.NoPos+1 will panic.
 	fileSet.AddFile("gofumpt_base.go", 1, 10)
@@ -254,13 +450,16 @@ func gofumptMain() {
 	}
 
 	if *cpuprofile != "" {
+		fdSem <- true
 		f, err := os.Create(*cpuprofile)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "creating cpu profile: %s\n", err)
-			exitCode = 2
+			s.AddReport(fmt.Errorf("creating cpu profile: %s", err))
 			return
 		}
-		defer f.Close()
+		defer func() {
+			f.Close()
+			<-fdSem
+		}()
 		pprof.StartCPUProfile(f)
 		defer pprof.StopCPUProfile()
 	}
@@ -270,35 +469,74 @@ func gofumptMain() {
 	args := flag.Args()
 	if len(args) == 0 {
 		if *write {
-			fmt.Fprintln(os.Stderr, "error: cannot use -w with standard input")
-			exitCode = 2
+			s.AddReport(fmt.Errorf("error: cannot use -w with standard input"))
 			return
 		}
-		if err := processFile("<standard input>", os.Stdin, os.Stdout, true, true); err != nil {
-			report(err)
-		}
+		s.Add(0, func(r *reporter) error {
+			// TODO: test explicit==true
+			return processFile("<standard input>", nil, os.Stdin, r, true)
+		})
 		return
 	}
 
 	for _, arg := range args {
 		switch info, err := os.Stat(arg); {
 		case err != nil:
-			report(err)
+			s.AddReport(err)
 		case !info.IsDir():
 			// Non-directory arguments are always formatted.
-			if err := processFile(arg, nil, os.Stdout, false, true); err != nil {
-				report(err)
-			}
+			arg := arg
+			println(arg)
+			s.Add(fileWeight(arg, info), func(r *reporter) error {
+				println(arg)
+				return processFile(arg, info, nil, r, true)
+			})
 		default:
 			// Directories are walked, ignoring non-Go files.
 			walkingVendorDir = filepath.Base(arg) == "vendor"
-			if err := filepath.WalkDir(arg, visitFile); err != nil {
-				report(err)
+			err := filepath.WalkDir(arg, func(path string, f fs.DirEntry, err error) error {
+				if !walkingVendorDir && filepath.Base(path) == "vendor" {
+					return filepath.SkipDir
+				}
+				if err != nil || !isGoFile(f) {
+					return err
+				}
+				info, err := f.Info()
+				if err != nil {
+					s.AddReport(err)
+					return nil
+				}
+				s.Add(fileWeight(path, info), func(r *reporter) error {
+					return processFile(path, info, nil, r, false)
+				})
+				return nil
+			})
+			if err != nil {
+				s.AddReport(err)
 			}
 		}
 	}
 }
 
+func fileWeight(path string, info fs.FileInfo) int64 {
+	if info == nil {
+		return exclusive
+	}
+	if info.Mode().Type() == fs.ModeSymlink {
+		var err error
+		info, err = os.Stat(path)
+		if err != nil {
+			return exclusive
+		}
+	}
+	if !info.Mode().IsRegular() {
+		// For non-regular files, FileInfo.Size is system-dependent and thus not a
+		// reliable indicator of weight.
+		return exclusive
+	}
+	return info.Size()
+}
+
 func diffWithReplaceTempFile(b1, b2 []byte, filename string) ([]byte, error) {
 	data, err := diff.Diff("gofumpt", b1, b2)
 	if len(data) > 0 {
@@ -342,6 +580,9 @@ const chmodSupported = runtime.GOOS != "windows"
 // with <number randomly chosen such that the file name is unique. backupFile returns
 // the chosen file name.
 func backupFile(filename string, data []byte, perm fs.FileMode) (string, error) {
+	fdSem <- true
+	defer func() { <-fdSem }()
+
 	// create backup file
 	f, err := os.CreateTemp(filepath.Dir(filename), filepath.Base(filename))
 	if err != nil {


Test patch:
diff --git a/main_test.go b/main_test.go
index e5d5bd2..e5f1366 100644
--- a/main_test.go
+++ b/main_test.go
@@ -17,10 +17,7 @@ import (
 
 func TestMain(m *testing.M) {
 	os.Exit(testscript.RunMain(m, map[string]func() int{
-		"gofumpt": func() int {
-			gofumptMain()
-			return exitCode
-		},
+		"gofumpt": main1,
 	}))
 }
 
diff --git a/testdata/scripts/block-empty.txt b/testdata/scripts/block-empty.txt
index bd205fa..435abf3 100644
--- a/testdata/scripts/block-empty.txt
+++ b/testdata/scripts/block-empty.txt
@@ -25,16 +25,16 @@ func f() {
 
 
 		// lone comment
-		
-		
+
+
 	}
 
 	type I interface {
 
 
 		// lone comment
-		
-		
+
+
 	}
 
 
@@ -43,19 +43,16 @@ func f() {
 type SOut struct {
 
 	// lone comment
-	
+
 }
 
 type IOut interface {
 
-	
+
 	// lone comment
 
 
 }
-
-
-
 -- foo.go.golden --
 package p
 
diff --git a/testdata/scripts/generated.txt b/testdata/scripts/generated.txt
index b9b962c..fc49d17 100644
--- a/testdata/scripts/generated.txt
+++ b/testdata/scripts/generated.txt
@@ -1,10 +1,26 @@
+# Explicitly given generated files are formatted with our rules.
 gofumpt foo.go
 cmp stdout foo.go.golden
 
+# stdin is still considered an explicit file.
+stdin foo.go
+gofumpt
+cmp stdout foo.go.golden
+
+# Implicitly walked generated files get formatted without the added rules.
 gofumpt -l .
-! stdout .
+stdout -count=1 '^badgofmt.go$'
+! stdout '^foo.go$'
 ! stderr .
 
+-- badgofmt.go --
+// Code generated by foo. DO NOT EDIT.
+
+package foo
+
+func f() {
+println("body")
+}
 -- foo.go --
 // foo is a package about bar.