Merge branch 'master' into work-with-older-versions

.github/workflows/go.yml

@@ -2,7 +2,7 @@ name: Go
 
 on:
   push:
-    branches: ['*']
+    branches: [master]
     tags: ['v*']
   pull_request:
     branches: ['*']
@@ -19,17 +19,15 @@ jobs:
         go: ["1.20.x", "1.21.x"]
         include:
         - go: 1.21.x
-          latest: true
 
     steps:
     - name: Checkout code
       uses: actions/checkout@v3
 
     - name: Setup Go
-      uses: actions/setup-go@v3
+      uses: actions/setup-go@v4
       with:
         go-version: ${{ matrix.go }}
-        cache: true
         cache-dependency-path: '**/go.sum'
 
     - name: Download Dependencies
@@ -39,16 +37,33 @@ jobs:
         (cd benchmarks && go mod download)
         (cd zapgrpc/internal/test && go mod download)
 
-    - name: Lint
-      if: matrix.latest
-      run: make lint
-
     - name: Test
       run: make cover
 
     - name: Upload coverage to codecov.io
       uses: codecov/codecov-action@v3
 
+  lint:
+    name: Lint
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v3
+      name: Check out repository
+    - uses: actions/setup-go@v4
+      name: Set up Go
+      with:
+        go-version: 1.21.x
+        cache: false  # managed by golangci-lint
+
+    - uses: golangci/golangci-lint-action@v3
+      name: Install golangci-lint
+      with:
+        version: latest
+        args: --version  # make lint will run the linter
+
+    - run: make lint
+      name: Lint
+
     - name: vulncheck
-      if: matrix.latest
       run: make vulncheck

.golangci.yml

@@ -0,0 +1,53 @@
+output:
+  # Make output more digestible with quickfix in vim/emacs/etc.
+  sort-results: true
+  print-issued-lines: false
+
+linters:
+  # We'll track the golangci-lint default linters manually
+  # instead of letting them change without our control.
+  disable-all: true
+  enable:
+    # golangci-lint defaults:
+    # - errcheck TODO: enable errcheck
+    - gosimple
+    - govet
+    - ineffassign
+    - staticcheck
+    - unused
+
+    # Our own extras:
+    - gofmt
+    - nolintlint # lints nolint directives
+    - revive
+
+linter-settings:
+  govet:
+    # These govet checks are disabled by default, but they're useful.
+    enable:
+      - niliness
+      - reflectvaluecompare
+      - sortslice
+      - unusedwrite
+
+issues:
+  # Print all issues reported by all linters.
+  max-issues-per-linter: 0
+  max-same-issues: 0
+
+  # Don't ignore some of the issues that golangci-lint considers okay.
+  # This includes documenting all exported entities.
+  exclude-use-default: false
+
+  exclude-rules:
+    # Don't warn on unused parameters.
+    # Parameter names are useful; replacing them with '_' is undesirable.
+    - linters: [revive]
+      text: 'unused-parameter: parameter \S+ seems to be unused, consider removing or renaming it as _'
+
+    # staticcheck already has smarter checks for empty blocks.
+    # revive's empty-block linter has false positives.
+    # For example, as of writing this, the following is not allowed.
+    #   for foo() { }
+    - linters: [revive]
+      text: 'empty-block: this block is empty, you can remove it'

Makefile

@@ -1,7 +1,9 @@
-export GOBIN ?= $(shell pwd)/bin
+# Directory containing the Makefile.
+PROJECT_ROOT = $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
+
+export GOBIN ?= $(PROJECT_ROOT)/bin
+export PATH := $(GOBIN):$(PATH)
 
-REVIVE = $(GOBIN)/revive
-STATICCHECK = $(GOBIN)/staticcheck
 GOVULNCHECK = $(GOBIN)/govulncheck
 BENCH_FLAGS ?= -cpuprofile=cpu.pprof -memprofile=mem.pprof -benchmem
 
@@ -11,47 +13,40 @@ MODULE_DIRS = . ./exp ./benchmarks ./zapgrpc/internal/test
 # Directories that we want to track coverage for.
 COVER_DIRS = . ./exp
 
-# Many Go tools take file globs or directories as arguments instead of packages.
-GO_FILES := $(shell \
-	find . '(' -path '*/.*' -o -path './vendor' ')' -prune \
-	-o -name '*.go' -print | cut -b3-)
-
 .PHONY: all
 all: lint test
 
 .PHONY: lint
