Suzy Mueller has uploaded this change for review.
go/analysis: add suggested fix for unkeyed composite literals
Include a suggested fix with the diagnostic for unkeyed composite
literals. This suggested fix will add the name of each of the
fields.
For golang/go#53062
Change-Id: I0c33191ff3cf66c95a9a055848274cc2b0c38224
---
M go/analysis/passes/composite/composite.go
M go/analysis/passes/composite/composite_test.go
A go/analysis/passes/composite/testdata/src/a/a.go.golden
A go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
M go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
A go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
6 files changed, 230 insertions(+), 10 deletions(-)
diff --git a/go/analysis/passes/composite/composite.go b/go/analysis/passes/composite/composite.go
index d3670ac..53a4b04 100644
--- a/go/analysis/passes/composite/composite.go
+++ b/go/analysis/passes/composite/composite.go
@@ -7,6 +7,7 @@
package composite
import (
+ "fmt"
"go/ast"
"go/types"
"strings"
@@ -83,7 +84,8 @@
}
for _, typ := range structuralTypes {
under := deref(typ.Underlying())
- if _, ok := under.(*types.Struct); !ok {
+ strct, ok := under.(*types.Struct)
+ if !ok {
// skip non-struct composite literals
continue
}
@@ -94,10 +96,24 @@
// check if the CompositeLit contains an unkeyed field
allKeyValue := true
- for _, e := range cl.Elts {
+ var suggestedFixAvailable = true
+ var missingKeys []analysis.TextEdit
+ for i, e := range cl.Elts {
if _, ok := e.(*ast.KeyValueExpr); !ok {
allKeyValue = false
- break
+ field := strct.Field(i)
+ if field == nil {
+ // If there is something wrong with the field we want to add,
+ // still send the diagnostic but don't include the suggested
+ // fix.
+ suggestedFixAvailable = false
+ break
+ }
+ missingKeys = append(missingKeys, analysis.TextEdit{
+ Pos: e.Pos(),
+ End: e.Pos(),
+ NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
+ })
}
}
if allKeyValue {
@@ -105,7 +121,26 @@
continue
}
- pass.ReportRangef(cl, "%s composite literal uses unkeyed fields", typeName)
+ diag := analysis.Diagnostic{
+ Pos: cl.Pos(),
+ End: cl.End(),
+ Message: fmt.Sprintf("%s composite literal uses unkeyed fields", typeName),
+ }
+ if suggestedFixAvailable {
+ diag.SuggestedFixes = []analysis.SuggestedFix{{
+ Message: "Add composite literal keys",
+ TextEdits: missingKeys,
+ }}
+ }
+ pass.Report(analysis.Diagnostic{
+ Pos: cl.Pos(),
+ End: cl.End(),
+ Message: fmt.Sprintf("%s composite literal uses unkeyed fields", typeName),
+ SuggestedFixes: []analysis.SuggestedFix{{
+ Message: "Add composite literal keys",
+ TextEdits: missingKeys,
+ }},
+ })
return
}
})
diff --git a/go/analysis/passes/composite/composite_test.go b/go/analysis/passes/composite/composite_test.go
index 952de8b..7afaaa7 100644
--- a/go/analysis/passes/composite/composite_test.go
+++ b/go/analysis/passes/composite/composite_test.go
@@ -18,5 +18,5 @@
if typeparams.Enabled {
pkgs = append(pkgs, "typeparams")
}
- analysistest.Run(t, testdata, composite.Analyzer, pkgs...)
+ analysistest.RunWithSuggestedFixes(t, testdata, composite.Analyzer, pkgs...)
}
diff --git a/go/analysis/passes/composite/testdata/src/a/a.go.golden b/go/analysis/passes/composite/testdata/src/a/a.go.golden
new file mode 100644
index 0000000..cc13ab4
--- /dev/null
+++ b/go/analysis/passes/composite/testdata/src/a/a.go.golden
@@ -0,0 +1,127 @@
+// 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 file contains the test for untagged struct literals.
+
+package a
+
+import (
+ "flag"
+ "go/scanner"
+ "go/token"
+ "image"
+ "unicode"
+)
+
+var Okay1 = []string{
+ "Name",
+ "Usage",
+ "DefValue",
+}
+
+var Okay2 = map[string]bool{
+ "Name": true,
+ "Usage": true,
+ "DefValue": true,
+}
+
+var Okay3 = struct {
+ X string
+ Y string
+ Z string
+}{
+ "Name",
+ "Usage",
+ "DefValue",
+}
+
+var Okay4 = []struct {
+ A int
+ B int
+}{
+ {1, 2},
+ {3, 4},
+}
+
+type MyStruct struct {
+ X string
+ Y string
+ Z string
+}
+
+var Okay5 = &MyStruct{
+ "Name",
+ "Usage",
+ "DefValue",
+}
+
+var Okay6 = []MyStruct{
+ {"foo", "bar", "baz"},
+ {"aa", "bb", "cc"},
+}
+
+var Okay7 = []*MyStruct{
+ {"foo", "bar", "baz"},
+ {"aa", "bb", "cc"},
+}
+
+// Testing is awkward because we need to reference things from a separate package
+// to trigger the warnings.
+
+var goodStructLiteral = flag.Flag{
+ Name: "Name",
+ Usage: "Usage",
+}
+var badStructLiteral = flag.Flag{ // want "unkeyed fields"
+ Name: "Name",
+ Usage: "Usage",
+ Value: nil, // Value
+ DefValue: "DefValue",
+}
+
+var delta [3]rune
+
+// SpecialCase is a named slice of CaseRange to test issue 9171.
+var goodNamedSliceLiteral = unicode.SpecialCase{
+ {Lo: 1, Hi: 2, Delta: delta},
+ unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta},
+}
+var badNamedSliceLiteral = unicode.SpecialCase{
+ {Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+ unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+}
+
+// ErrorList is a named slice, so no warnings should be emitted.
+var goodScannerErrorList = scanner.ErrorList{
+ &scanner.Error{Msg: "foobar"},
+}
+var badScannerErrorList = scanner.ErrorList{
+ &scanner.Error{Pos: token.Position{}, Msg: "foobar"}, // want "unkeyed fields"
+}
+
+// Check whitelisted structs: if vet is run with --compositewhitelist=false,
+// this line triggers an error.
+var whitelistedPoint = image.Point{1, 2}
+
+// Do not check type from unknown package.
+// See issue 15408.
+var unknownPkgVar = unicode.NoSuchType{"foo", "bar"}
+
+// A named pointer slice of CaseRange to test issue 23539. In
+// particular, we're interested in how some slice elements omit their
+// type.
+var goodNamedPointerSliceLiteral = []*unicode.CaseRange{
+ {Lo: 1, Hi: 2},
+ &unicode.CaseRange{Lo: 1, Hi: 2},
+}
+var badNamedPointerSliceLiteral = []*unicode.CaseRange{
+ {Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+ &unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+}
+
+// unicode.Range16 is whitelisted, so there'll be no vet error
+var range16 = unicode.Range16{0xfdd0, 0xfdef, 1}
+
+// unicode.Range32 is whitelisted, so there'll be no vet error
+var range32 = unicode.Range32{0x1fffe, 0x1ffff, 1}
diff --git a/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden b/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
new file mode 100644
index 0000000..20b652e
--- /dev/null
+++ b/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
@@ -0,0 +1,16 @@
+// Copyright 2022 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.
+
+//go:build go1.18
+// +build go1.18
+
+package a
+
+import "testing"
+
+var fuzzTargets = []testing.InternalFuzzTarget{
+ {"Fuzz", Fuzz},
+}
+
+func Fuzz(f *testing.F) {}
diff --git a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
index dd5d57e..f9a5e1f 100644
--- a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
+++ b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
@@ -6,7 +6,7 @@
import "typeparams/lib"
-type localStruct struct { F int }
+type localStruct struct{ F int }
func F[
T1 ~struct{ f int },
@@ -20,8 +20,8 @@
_ = T1{2}
_ = T2a{2}
_ = T2b{2} // want "unkeyed fields"
- _ = T3{1,2}
- _ = T4{1,2}
- _ = T5{1:2}
- _ = T6{1:2}
+ _ = T3{1, 2}
+ _ = T4{1, 2}
+ _ = T5{1: 2}
+ _ = T6{1: 2}
}
diff --git a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
new file mode 100644
index 0000000..66cd915
--- /dev/null
+++ b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
@@ -0,0 +1,27 @@
+// Copyright 2021 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.
+
+package typeparams
+
+import "typeparams/lib"
+
+type localStruct struct{ F int }
+
+func F[
+ T1 ~struct{ f int },
+ T2a localStruct,
+ T2b lib.Struct,
+ T3 ~[]int,
+ T4 lib.Slice,
+ T5 ~map[int]int,
+ T6 lib.Map,
+]() {
+ _ = T1{2}
+ _ = T2a{2}
+ _ = T2b{F: 2} // want "unkeyed fields"
+ _ = T3{1, 2}
+ _ = T4{1, 2}
+ _ = T5{1: 2}
+ _ = T6{1: 2}
+}
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/33756279-3b10-4131-a7ec-457b42ebdc3c
Patch set 1:gopls-CI +1
Attention is currently required from: Suzy Mueller.
6 comments:
Patchset:
Nice feature!
File go/analysis/passes/composite/composite.go:
Patch Set #1, Line 104: field := strct.Field(i)
Analyzers with RunDespiteErrors=true must be very defensive. Here specifically, it's possible that the struct literal has more entries than the struct has fields. (This is of course a compile error.) So we need to check that i < strct.NumFields before this statement. And let's add a test case that would have caught it.
(I do wonder whether RunDespiteErrors is worthwhile in most cases. It's much simpler to run analyzers only when the user has gotten their program to compile. I suppose it may be useful in an analyzer that points out a hard-to-spot mistake that would make it hard to get the program to compile.)
Patch Set #1, Line 105: if field == nil {
When is this condition true? I would not expect it.
Patch Set #1, Line 115: NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
If field.Name() is nonexported then the fix won't compile---and nor would the original code, so perhaps the right thing to do in that case is nothing.
Patch Set #1, Line 131: composite
"Add field names to struct literal"
ditto below
Patch Set #1, Line 138: Message: fmt.Sprintf("%s composite literal uses unkeyed fields", typeName),
s/composite/struct/
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alan Donovan, Suzy Mueller.
Suzy Mueller uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Suzy Mueller, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
go/analysis: add suggested fix for unkeyed composite literals
Include a suggested fix with the diagnostic for unkeyed composite
literals. This suggested fix will add the name of each of the
fields.
For golang/go#53062
Change-Id: I0c33191ff3cf66c95a9a055848274cc2b0c38224
---
M go/analysis/passes/composite/composite.go
M go/analysis/passes/composite/composite_test.go
M go/analysis/passes/composite/testdata/src/a/a.go
A go/analysis/passes/composite/testdata/src/a/a.go.golden
A go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
M go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
A go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
7 files changed, 250 insertions(+), 12 deletions(-)
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alan Donovan, Suzy Mueller.
5 comments:
File go/analysis/passes/composite/composite.go:
Patch Set #1, Line 104: field := strct.Field(i)
Analyzers with RunDespiteErrors=true must be very defensive. […]
Good catch. Adding a test case.
Patch Set #1, Line 105: if field == nil {
When is this condition true? I would not expect it.
When I read the documentation for (*types.Struct).Field, I had interpreted it to mean that it would return nil for out of bounds i (which is clearly a very incorrect assumption). Now that we have dealt with out of bounds above I will remove this check.
Patch Set #1, Line 115: NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
If field. […]
Will add a check for unexported as well. Not sure the best way to test this though.
Patch Set #1, Line 131: composite
"Add field names to struct literal" […]
Done
Patch Set #1, Line 138: Message: fmt.Sprintf("%s composite literal uses unkeyed fields", typeName),
s/composite/struct/
Done.
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alan Donovan, Suzy Mueller.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/1f764b9d-4efa-4ee3-b80a-d0230b8a6094
Patch set 2:gopls-CI +1
Attention is currently required from: Suzy Mueller.
2 comments:
Patchset:
Logic looks good.
File go/analysis/passes/composite/composite.go:
Patch Set #1, Line 115: NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
Will add a check for unexported as well. Not sure the best way to test this though.
&sync.Mutex{0, 0}?
The representation of Mutex is very unlikely to change.
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alan Donovan, Suzy Mueller.
Suzy Mueller uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Suzy Mueller, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
go/analysis: add suggested fix for unkeyed composite literals
Include a suggested fix with the diagnostic for unkeyed composite
literals. This suggested fix will add the name of each of the
fields.
For golang/go#53062
Change-Id: I0c33191ff3cf66c95a9a055848274cc2b0c38224
---
M go/analysis/passes/composite/composite.go
M go/analysis/passes/composite/composite_test.go
M go/analysis/passes/composite/testdata/src/a/a.go
A go/analysis/passes/composite/testdata/src/a/a.go.golden
A go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
M go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
A go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
7 files changed, 260 insertions(+), 12 deletions(-)
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alan Donovan, Suzy Mueller.
1 comment:
File go/analysis/passes/composite/composite.go:
Patch Set #1, Line 115: NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
&sync.Mutex{0, 0}? […]
Nice! Thanks.
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Suzy Mueller.
Patch set 3:Code-Review +2
Attention is currently required from: Suzy Mueller.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/4793bbe8-fd69-4576-bb16-1f689f548b33
Patch set 3:gopls-CI +1
Suzy Mueller submitted this change.
go/analysis: add suggested fix for unkeyed composite literals
Include a suggested fix with the diagnostic for unkeyed composite
literals. This suggested fix will add the name of each of the
fields.
For golang/go#53062
Change-Id: I0c33191ff3cf66c95a9a055848274cc2b0c38224
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414674
gopls-CI: kokoro <noreply...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Alan Donovan <adon...@google.com>
Run-TryBot: Suzy Mueller <suz...@golang.org>
---
M go/analysis/passes/composite/composite.go
M go/analysis/passes/composite/composite_test.go
M go/analysis/passes/composite/testdata/src/a/a.go
A go/analysis/passes/composite/testdata/src/a/a.go.golden
A go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden
M go/analysis/passes/composite/testdata/src/typeparams/typeparams.go
A go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden
7 files changed, 265 insertions(+), 12 deletions(-)
diff --git a/go/analysis/passes/composite/composite.go b/go/analysis/passes/composite/composite.go
index d3670ac..64e184d 100644
--- a/go/analysis/passes/composite/composite.go
+++ b/go/analysis/passes/composite/composite.go
@@ -7,6 +7,7 @@
package composite
import (
+ "fmt"
"go/ast"
"go/types"
"strings"
@@ -83,7 +84,8 @@
}
for _, typ := range structuralTypes {
under := deref(typ.Underlying())
- if _, ok := under.(*types.Struct); !ok {
+ strct, ok := under.(*types.Struct)
+ if !ok {
// skip non-struct composite literals
continue
}
@@ -92,20 +94,47 @@
continue
}
- // check if the CompositeLit contains an unkeyed field
+ // check if the struct contains an unkeyed field
allKeyValue := true
- for _, e := range cl.Elts {
+ var suggestedFixAvailable = len(cl.Elts) == strct.NumFields()
+ var missingKeys []analysis.TextEdit
+ for i, e := range cl.Elts {
if _, ok := e.(*ast.KeyValueExpr); !ok {
allKeyValue = false
- break
+ if i >= strct.NumFields() {
+ break
+ }
+ field := strct.Field(i)
+ if !field.Exported() {
+ // Adding unexported field names for structs not defined
+ // locally will not work.
+ suggestedFixAvailable = false
+ break
+ }
+ missingKeys = append(missingKeys, analysis.TextEdit{
+ Pos: e.Pos(),
+ End: e.Pos(),
+ NewText: []byte(fmt.Sprintf("%s: ", field.Name())),
+ })
}
}
if allKeyValue {
- // all the composite literal fields are keyed
+ // all the struct fields are keyed
continue
}
- pass.ReportRangef(cl, "%s composite literal uses unkeyed fields", typeName)
+ diag := analysis.Diagnostic{
+ Pos: cl.Pos(),
+ End: cl.End(),
+ Message: fmt.Sprintf("%s struct literal uses unkeyed fields", typeName),
+ }
+ if suggestedFixAvailable {
+ diag.SuggestedFixes = []analysis.SuggestedFix{{
+ Message: "Add field names to struct literal",
+ TextEdits: missingKeys,
+ }}
+ }
+ pass.Report(diag)
return
}
})
diff --git a/go/analysis/passes/composite/composite_test.go b/go/analysis/passes/composite/composite_test.go
index 952de8b..7afaaa7 100644
--- a/go/analysis/passes/composite/composite_test.go
+++ b/go/analysis/passes/composite/composite_test.go
@@ -18,5 +18,5 @@
if typeparams.Enabled {
pkgs = append(pkgs, "typeparams")
}
- analysistest.Run(t, testdata, composite.Analyzer, pkgs...)
+ analysistest.RunWithSuggestedFixes(t, testdata, composite.Analyzer, pkgs...)
}
diff --git a/go/analysis/passes/composite/testdata/src/a/a.go b/go/analysis/passes/composite/testdata/src/a/a.go
index 3a5bc20..cd69d39 100644
--- a/go/analysis/passes/composite/testdata/src/a/a.go
+++ b/go/analysis/passes/composite/testdata/src/a/a.go
@@ -11,6 +11,7 @@
"go/scanner"
"go/token"
"image"
+ "sync"
"unicode"
)
@@ -79,6 +80,18 @@
nil, // Value
"DefValue",
}
+var tooManyFieldsStructLiteral = flag.Flag{ // want "unkeyed fields"
+ "Name",
+ "Usage",
+ nil, // Value
+ "DefValue",
+ "Extra Field",
+}
+var tooFewFieldsStructLiteral = flag.Flag{ // want "unkeyed fields"
+ "Name",
+ "Usage",
+ nil, // Value
+}
var delta [3]rune
@@ -100,6 +113,10 @@
&scanner.Error{token.Position{}, "foobar"}, // want "unkeyed fields"
}
+// sync.Mutex has unexported fields. We expect a diagnostic but no
+// suggested fix.
+var mu = sync.Mutex{0, 0} // want "unkeyed fields"
+
// Check whitelisted structs: if vet is run with --compositewhitelist=false,
// this line triggers an error.
var whitelistedPoint = image.Point{1, 2}
diff --git a/go/analysis/passes/composite/testdata/src/a/a.go.golden b/go/analysis/passes/composite/testdata/src/a/a.go.golden
new file mode 100644
index 0000000..fe73a2e
--- /dev/null
+++ b/go/analysis/passes/composite/testdata/src/a/a.go.golden
@@ -0,0 +1,144 @@
+// 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 file contains the test for untagged struct literals.
+
+package a
+
+import (
+ "flag"
+ "go/scanner"
+ "go/token"
+ "image"
+ "sync"
+var tooManyFieldsStructLiteral = flag.Flag{ // want "unkeyed fields"
+ "Name",
+ "Usage",
+ nil, // Value
+ "DefValue",
+ "Extra Field",
+}
+var tooFewFieldsStructLiteral = flag.Flag{ // want "unkeyed fields"
+ "Name",
+ "Usage",
+ nil, // Value
+}
+
+var delta [3]rune
+
+// SpecialCase is a named slice of CaseRange to test issue 9171.
+var goodNamedSliceLiteral = unicode.SpecialCase{
+ {Lo: 1, Hi: 2, Delta: delta},
+ unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta},
+}
+var badNamedSliceLiteral = unicode.SpecialCase{
+ {Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+ unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields"
+}
+
+// ErrorList is a named slice, so no warnings should be emitted.
+var goodScannerErrorList = scanner.ErrorList{
+ &scanner.Error{Msg: "foobar"},
+}
+var badScannerErrorList = scanner.ErrorList{
+ &scanner.Error{Pos: token.Position{}, Msg: "foobar"}, // want "unkeyed fields"
+}
+
+// sync.Mutex has unexported fields. We expect a diagnostic but no
+// suggested fix.
+var mu = sync.Mutex{0, 0} // want "unkeyed fields"
+
To view, visit change 414674. To unsubscribe, or for help writing mail filters, visit settings.