Skip to content

Commit ed8e217

Browse files
blizzy78blizzy78
committed
improve inferring the types of idents (fixes #12)
1 parent 6048bfd commit ed8e217

File tree

4 files changed

+202
-27
lines changed

4 files changed

+202
-27
lines changed

‎testdata/src/decl/test.go

+35
Original file line numberDiff line numberDiff line change
@@ -167,3 +167,38 @@ func foo13() {
167167
println()
168168
println(mi)
169169
}
170+
171+
func foo14() {
172+
for i := range []string{"a", "b", "c"} {
173+
println()
174+
println()
175+
println()
176+
println()
177+
println()
178+
println()
179+
println(i)
180+
}
181+
182+
for i, s := range []string{"a", "b", "c"} {
183+
println()
184+
println()
185+
println()
186+
println()
187+
println()
188+
println()
189+
println(i, s)
190+
}
191+
192+
i, s := getTwo()
193+
println()
194+
println()
195+
println()
196+
println()
197+
println()
198+
println()
199+
println(i, s)
200+
}
201+
202+
func getTwo() (int, string) {
203+
return 1, "2"
204+
}

‎testdata/src/test/test.go

+69
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,16 @@ func foo(p int, p2 int, ip int, longParam int) (rv string) { // want "parameter
7272

7373
println(CI)
7474

75+
for x := range []struct{}{} { // want "variable name 'x' is too short for the scope of its usage"
76+
println()
77+
println()
78+
println()
79+
println()
80+
println()
81+
println()
82+
println(x)
83+
}
84+
7585
return
7686
}
7787

@@ -98,3 +108,62 @@ func Conventionals(ctx context.Context, t *testing.T, b *testing.B, tb testing.T
98108
pb.Next()
99109
ctx.Err()
100110
}
111+
112+
func Conventionals2(t *testing.T) {
113+
println()
114+
println()
115+
println()
116+
println()
117+
println()
118+
println()
119+
120+
var _ = struct{ t testing.TB }{t: t}
121+
}
122+
123+
func Conventionals3() {
124+
var t *testing.T = nil
125+
126+
println()
127+
println()
128+
println()
129+
println()
130+
println()
131+
println()
132+
133+
var _ = struct{ t testing.TB }{t: t}
134+
}
135+
136+
func Conventionals4() {
137+
t := &testing.T{}
138+
139+
println()
140+
println()
141+
println()
142+
println()
143+
println()
144+
println()
145+
146+
var _ = struct{ t testing.TB }{t: t}
147+
148+
println(t)
149+
}
150+
151+
func switcheroo() {
152+
type inter interface{}
153+
type str struct{}
154+
type str2 struct{}
155+
156+
var x inter
157+
switch y := x.(type) { // want "variable name 'y' is too short for the scope of its usage"
158+
// fill
159+
// fill
160+
// fill
161+
// fill
162+
// fill
163+
// fill
164+
case *str:
165+
_ = y
166+
case *str2:
167+
_ = y
168+
}
169+
}

‎varnamelen.go

+97-26
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"go/ast"
55
"go/token"
66
"go/types"
7+
"sort"
78
"strings"
89

910
"golang.org/x/tools/go/analysis"
@@ -169,7 +170,7 @@ func (v *varNameLen) run(pass *analysis.Pass) {
169170
}
170171

171172
// checkVariables applies v to variables in varToDist.
172-
func (v *varNameLen) checkVariables(pass *analysis.Pass, varToDist map[variable]int) {
173+
func (v *varNameLen) checkVariables(pass *analysis.Pass, varToDist map[variable]int) { //nolint:gocognit // it's not that complicated
173174
for variable, dist := range varToDist {
174175
if v.ignoreNames.contains(variable.name) {
175176
continue
@@ -195,6 +196,10 @@ func (v *varNameLen) checkVariables(pass *analysis.Pass, varToDist map[variable]
195196
continue
196197
}
197198

199+
if variable.isConventional() {
200+
continue
201+
}
202+
198203
if variable.assign != nil {
199204
pass.Reportf(variable.assign.Pos(), "%s name '%s' is too short for the scope of its usage", variable.kindName(), variable.name)
200205
continue
@@ -279,16 +284,23 @@ func (v *varNameLen) checkChannelReceiveOk(vari variable) bool {
279284

280285
// distances returns maps of variables, parameters, and return values mapping to their longest usage distances.
281286
func (v *varNameLen) distances(pass *analysis.Pass) (map[variable]int, map[parameter]int, map[parameter]int) {
282-
assignIdents, valueSpecIdents, paramIdents, returnIdents, imports := v.identsAndImports(pass)
287+
assignIdents, valueSpecIdents, paramIdents, returnIdents, imports, switches := v.identsAndImports(pass)
283288

284289
varToDist := map[variable]int{}
285290

286291
for _, ident := range assignIdents {
287292
assign := ident.Obj.Decl.(*ast.AssignStmt) //nolint:forcetypeassert // check is done in identsAndImports
288293

294+
var typ string
295+
if isTypeSwitchAssign(assign, switches) {
296+
typ = "<type-switched>"
297+
} else {
298+
typ = shortTypeName(pass.TypesInfo.TypeOf(ident), imports)
299+
}
300+
289301
variable := variable{
290302
name: ident.Name,
291-
typ: shortTypeName(pass.TypesInfo.TypeOf(identAssignExpr(ident, assign)), imports),
303+
typ: typ,
292304
assign: assign,
293305
}
294306

@@ -303,7 +315,7 @@ func (v *varNameLen) distances(pass *analysis.Pass) (map[variable]int, map[param
303315
variable := variable{
304316
name: ident.Name,
305317
constant: ident.Obj.Kind == ast.Con,
306-
typ: shortTypeName(pass.TypesInfo.TypeOf(valueSpec.Type), imports),
318+
typ: shortTypeName(pass.TypesInfo.TypeOf(ident), imports),
307319
valueSpec: valueSpec,
308320
}
309321

@@ -335,7 +347,7 @@ func (v *varNameLen) distances(pass *analysis.Pass) (map[variable]int, map[param
335347

336348
param := parameter{
337349
name: ident.Name,
338-
typ: shortTypeName(pass.TypesInfo.TypeOf(field.Type), imports),
350+
typ: shortTypeName(pass.TypesInfo.TypeOf(ident), imports),
339351
field: field,
340352
}
341353

@@ -348,18 +360,22 @@ func (v *varNameLen) distances(pass *analysis.Pass) (map[variable]int, map[param
348360
}
349361

350362
// identsAndImports returns Idents referencing assign statements, value specifications, parameters, and return values, respectively,
351-
// as well as import declarations.
352-
func (v *varNameLen) identsAndImports(pass *analysis.Pass) ([]*ast.Ident, []*ast.Ident, []*ast.Ident, []*ast.Ident, []importDeclaration) { //nolint:gocognit,cyclop // this is complex stuff
363+
// as well as import declarations, and type switch statements.
364+
func (v *varNameLen) identsAndImports(pass *analysis.Pass) ([]*ast.Ident, []*ast.Ident, []*ast.Ident, []*ast.Ident, []importDeclaration, []*ast.TypeSwitchStmt) { //nolint:gocognit,cyclop // this is complex stuff
353365
inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) //nolint:forcetypeassert // inspect.Analyzer always returns *inspector.Inspector
354366

355367
filter := []ast.Node{
356368
(*ast.ImportSpec)(nil),
357369
(*ast.FuncDecl)(nil),
370+
(*ast.CompositeLit)(nil),
371+
(*ast.TypeSwitchStmt)(nil),
358372
(*ast.Ident)(nil),
359373
}
360374

361375
funcs := []*ast.FuncDecl{}
362376
methods := []*ast.FuncDecl{}
377+
compositeLits := []*ast.CompositeLit{}
378+
switches := []*ast.TypeSwitchStmt{}
363379

364380
imports := []importDeclaration{}
365381
assignIdents := []*ast.Ident{}
@@ -386,11 +402,21 @@ func (v *varNameLen) identsAndImports(pass *analysis.Pass) ([]*ast.Ident, []*ast
386402

387403
methods = append(methods, node2)
388404

405+
case *ast.CompositeLit:
406+
compositeLits = append(compositeLits, node2)
407+
408+
case *ast.TypeSwitchStmt:
409+
switches = append(switches, node2)
410+
389411
case *ast.Ident:
390412
if node2.Obj == nil {
391413
return
392414
}
393415

416+
if isCompositeLitKey(node2, compositeLits) {
417+
return
418+
}
419+
394420
switch objDecl := node2.Obj.Decl.(type) {
395421
case *ast.AssignStmt:
396422
assignIdents = append(assignIdents, node2)
@@ -399,7 +425,13 @@ func (v *varNameLen) identsAndImports(pass *analysis.Pass) ([]*ast.Ident, []*ast
399425
valueSpecIdents = append(valueSpecIdents, node2)
400426

401427
case *ast.Field:
402-
if isReceiver(objDecl, methods) && !v.checkReceiver {
428+
if isReceiver(objDecl, methods) {
429+
if !v.checkReceiver {
430+
return
431+
}
432+
433+
paramIdents = append(paramIdents, node2)
434+
403435
return
404436
}
405437

@@ -423,7 +455,12 @@ func (v *varNameLen) identsAndImports(pass *analysis.Pass) ([]*ast.Ident, []*ast
423455
self: true,
424456
})
425457

426-
return assignIdents, valueSpecIdents, paramIdents, returnIdents, imports
458+
sort.Slice(imports, func(a, b int) bool {
459+
// reversed: longest path first
460+
return len(imports[a].path) > len(imports[b].path)
461+
})
462+
463+
return assignIdents, valueSpecIdents, paramIdents, returnIdents, imports, switches
427464
}
428465

429466
func importSpecToDecl(spec *ast.ImportSpec, imports []*types.Package) (importDeclaration, bool) {
@@ -555,6 +592,18 @@ func (v variable) isChannelReceiveOk() bool {
555592
return true
556593
}
557594

595+
// isConventional returns true if v matches a conventional Go variable/parameter name and type,
596+
// such as "ctx context.Context" or "t *testing.T".
597+
func (v variable) isConventional() bool {
598+
for _, decl := range conventionalDecls {
599+
if v.match(decl) {
600+
return true
601+
}
602+
}
603+
604+
return false
605+
}
606+
558607
// match returns true if v matches decl.
559608
func (v variable) match(decl declaration) bool {
560609
if v.name != decl.name {
@@ -615,8 +664,41 @@ func isReturn(field *ast.Field, funcs []*ast.FuncDecl) bool {
615664
return false
616665
}
617666

618-
// isConventional returns true if p is a conventional Go parameter, such as "ctx context.Context" or
619-
// "t *testing.T".
667+
// isKeyValueKey returns true if ident is a key of any of the given key/value expressions.
668+
func isCompositeLitKey(ident *ast.Ident, compositeLits []*ast.CompositeLit) bool {
669+
for _, cl := range compositeLits {
670+
if _, ok := cl.Type.(*ast.MapType); ok {
671+
continue
672+
}
673+
674+
for _, kvExpr := range cl.Elts {
675+
kv, ok := kvExpr.(*ast.KeyValueExpr)
676+
if !ok {
677+
continue
678+
}
679+
680+
if kv.Key == ident {
681+
return true
682+
}
683+
}
684+
}
685+
686+
return false
687+
}
688+
689+
// isTypeSwitchAssign returns true if assign is an assign statement of any of the given type switch statements.
690+
func isTypeSwitchAssign(assign *ast.AssignStmt, switches []*ast.TypeSwitchStmt) bool {
691+
for _, s := range switches {
692+
if s.Assign == assign {
693+
return true
694+
}
695+
}
696+
697+
return false
698+
}
699+
700+
// isConventional returns true if v matches a conventional Go variable/parameter name and type,
701+
// such as "ctx context.Context" or "t *testing.T".
620702
func (p parameter) isConventional() bool {
621703
for _, decl := range conventionalDecls {
622704
if p.match(decl) {
@@ -658,17 +740,6 @@ func (d declaration) matchType(typ string) bool {
658740
return d.typ == typ
659741
}
660742

661-
// identAssignExpr returns the expression that is assigned to ident.
662-
//
663-
// TODO: This currently only works for simple one-to-one assignments without the use of multi-values.
664-
func identAssignExpr(_ *ast.Ident, assign *ast.AssignStmt) ast.Expr {
665-
if len(assign.Lhs) != 1 || len(assign.Rhs) != 1 {
666-
return nil
667-
}
668-
669-
return assign.Rhs[0]
670-
}
671-
672743
// shortTypeName returns the short name of typ, with respect to imports.
673744
// For example, if package github.com/matryer/is is imported with alias "x",
674745
// and typ represents []*github.com/matryer/is.I, shortTypeName will return "[]*x.I".
@@ -683,12 +754,12 @@ func shortTypeName(typ types.Type, imports []importDeclaration) string {
683754
for _, imp := range imports {
684755
prefix := imp.path + "."
685756

686-
if imp.self {
687-
typStr = strings.ReplaceAll(typStr, prefix, "")
688-
continue
757+
replace := ""
758+
if !imp.self {
759+
replace = imp.name + "."
689760
}
690761

691-
typStr = strings.ReplaceAll(typStr, prefix, imp.name+".")
762+
typStr = strings.ReplaceAll(typStr, prefix, replace)
692763
}
693764

694765
return typStr

‎varnamelen_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ func TestVarNameLen_Run_IgnoreDeclarations(t *testing.T) {
6767
analyzer := NewAnalyzer()
6868
_ = analyzer.Flags.Set("minNameLength", "4")
6969
_ = analyzer.Flags.Set("checkReturn", "true")
70-
_ = analyzer.Flags.Set("ignoreDecls", "c context.Context, b bb.Buffer, b *strings.Builder, d *bb.Buffer, i int, ip *int, const C, f func(), m map[int]*bb.Buffer, mi int")
70+
_ = analyzer.Flags.Set("ignoreDecls", "c context.Context, b bb.Buffer, b *strings.Builder, d *bb.Buffer, i int, ip *int, const C, f func(), m map[int]*bb.Buffer, mi int, s string")
7171

7272
wd, _ := os.Getwd()
7373
analysistest.Run(t, wd+"/testdata", analyzer, "decl")

0 commit comments

Comments
 (0)