-lint: $(REVIVE) $(STATICCHECK)
-	@rm -rf lint.log
-	@echo "Checking formatting..."
-	@gofmt -d -s $(GO_FILES) 2>&1 | tee lint.log
-	@echo "Checking vet..."
-	@$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go vet ./... 2>&1) &&) true | tee -a lint.log
-	@echo "Checking lint..."
-	@$(foreach dir,$(MODULE_DIRS),(cd $(dir) && \
-		$(REVIVE) -set_exit_status ./... 2>&1) &&) true | tee -a lint.log
-	@echo "Checking staticcheck..."
-	@$(foreach dir,$(MODULE_DIRS),(cd $(dir) && $(STATICCHECK) ./... 2>&1) &&) true | tee -a lint.log
-	@echo "Checking for unresolved FIXMEs..."
-	@git grep -i fixme | grep -v -e Makefile | tee -a lint.log
-	@echo "Checking for license headers..."
-	@./checklicense.sh | tee -a lint.log
-	@[ ! -s lint.log ]
-	@echo "Checking 'go mod tidy'..."
-	@make tidy
-	@if ! git diff --quiet; then \
-		echo "'go mod tidy' resulted in changes or working tree is dirty:"; \
-		git --no-pager diff; \
-	fi
-
-$(REVIVE):
-	cd tools && go install github.com/mgechev/revive
+lint: golangci-lint tidy-lint license-lint
+
+.PHONY: golangci-lint
+golangci-lint:
+	@$(foreach mod,$(MODULE_DIRS), \
+		(cd $(mod) && \
+		echo "[lint] golangci-lint: $(mod)" && \
+		golangci-lint run --path-prefix $(mod)) &&) true
+
+.PHONY: tidy
+tidy:
+	@$(foreach dir,$(MODULE_DIRS), \
+		(cd $(dir) && go mod tidy) &&) true
+
+.PHONY: tidy-lint
+tidy-lint:
+	@$(foreach mod,$(MODULE_DIRS), \
+		(cd $(mod) && \
+		echo "[lint] tidy: $(mod)" && \
+		go mod tidy && \
+		git diff --exit-code -- go.mod go.sum) &&) true
+
+
+.PHONY: license-lint
+license-lint:
+	./checklicense.sh
 
 $(GOVULNCHECK):
 	cd tools && go install golang.org/x/vuln/cmd/govulncheck
 
-$(STATICCHECK):
-	cd tools && go install honnef.co/go/tools/cmd/staticcheck
-
 .PHONY: test
 test:
 	@$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go test -race ./...) &&) true
@@ -76,10 +71,6 @@ updatereadme:
 	rm -f README.md
 	cat .readme.tmpl | go run internal/readme/readme.go > README.md
 
-.PHONY: tidy
-tidy:
-	@$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go mod tidy) &&) true
-
 .PHONY: vulncheck
 vulncheck: $(GOVULNCHECK)
 	$(GOVULNCHECK) ./...

logger.go

@@ -173,7 +173,8 @@ func (log *Logger) WithOptions(opts ...Option) *Logger {
 }
 
 // With creates a child logger and adds structured context to it. Fields added
