internal/lsp: make sure that `gofmt -s` analyses don't modify AST
The code for `gofmt -s` directly modifies the AST, since the ASTs are
not long-lived. Some of this code made it into our analysis
implementations, causing very strange bugs to manifest. Added a
regression test for this specific case.
Fixes golang/go#38267
Change-Id: I235620adcbf2bbc7027c6d83ff2c7fe74729062e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227299
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
(cherry picked from commit 1fd976651f4da6cc8c239c50463fb2d82641fa77)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227354
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/internal/lsp/analysis/simplifycompositelit/simplifycompositelit.go b/internal/lsp/analysis/simplifycompositelit/simplifycompositelit.go
index 68b69ee..eae53db 100644
--- a/internal/lsp/analysis/simplifycompositelit/simplifycompositelit.go
+++ b/internal/lsp/analysis/simplifycompositelit/simplifycompositelit.go
@@ -62,33 +62,28 @@
ktyp = reflect.ValueOf(keyType)
}
typ := reflect.ValueOf(eltType)
- for i, x := range outer.Elts {
- px := &outer.Elts[i]
+ for _, x := range outer.Elts {
// look at value of indexed/named elements
if t, ok := x.(*ast.KeyValueExpr); ok {
if keyType != nil {
- simplifyLiteral(pass, ktyp, keyType, t.Key, &t.Key)
+ simplifyLiteral(pass, ktyp, keyType, t.Key)
}
x = t.Value
- px = &t.Value
}
- simplifyLiteral(pass, typ, eltType, x, px)
+ simplifyLiteral(pass, typ, eltType, x)
}
})
return nil, nil
}
-func simplifyLiteral(pass *analysis.Pass, typ reflect.Value, astType, x ast.Expr, px *ast.Expr) {
+func simplifyLiteral(pass *analysis.Pass, typ reflect.Value, astType, x ast.Expr) {
// if the element is a composite literal and its literal type
// matches the outer literal's element type exactly, the inner
// literal type may be omitted
if inner, ok := x.(*ast.CompositeLit); ok && match(typ, reflect.ValueOf(inner.Type)) {
- old := inner.Type
- inner.Type = nil
var b bytes.Buffer
- printer.Fprint(&b, pass.Fset, old)
- createDiagnostic(pass, old.Pos(), old.End(), b.String())
- inner.Type = old
+ printer.Fprint(&b, pass.Fset, inner.Type)
+ createDiagnostic(pass, inner.Type.Pos(), inner.Type.End(), b.String())
}
// if the outer literal's element type is a pointer type *T
// and the element is & of a composite literal of type T,
@@ -97,15 +92,10 @@
if addr, ok := x.(*ast.UnaryExpr); ok && addr.Op == token.AND {
if inner, ok := addr.X.(*ast.CompositeLit); ok {
if match(reflect.ValueOf(ptr.X), reflect.ValueOf(inner.Type)) {
- old := inner.Type
- inner.Type = nil // drop T
- *px = inner // drop &
var b bytes.Buffer
- printer.Fprint(&b, pass.Fset, old)
- // Do old.Pos()-1 to account for the &.
- createDiagnostic(pass, old.Pos()-1, old.End(), "&"+b.String())
- inner.Type = old
- *px = inner // add &
+ printer.Fprint(&b, pass.Fset, inner.Type)
+ // Account for the & by subtracting 1 from typ.Pos().
+ createDiagnostic(pass, inner.Type.Pos()-1, inner.Type.End(), "&"+b.String())
}
}
}
diff --git a/internal/lsp/analysis/simplifyrange/simplifyrange.go b/internal/lsp/analysis/simplifyrange/simplifyrange.go
index e53f50d..c9cb387 100644
--- a/internal/lsp/analysis/simplifyrange/simplifyrange.go
+++ b/internal/lsp/analysis/simplifyrange/simplifyrange.go
@@ -45,24 +45,26 @@
(*ast.RangeStmt)(nil),
}
inspect.Preorder(nodeFilter, func(n ast.Node) {
- stmt := n.(*ast.RangeStmt)
- end := newlineIndex(pass.Fset, stmt)
- var old ast.Expr
+ var copy *ast.RangeStmt
+ if stmt, ok := n.(*ast.RangeStmt); ok {
+ x := *stmt
+ copy = &x
+ }
+ if copy == nil {
+ return
+ }
+ end := newlineIndex(pass.Fset, copy)
+
// Range statements of the form: for i, _ := range x {}
- if isBlank(stmt.Value) {
- old = stmt.Value
- defer func() {
- stmt.Value = old
- }()
- stmt.Value = nil
+ var old ast.Expr
+ if isBlank(copy.Value) {
+ old = copy.Value
+ copy.Value = nil
}
// Range statements of the form: for _ := range x {}
- if isBlank(stmt.Key) && stmt.Value == nil {
- old = stmt.Key
- defer func() {
- stmt.Key = old
- }()
- stmt.Key = nil
+ if isBlank(copy.Key) && copy.Value == nil {
+ old = copy.Key
+ copy.Key = nil
}
// Return early if neither if condition is met.
if old == nil {
@@ -72,7 +74,7 @@
Pos: old.Pos(),
End: old.End(),
Message: "simplify range expression",
- SuggestedFixes: suggestedFixes(pass.Fset, stmt, end),
+ SuggestedFixes: suggestedFixes(pass.Fset, copy, end),
})
})
return nil, nil
diff --git a/internal/lsp/analysis/simplifyslice/simplifyslice.go b/internal/lsp/analysis/simplifyslice/simplifyslice.go
index 01a6bc3..da1728e 100644
--- a/internal/lsp/analysis/simplifyslice/simplifyslice.go
+++ b/internal/lsp/analysis/simplifyslice/simplifyslice.go
@@ -74,23 +74,21 @@
if !ok || arg.Obj != s.Obj {
return
}
- old := expr.High
var b bytes.Buffer
- printer.Fprint(&b, pass.Fset, old)
+ printer.Fprint(&b, pass.Fset, expr.High)
pass.Report(analysis.Diagnostic{
- Pos: old.Pos(),
- End: old.End(),
+ Pos: expr.High.Pos(),
+ End: expr.High.End(),
Message: fmt.Sprintf("unneeded: %s", b.String()),
SuggestedFixes: []analysis.SuggestedFix{{
Message: fmt.Sprintf("Remove '%s'", b.String()),
TextEdits: []analysis.TextEdit{{
- Pos: old.Pos(),
- End: old.End(),
+ Pos: expr.High.Pos(),
+ End: expr.High.End(),
NewText: []byte{},
}},
}},
})
- expr.High = old
})
return nil, nil
}
diff --git a/internal/lsp/regtest/diagnostics_test.go b/internal/lsp/regtest/diagnostics_test.go
index 4739e04..b4a5867 100644
--- a/internal/lsp/regtest/diagnostics_test.go
+++ b/internal/lsp/regtest/diagnostics_test.go
@@ -194,3 +194,50 @@
})
})
}
+
+const testPackage = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- lib.go --
+package lib
+
+func Hello(x string) {
+ _ = x
+}
+-- lib_test.go --
+package lib
+
+import "testing"
+
+type testStruct struct{
+ name string
+}
+
+func TestHello(t *testing.T) {
+ testStructs := []*testStruct{
+ &testStruct{"hello"},
+ &testStruct{"goodbye"},
+ }
+ for y := range testStructs {
+ _ = y
+ }
+}
+`
+
+func Test_Issue38267(t *testing.T) {
+ runner.Run(t, testPackage, func(env *Env) {
+ env.OpenFile("lib_test.go")
+ env.Await(
+ DiagnosticAt("lib_test.go", 10, 2),
+ DiagnosticAt("lib_test.go", 11, 2),
+ )
+ env.OpenFile("lib.go")
+ env.RegexpReplace("lib.go", "_ = x", "var y int")
+ env.Await(
+ env.DiagnosticAtRegexp("lib.go", "y int"),
+ EmptyDiagnostics("lib_test.go"),
+ )
+ })
+}