-// to the child don't affect the parent, and vice versa.
+// to the child don't affect the parent, and vice versa. Any fields that
+// require evaluation (such as Objects) are evaluated upon invocation of With.
 func (log *Logger) With(fields ...Field) *Logger {
 	if len(fields) == 0 {
 		return log
@@ -199,6 +200,8 @@ func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
 
 // Log logs a message at the specified level. The message includes any fields
 // passed at the log site, as well as any fields accumulated on the logger.
+// Any Fields that require  evaluation (such as Objects) are evaluated upon
+// invocation of Log.
 func (log *Logger) Log(lvl zapcore.Level, msg string, fields ...Field) {
 	if ce := log.check(lvl, msg); ce != nil {
 		ce.Write(fields...)
@@ -288,8 +291,8 @@ func (log *Logger) Name() string {
 }
 
 func (log *Logger) clone() *Logger {
-	copy := *log
-	return &copy
+	clone := *log
+	return &clone
 }
 
 func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {

logger_bench_test.go

@@ -23,6 +23,7 @@ package zap
 import (
 	"errors"
 	"runtime"
+	"strconv"
 	"sync"
 	"testing"
 	"time"
@@ -198,6 +199,37 @@ func BenchmarkAddCallerAndStacktrace(b *testing.B) {
 		}
 	})
 }
+func Benchmark5WithsUsed(b *testing.B) {
+	benchmarkWithUsed(b, 5, true)
+}
+
+// This benchmark will be used in future as a
+// baseline for improving
+func Benchmark5WithsNotUsed(b *testing.B) {
+	benchmarkWithUsed(b, 5, false)
+}
+
+func benchmarkWithUsed(b *testing.B, N int, use bool) {
+	keys := make([]string, N)
+	values := make([]string, N)
+	for i := 0; i < N; i++ {
+		keys[i] = "k" + strconv.Itoa(i)
+		values[i] = "v" + strconv.Itoa(i)
+	}
+
+	b.ResetTimer()
+
+	withBenchedLogger(b, func(log *Logger) {
+		for i := 0; i < N; i++ {
+			log = log.With(String(keys[i], values[i]))
+		}
+		if use {
+			log.Info("used")
+			return
+		}
+		runtime.KeepAlive(log)
+	})
+}
 
 func Benchmark10Fields(b *testing.B) {
 	withBenchedLogger(b, func(log *Logger) {

logger_test.go

@@ -130,16 +130,55 @@ func TestLoggerWith(t *testing.T) {
 		// shouldn't stomp on each other's fields or affect the parent's fields.
 		logger.With(String("one", "two")).Info("")
 		logger.With(String("three", "four")).Info("")
+		logger.With(String("five", "six")).With(String("seven", "eight")).Info("")
 		logger.Info("")
 
 		assert.Equal(t, []observer.LoggedEntry{
 			{Context: []Field{Int("foo", 42), String("one", "two")}},
 			{Context: []Field{Int("foo", 42), String("three", "four")}},
+			{Context: []Field{Int("foo", 42), String("five", "six"), String("seven", "eight")}},
 			{Context: []Field{Int("foo", 42)}},
 		}, logs.AllUntimed(), "Unexpected cross-talk between child loggers.")
 	})
 }
 
+func TestLoggerWithCaptures(t *testing.T) {
+	enc := zapcore.NewJSONEncoder(zapcore.EncoderConfig{
+		MessageKey: "m",
+	})
+
+	var bs ztest.Buffer
+	logger := New(zapcore.NewCore(enc, &bs, DebugLevel))
+
+	x := 0
+	arr := zapcore.ArrayMarshalerFunc(func(enc zapcore.ArrayEncoder) error {
+		enc.AppendInt(x)
+		return nil
+	})
+
+	// Demonstrate the arguments are captured when With() and Info() are invoked.
+	logger = logger.With(Array("a", arr))
+	x = 1
+	logger.Info("hello", Array("b", arr))
+	x = 2
+	logger = logger.With(Array("c", arr))
+	logger.Info("world")
+
+	if lines := bs.Lines(); assert.Len(t, lines, 2) {
+		assert.JSONEq(t, `{
+			"m": "hello",
+			"a": [0],
+			"b": [1]
+		}`, lines[0], "Unexpected output from first log.")
+
+		assert.JSONEq(t, `{
+			"m": "world",
+			"a": [0],
+			"c": [2]
+		}`, lines[1], "Unexpected output from second log.")
+	}
+}
+
 func TestLoggerLogPanic(t *testing.T) {
 	for _, tt := range []struct {
 		do       func(*Logger)

sink_test.go

@@ -68,13 +68,12 @@ func TestRegisterSink(t *testing.T) {
 	require.NoError(t, RegisterSink(strings.ToUpper(memScheme), memFactory), "Failed to register scheme %q.", memScheme)
 	require.NoError(t, RegisterSink(nopScheme, nopFactory), "Failed to register scheme %q.", nopScheme)
 
-	sink, close, err := Open(
+	sink, closeSink, err := Open(
 		memScheme+"://somewhere",
 		nopScheme+"://somewhere-else",
 	)
 	require.NoError(t, err, "Unexpected error opening URLs with registered schemes.")
-
-	defer close()
+	defer closeSink()
 
 	assert.Equal(t, 1, memCalls, "Unexpected number of calls to memory factory.")
 	assert.Equal(t, 1, nopCalls, "Unexpected number of calls to no-op factory.")

tools/go.mod

@@ -1,28 +1,12 @@
 module go.uber.org/zap/tools
 
-require (
-	github.com/mgechev/revive v1.2.5
-	golang.org/x/vuln v1.0.0
-	honnef.co/go/tools v0.4.4
-)
+require golang.org/x/vuln v1.0.0
 
 require (
-	github.com/BurntSushi/toml v1.2.1 // indirect
-	github.com/chavacava/garif v0.0.0-20221024190013-b3ef35877348 // indirect
-	github.com/fatih/color v1.14.1 // indirect
-	github.com/fatih/structtag v1.2.0 // indirect
-	github.com/mattn/go-colorable v0.1.13 // indirect
-	github.com/mattn/go-isatty v0.0.17 // indirect
-	github.com/mattn/go-runewidth v0.0.9 // indirect
-	github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517 // indirect
-	github.com/mitchellh/go-homedir v1.1.0 // indirect
-	github.com/olekukonko/tablewriter v0.0.5 // indirect
-	github.com/pkg/errors v0.9.1 // indirect
-	golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect
 	golang.org/x/mod v0.10.0 // indirect
-	golang.org/x/sync v0.2.0 // indirect
-	golang.org/x/sys v0.8.0 // indirect
-	golang.org/x/tools v0.9.4-0.20230601214343-86c93e8732cc // indirect
+	golang.org/x/sync v0.1.0 // indirect
+	golang.org/x/sys v0.7.0 // indirect
+	golang.org/x/tools v0.8.1-0.20230421161920-b9619ee54b47 // indirect
 )
 
 go 1.20