Writing Useful go/analysis Linter
November 10, 2019 | 27 min read
We’re going to write useful linter for Go using go/analysis. Then we will integrate it into go vet and golangci-lint.
What to lint
Let’s find all printf-like functions that don’t end with f. It’s the Go convention to name such functions with f at the end: fmt.Sprintf, fmt.Errorf,
fmt.Printf, fmt.Fprintf, fmt.Scanf, fmt.Sscanf, log.Printf, log.Fatalf, log.Panicf.
Writing a simple linter
Sample program with an issue
Let’s use the following example program for explanation and testing. Here myLog should be myLogf:
package main
import "log"
func myLog(format string, args ...interface{}) {
const prefix = "[my] "
log.Printf(prefix + format, args...)
}What is AST
Usually, such issues in code are detected by analyzing AST. AST - abstract syntax tree.
Let’s visualize AST of our program by this online visualizer.
Expand the full AST
0 *ast.File {
1 . Package: 1:1
2 . Name: *ast.Ident {
3 . . NamePos: 1:9
4 . . Name: "p"
5 . }
6 . Decls: []ast.Decl (len = 2) {
7 . . 0: *ast.GenDecl {
8 . . . TokPos: 3:1
9 . . . Tok: import
10 . . . Lparen: -
11 . . . Specs: []ast.Spec (len = 1) {
12 . . . . 0: *ast.ImportSpec {
13 . . . . . Path: *ast.BasicLit {
14 . . . . . . ValuePos: 3:8
15 . . . . . . Kind: STRING
16 . . . . . . Value: "\"log\""
17 . . . . . }
18 . . . . . EndPos: -
19 . . . . }
20 . . . }
21 . . . Rparen: -
22 . . }
23 . . 1: *ast.FuncDecl {
24 . . . Name: *ast.Ident {
25 . . . . NamePos: 5:6
26 . . . . Name: "myLog"
27 . . . . Obj: *ast.Object {
28 . . . . . Kind: func
29 . . . . . Name: "myLog"
30 . . . . . Decl: *(obj @ 23)
31 . . . . }
32 . . . }
33 . . . Type: *ast.FuncType {
34 . . . . Func: 5:1
35 . . . . Params: *ast.FieldList {
36 . . . . . Opening: 5:11
37 . . . . . List: []*ast.Field (len = 2) {
38 . . . . . . 0: *ast.Field {
39 . . . . . . . Names: []*ast.Ident (len = 1) {
40 . . . . . . . . 0: *ast.Ident {
41 . . . . . . . . . NamePos: 5:12
42 . . . . . . . . . Name: "format"
43 . . . . . . . . . Obj: *ast.Object {
44 . . . . . . . . . . Kind: var
45 . . . . . . . . . . Name: "format"
46 . . . . . . . . . . Decl: *(obj @ 38)
47 . . . . . . . . . }
48 . . . . . . . . }
49 . . . . . . . }
50 . . . . . . . Type: *ast.Ident {
51 . . . . . . . . NamePos: 5:19
52 . . . . . . . . Name: "string"
53 . . . . . . . }
54 . . . . . . }
55 . . . . . . 1: *ast.Field {
56 . . . . . . . Names: []*ast.Ident (len = 1) {
57 . . . . . . . . 0: *ast.Ident {
58 . . . . . . . . . NamePos: 5:27
59 . . . . . . . . . Name: "args"
60 . . . . . . . . . Obj: *ast.Object {
61 . . . . . . . . . . Kind: var
62 . . . . . . . . . . Name: "args"
63 . . . . . . . . . . Decl: *(obj @ 55)
64 . . . . . . . . . }
65 . . . . . . . . }
66 . . . . . . . }
67 . . . . . . . Type: *ast.Ellipsis {
68 . . . . . . . . Ellipsis: 5:32
69 . . . . . . . . Elt: *ast.InterfaceType {
70 . . . . . . . . . Interface: 5:35
71 . . . . . . . . . Methods: *ast.FieldList {
72 . . . . . . . . . . Opening: 5:44
73 . . . . . . . . . . Closing: 5:45
74 . . . . . . . . . }
75 . . . . . . . . . Incomplete: false
76 . . . . . . . . }
77 . . . . . . . }
78 . . . . . . }
79 . . . . . }
80 . . . . . Closing: 5:46
81 . . . . }
82 . . . }
83 . . . Body: *ast.BlockStmt {
84 . . . . Lbrace: 5:48
85 . . . . List: []ast.Stmt (len = 2) {
86 . . . . . 0: *ast.DeclStmt {
87 . . . . . . Decl: *ast.GenDecl {
88 . . . . . . . TokPos: 6:3
89 . . . . . . . Tok: const
90 . . . . . . . Lparen: -
91 . . . . . . . Specs: []ast.Spec (len = 1) {
92 . . . . . . . . 0: *ast.ValueSpec {
93 . . . . . . . . . Names: []*ast.Ident (len = 1) {
94 . . . . . . . . . . 0: *ast.Ident {
95 . . . . . . . . . . . NamePos: 6:9
96 . . . . . . . . . . . Name: "prefix"
97 . . . . . . . . . . . Obj: *ast.Object {
98 . . . . . . . . . . . . Kind: const
99 . . . . . . . . . . . . Name: "prefix"
100 . . . . . . . . . . . . Decl: *(obj @ 92)
101 . . . . . . . . . . . . Data: 0
102 . . . . . . . . . . . }
103 . . . . . . . . . . }
104 . . . . . . . . . }
105 . . . . . . . . . Values: []ast.Expr (len = 1) {
106 . . . . . . . . . . 0: *ast.BasicLit {
107 . . . . . . . . . . . ValuePos: 6:18
108 . . . . . . . . . . . Kind: STRING
109 . . . . . . . . . . . Value: "\"[my] \""
110 . . . . . . . . . . }
111 . . . . . . . . . }
112 . . . . . . . . }
113 . . . . . . . }
114 . . . . . . . Rparen: -
115 . . . . . . }
116 . . . . . }
117 . . . . . 1: *ast.ExprStmt {
118 . . . . . . X: *ast.CallExpr {
119 . . . . . . . Fun: *ast.SelectorExpr {
120 . . . . . . . . X: *ast.Ident {
121 . . . . . . . . . NamePos: 7:3
122 . . . . . . . . . Name: "log"
123 . . . . . . . . }
124 . . . . . . . . Sel: *ast.Ident {
125 . . . . . . . . . NamePos: 7:7
126 . . . . . . . . . Name: "Printf"
127 . . . . . . . . }
128 . . . . . . . }
129 . . . . . . . Lparen: 7:13
130 . . . . . . . Args: []ast.Expr (len = 2) {
131 . . . . . . . . 0: *ast.BinaryExpr {
132 . . . . . . . . . X: *ast.Ident {
133 . . . . . . . . . . NamePos: 7:14
134 . . . . . . . . . . Name: "prefix"
135 . . . . . . . . . . Obj: *(obj @ 97)
136 . . . . . . . . . }
137 . . . . . . . . . OpPos: 7:21
138 . . . . . . . . . Op: +
139 . . . . . . . . . Y: *ast.Ident {
140 . . . . . . . . . . NamePos: 7:23
141 . . . . . . . . . . Name: "format"
142 . . . . . . . . . . Obj: *(obj @ 43)
143 . . . . . . . . . }
144 . . . . . . . . }
145 . . . . . . . . 1: *ast.Ident {
146 . . . . . . . . . NamePos: 7:31
147 . . . . . . . . . Name: "args"
148 . . . . . . . . . Obj: *(obj @ 60)
149 . . . . . . . . }
150 . . . . . . . }
151 . . . . . . . Ellipsis: 7:35
152 . . . . . . . Rparen: 7:38
153 . . . . . . }
154 . . . . . }
155 . . . . }
156 . . . . Rbrace: 8:1
157 . . . }
158 . . }
159 . }
160 . Scope: *ast.Scope {
161 . . Objects: map[string]*ast.Object (len = 1) {
162 . . . "myLog": *(obj @ 27)
163 . . }
164 . }
165 . Imports: []*ast.ImportSpec (len = 1) {
166 . . 0: *(obj @ 12)
167 . }
168 . Unresolved: []*ast.Ident (len = 2) {
169 . . 0: *(obj @ 50)
170 . . 1: *(obj @ 120)
171 . }
172 }The most interesting part of the AST for us is the myLog function declaration. We can simplify it to:
&ast.FuncDecl{
Name: &ast.Ident{Name: "myLog"},
Type: &ast.FuncType{
Params: &ast.FieldList{
List: []*ast.Field{
&ast.Field{
Names: []*ast.Ident{&ast.Ident{Name: "format"}},
Type: &ast.Ident{Name: "string"},
},
&ast.Field{
Names: []*ast.Ident{&ast.Ident{Name: "args"}},
Type: &ast.Ellipsis{
Elt: &ast.InterfaceType{
Methods: &ast.FieldList{
List: nil, // no methods in the interface{}
}, // end of Methods
}, // end of &ast.InterfaceType
}, // end of &ast.Ellipsis
}, // end of &ast.Field
}, // end of []*ast.Field
}, // end of &ast.FieldList
}, // end of &ast.FuncType
} // end of &ast.FuncDeclLet’s write it
To analyze AST we need to parse it first:
package main
import (
"bytes"
"fmt"
"go/ast"
"go/parser"
"go/printer"
"go/token"
"log"
"os"
)
func main() {
v := visitor{fset: token.NewFileSet()}
for _, filePath := range os.Args[1:] {
if filePath == "--" { // to be able to run this like "go run main.go -- input.go"
continue
}
f, err := parser.ParseFile(v.fset, filePath, nil, 0)
if err != nil {
log.Fatalf("Failed to parse file %s: %s", filePath, err)
}
ast.Walk(&v, f)
}
}
type visitor struct {
fset *token.FileSet
}
func (v *visitor) Visit(node ast.Node) ast.Visitor {
if node == nil {
return nil
}
var buf bytes.Buffer
printer.Fprint(&buf, v.fset, node)
fmt.Printf("%s | %#v\n", buf.String(), node)
return v
}parser.ParseFile parses source file and builds AST. ast.Walk visits nodes of AST with our visitor: it continues visiting nodes until the Visit method returns nil or all nodes were visited.
In this simple implementation, we parse source files and visit all AST nodes.
Let’s run it on the example file with myLog function:
go run ./main.go -- ./example.goShow output
import "log"
func myLog(format string, args ...interface{}) {
const prefix = "[my] "
log.Printf(prefix+format, args...)
}
| &ast.File{Doc:(*ast.CommentGroup)(nil), Package:1, Name:(*ast.Ident)(0xc000096020), Decls:[]ast.Decl{(*ast.GenDecl)(0xc0000a2040), (*ast.FuncDecl)(0xc0000882a0)}, Scope:(*ast.Scope)(0xc00009a050), Imports:[]*ast.ImportSpec{(*ast.ImportSpec)(0xc000088150)}, Unresolved:[]*ast.Ident{(*ast.Ident)(0xc0000960a0), (*ast.Ident)(0xc000096180)}, Comments:[]*ast.CommentGroup(nil)}
main | &ast.Ident{NamePos:9, Name:"main", Obj:(*ast.Object)(nil)}
import "log" | &ast.GenDecl{Doc:(*ast.CommentGroup)(nil), TokPos:15, Tok:75, Lparen:0, Specs:[]ast.Spec{(*ast.ImportSpec)(0xc000088150)}, Rparen:0}
"log" | &ast.ImportSpec{Doc:(*ast.CommentGroup)(nil), Name:(*ast.Ident)(nil), Path:(*ast.BasicLit)(0xc000096040), Comment:(*ast.CommentGroup)(nil), EndPos:0}
"log" | &ast.BasicLit{ValuePos:22, Kind:9, Value:"\"log\""}
func myLog(format string, args ...interface{}) {
const prefix = "[my] "
log.Printf(prefix+format, args...)
} | &ast.FuncDecl{Doc:(*ast.CommentGroup)(nil), Recv:(*ast.FieldList)(nil), Name:(*ast.Ident)(0xc000096060), Type:(*ast.FuncType)(0xc000096280), Body:(*ast.BlockStmt)(0xc000088270)}
myLog | &ast.Ident{NamePos:34, Name:"myLog", Obj:(*ast.Object)(0xc00009c1e0)}
func(format string, args ...interface{}) | &ast.FuncType{Func:29, Params:(*ast.FieldList)(0xc0000881e0), Results:(*ast.FieldList)(nil)}
| &ast.FieldList{Opening:39, List:[]*ast.Field{(*ast.Field)(0xc0000a2080), (*ast.Field)(0xc0000a20c0)}, Closing:74}
| &ast.Field{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident{(*ast.Ident)(0xc000096080)}, Type:(*ast.Ident)(0xc0000960a0), Tag:(*ast.BasicLit)(nil), Comment:(*ast.CommentGroup)(nil)}
format | &ast.Ident{NamePos:40, Name:"format", Obj:(*ast.Object)(0xc00009c0a0)}
string | &ast.Ident{NamePos:47, Name:"string", Obj:(*ast.Object)(nil)}
| &ast.Field{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident{(*ast.Ident)(0xc0000960c0)}, Type:(*ast.Ellipsis)(0xc000096100), Tag:(*ast.BasicLit)(nil), Comment:(*ast.CommentGroup)(nil)}
args | &ast.Ident{NamePos:55, Name:"args", Obj:(*ast.Object)(0xc00009c0f0)}
...interface{} | &ast.Ellipsis{Ellipsis:60, Elt:(*ast.InterfaceType)(0xc0000960e0)}
interface{} | &ast.InterfaceType{Interface:63, Methods:(*ast.FieldList)(0xc0000881b0), Incomplete:false}
| &ast.FieldList{Opening:72, List:[]*ast.Field(nil), Closing:73}
{
const prefix = "[my] "
log.Printf(prefix+format, args...)
} | &ast.BlockStmt{Lbrace:76, List:[]ast.Stmt{(*ast.DeclStmt)(0xc00009a0e0), (*ast.ExprStmt)(0xc00009a130)}, Rbrace:138}
const prefix = "[my] " | &ast.DeclStmt{Decl:(*ast.GenDecl)(0xc0000a2100)}
const prefix = "[my] " | &ast.GenDecl{Doc:(*ast.CommentGroup)(nil), TokPos:79, Tok:64, Lparen:0, Specs:[]ast.Spec{(*ast.ValueSpec)(0xc00009c140)}, Rparen:0}
prefix = "[my] " | &ast.ValueSpec{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident{(*ast.Ident)(0xc000096140)}, Type:ast.Expr(nil), Values:[]ast.Expr{(*ast.BasicLit)(0xc000096160)}, Comment:(*ast.CommentGroup)(nil)}
prefix | &ast.Ident{NamePos:85, Name:"prefix", Obj:(*ast.Object)(0xc00009c190)}
"[my] " | &ast.BasicLit{ValuePos:94, Kind:9, Value:"\"[my] \""}
log.Printf(prefix+format, args...) | &ast.ExprStmt{X:(*ast.CallExpr)(0xc0000a2140)}
log.Printf(prefix+format, args...) | &ast.CallExpr{Fun:(*ast.SelectorExpr)(0xc0000961c0), Lparen:113, Args:[]ast.Expr{(*ast.BinaryExpr)(0xc000088240), (*ast.Ident)(0xc000096220)}, Ellipsis:133, Rparen:136}
log.Printf | &ast.SelectorExpr{X:(*ast.Ident)(0xc000096180), Sel:(*ast.Ident)(0xc0000961a0)}
log | &ast.Ident{NamePos:103, Name:"log", Obj:(*ast.Object)(nil)}
Printf | &ast.Ident{NamePos:107, Name:"Printf", Obj:(*ast.Object)(nil)}
prefix + format | &ast.BinaryExpr{X:(*ast.Ident)(0xc0000961e0), OpPos:120, Op:12, Y:(*ast.Ident)(0xc000096200)}
prefix | &ast.Ident{NamePos:114, Name:"prefix", Obj:(*ast.Object)(0xc00009c190)}
format | &ast.Ident{NamePos:121, Name:"format", Obj:(*ast.Object)(0xc00009c0a0)}
args | &ast.Ident{NamePos:129, Name:"args", Obj:(*ast.Object)(0xc00009c0f0)}You can see that it visits and prints all nodes. Now let’s write linting in visitor.Visit:
func (v *visitor) Visit(node ast.Node) ast.Visitor {
funcDecl, ok := node.(*ast.FuncDecl)
if !ok {
return v
}
params := funcDecl.Type.Params.List
if len(params) != 2 { // [0] must be format (string), [1] must be args (...interface{})
return v
}
firstParamType, ok := params[0].Type.(*ast.Ident)
if !ok { // first param type isn't identificator so it can't be of type "string"
return v
}
if firstParamType.Name != "string" { // first param (format) type is not string
return v
}
secondParamType, ok := params[1].Type.(*ast.Ellipsis)
if !ok { // args are not ellipsis (...args)
return v
}
elementType, ok := secondParamType.Elt.(*ast.InterfaceType)
if !ok { // args are not of interface type, but we need interface{}
return v
}
if elementType.Methods != nil && len(elementType.Methods.List) != 0 {
return v // has >= 1 method in interface, but we need an empty interface "interface{}"
}
if strings.HasSuffix(funcDecl.Name.Name, "f") {
return v
}
fmt.Printf("%s: printf-like formatting function '%s' should be named '%sf'\n",
v.fset.Position(node.Pos()), funcDecl.Name.Name, funcDecl.Name.Name)
return v
}Run it and ensure that it works:
$ go run ./main.go -- ./example.go
./example.go:5:1: printf-like formatting function 'myLog' should be named 'myLogf'We’ve implemented a simple linter without go/analysis or any other frameworks. Let’s move on to how we can use go/analysis to improve it.
Using go/analysis
Go/analysis is an API for modular analysis. Also, it’s a common interface for all linters. This API simplifies developing a new linter. It forces using best practices for static analysis, for example, working with one package at a time.
The primary type in the API is analysis.Analyzer. An Analyzer statically describes an analysis function: its name, documentation, flags, relationship to other analyzers, and of course, it’s logic.
To define an analysis, a user declares a variable of type *analysis.Analyzer:
type Analyzer struct {
Name string
Doc string
Flags flag.FlagSet
Run func(*Pass) (interface{}, error)
RunDespiteErrors bool
ResultType reflect.Type
Requires []*Analyzer
FactTypes []Fact
}The most interesting field is Run: it’s where linting logic lives. It takes *analysis.Pass as an argument:
type Pass struct {
Fset *token.FileSet
Files []*ast.File
OtherFiles []string
Pkg *types.Package
TypesInfo *types.Info
ResultOf map[*Analyzer]interface{}
Report func(Diagnostic)
...
}A Pass describes a single unit of work: the application of a particular Analyzer to a particular package of Go code. The Pass provides information to the Analyzer’s Run function about the package being analyzed. It provides operations to the Run function for reporting diagnostics and other information back to the driver.
The Fset, Files, Pkg, and TypesInfo fields provide source positions, the syntax trees, type information for a single package of Go code. We don’t need type information, so Pkg and TypesInfo aren’t needed. Wait, but do we need to know that the first argument of printf-like function has type string? Yes, we need. But AST contains enough information about that, therefore we don’t need type-checking.
Why do we need go/analysis
There are 2 main reasons to use go/analysis: unified interface and code reuse.
Go/analysis provides a unified interface for linters. It simplifies their integration into IDE, metalinters, code review tools, etc.
For example, any go/analysis linter can be effectively run by go vet. We will talk about integration later in this article.
In our simple linter, we require a user to pass a list of all files to analyze. But the convenient way is to run linter without arguments or with ./... to run it recursively on all files in all packages. We can implement it ourselves: I already did it in the first version of golangci-lint. It requires 100-500 lines of code. Or we can use the go/analysis API to do this in 10 lines of code.
Let’s use go/analysis
Let’s port our simple linter to use go/analysis API. I’ve made the repository go-printf-func-name for that.
First, let’s define the project structure by following popular Go project layout:
.
├── README.md
├── cmd
│ └── go-printf-func-name
│ └── main.go
├── go.mod
├── go.sum
└── pkg
└── analyzer
└── analyzer.goLet’s start from the main function:
package main
import (
"github.com/jirfag/go-printf-func-name/pkg/analyzer"
"golang.org/x/tools/go/analysis/singlechecker"
)
func main() {
singlechecker.Main(analyzer.Analyzer)
}That’s all! singlechecker.Main generates CLI for running linter.
Let’s define Analyzer:
package analyzer
import (
"go/ast"
"strings"
"golang.org/x/tools/go/analysis"
)
var Analyzer = &analysis.Analyzer{
Name: "goprintffuncname",
Doc: "Checks that printf-like functions are named with `f` at the end.",
Run: run,
}
func run(pass *analysis.Pass) (interface{}, error) {
inspect := func(node ast.Node) bool {
funcDecl, ok := node.(*ast.FuncDecl)
if !ok {
return true
}
params := funcDecl.Type.Params.List
if len(params) != 2 { // [0] must be format (string), [1] must be args (...interface{})
return true
}
firstParamType, ok := params[0].Type.(*ast.Ident)
if !ok { // first param type isn't identificator so it can't be of type "string"
return true
}
if firstParamType.Name != "string" { // first param (format) type is not string
return true
}
secondParamType, ok := params[1].Type.(*ast.Ellipsis)
if !ok { // args are not ellipsis (...args)
return true
}
elementType, ok := secondParamType.Elt.(*ast.InterfaceType)
if !ok { // args are not of interface type, but we need interface{}
return true
}
if elementType.Methods != nil && len(elementType.Methods.List) != 0 {
return true // has >= 1 method in interface, but we need an empty interface "interface{}"
}
if strings.HasSuffix(funcDecl.Name.Name, "f") {
return true
}
pass.Reportf(node.Pos(), "printf-like formatting function '%s' should be named '%sf'",
funcDecl.Name.Name, funcDecl.Name.Name)
return true
}
for _, f := range pass.Files {
ast.Inspect(f, inspect)
}
return nil, nil
}Notice what changed:
- We use
ast.Inspectinstead ofast.Walk: it requires less code. To do that we just need to remove the visitor and returntrueinstead of the visitor. - We iterate over
pass.Filesto get syntax trees (AST) for the current package. We don’t load them manually byparser.ParseFileanymore. - We report about found issues by
pass.Reportfinstead offmt.Printf. This method fromgo/analysisabstracts reporting logic from us. Also, we’ve removed\nfrom the message.
Run it
Let’s run it:
$ go run ./cmd/go-printf-func-name/main.go
goprintffuncname: Checks that printf-like functions are named with `f` at the end.
Usage: goprintffuncname [-flag] [package]
Flags: -V print version and exit
-all
no effect (deprecated)
-c int
display offending line with this many lines of context (default -1)
-cpuprofile string
write CPU profile to this file
-debug string
debug flags, any subset of "fpstv"
-fix
apply all suggested fixes
-flags
print analyzer flags in JSON
-json
emit JSON output
-memprofile string
write memory profile to this file
-source
no effect (deprecated)
-tags string
no effect (deprecated)
-trace string
write trace log to this file
-v no effect (deprecated)
exit status 1Thanks to go/analysis we got a lot of command-line options.
Now it’s time to test it again on example source code:
$ go run ./cmd/go-printf-func-name/main.go -- ./example.go
/Users/denis/go-printf-func-name/example.go:5:1: printf-like formatting function 'myLog' should be named 'myLogf'
exit status 3Yes, it works!
ast.Inspect vs Inspector
Did you notice ineffectiveness in ast.Inspect and ast.Walk? We visit every AST node: binary expressions, variables, constants, etc. But we need only function declarations.
Can we visit only them to improve performance?
Yes, we can. Go/analysis contains inspect.Analyzer. This analyzer filters AST nodes by types.
First, we should require it as a dependency for our analyzer:
var Analyzer = &analysis.Analyzer{
Name: "goprintffuncname",
Doc: "Checks that printf-like functions are named with `f` at the end.",
Run: run,
Requires: []*analysis.Analyzer{inspect.Analyzer},}Change the run function to:
func run(pass *analysis.Pass) (interface{}, error) {
// pass.ResultOf[inspect.Analyzer] will be set if we've added inspect.Analyzer to Requires.
inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
nodeFilter := []ast.Node{ // filter needed nodes: visit only them
(*ast.FuncDecl)(nil),
}
inspector.Preorder(nodeFilter, func(node ast.Node) {
funcDecl := node.(*ast.FuncDecl)
params := funcDecl.Type.Params.List
if len(params) != 2 { // [0] must be format (string), [1] must be args (...interface{})
return
}
firstParamType, ok := params[0].Type.(*ast.Ident)
if !ok { // first param type isn't identificator so it can't be of type "string"
return
}
if firstParamType.Name != "string" { // first param (format) type is not string
return
}
secondParamType, ok := params[1].Type.(*ast.Ellipsis)
if !ok { // args are not ellipsis (...args)
return
}
elementType, ok := secondParamType.Elt.(*ast.InterfaceType)
if !ok { // args are not of interface type, but we need interface{}
return
}
if elementType.Methods != nil && len(elementType.Methods.List) != 0 {
return // has >= 1 method in interface, but we need an empty interface "interface{}"
}
if strings.HasSuffix(funcDecl.Name.Name, "f") {
return
}
pass.Reportf(node.Pos(), "printf-like formatting function '%s' should be named '%sf'",
funcDecl.Name.Name, funcDecl.Name.Name)
})
return nil, nil
}Here we’ve changed AST walking a little bit. Now we use inspector.Preorder and don’t return anything from visiting function.
The excerpt from the inspector’s comments explains the profit from its usage:
// During construction, the inspector does a complete traversal and
// builds a list of push/pop events and their node type. Subsequent
// method calls that request a traversal scan this list, rather than walk
// the AST, and perform type filtering using efficient bit sets.
//
// Experiments suggest the inspector's traversals are about 2.5x faster
// than ast.Inspect, but it may take around 5 traversals for this
// benefit to amortize the inspector's construction cost.
// If efficiency is the primary concern, do not use Inspector for
// one-off traversals.We expect our linter to be used from linters runner like golangci-lint. Such runners contain dozens of go/analysis analyzers, therefore, we are sure that inspector construction time will be amortized. And even if it’s slower - it’s just a simpler API to traverse syntax trees.
Testing
Automated tests
It’s time to test our linter. Go/analysis provides a framework for this. Let’s create files analyzer_test.go and testdata/src/p/p.go:
.
├── README.md
├── cmd
│ └── go-printf-func-name
│ └── main.go
├── example.go
├── go.mod
├── go.sum
├── pkg
│ └── analyzer
│ ├── analyzer.go
│ └── analyzer_test.go
└── testdata
└── src
└── p
└── p.goIn the file pkg/analyzer/analyzer_test.go we write:
func TestAll(t *testing.T) {
wd, err := os.Getwd()
if err != nil {
t.Fatalf("Failed to get wd: %s", err)
}
testdata := filepath.Join(filepath.Dir(filepath.Dir(wd)), "testdata")
analysistest.Run(t, testdata, analyzer.Analyzer, "p")
}Now we can write test functions in testdata/src/p/p.go. If linter should report about a function - we leave a comment // want "printf-like formatting function". The go/analysis finds such comments and ensures that they match found issues on these lines.
Let’s start from simple tests:
package p
func notPrintfFuncAtAll() {}
func funcWithEllipsis(args ...interface{}) {}
func printfLikeButWithStrings(format string, args ...string) {}
func printfLikeButWithBadFormat(format int, args ...interface{}) {}
func prinfLikeFunc(format string, args ...interface{}) {} // want "printf-like formatting function"
func prinfLikeFuncWithReturnValue(format string, args ...interface{}) string { // want "printf-like formatting function"
return ""
}If we run tests they pass:
$ go test -v ./...
? github.com/jirfag/go-printf-func-name [no test files]
? github.com/jirfag/go-printf-func-name/cmd/go-printf-func-name [no test files]
=== RUN TestAll
--- PASS: TestAll (0.29s)
PASS
ok github.com/jirfag/go-printf-func-name/pkg/analyzer 0.387sDid we finish with tests? No. Let’s check our code coverage. I’m using VS Code with the following configuration for code coverage:
"go.coverOnSave": true,
"go.coverageDecorator": {
"type": "gutter",
"coveredHighlightColor": "rgba(64,128,128,0.5)",
"uncoveredHighlightColor": "rgba(128,64,64,0.25)",
"coveredGutterStyle": "blockgreen",
"uncoveredGutterStyle": "blockred"
},
"go.coverOnSingleTest": true,
}I’ve run the test from VS Code and have checked the coverage in pkg/analyzer/analyzer.go:
Some code blocks are collapsed to reduce the screenshot height. Covered lines are marked with green strips to the left of the line numbers. You can see that we didn’t cover 4 cases, let’s fix it. I’ve added these test cases:
func secondArgIsNotEllipsis(arg1 string, arg2 int) {}
func printfLikeButWithExtraInterfaceMethods(format string, args ...interface {
String() string
}) {
}
func prinfLikeFuncf(format string, args ...interface{}) {}I’ve checked coverage again: all cases except one are covered. The uncovered one is:
firstParamType, ok := params[0].Type.(*ast.Ident)
if !ok { // first param type isn't identificator so it can't be of type "string"
return
}I couldn’t find a test case for it.
Did we finish with tests? No. We’ve forgotten about important test case:
func prinfLikeFuncWithExtraArgs1(extraArg, format string, args ...interface{}) {} // want "printf-like formatting function"
func prinfLikeFuncWithExtraArgs2(extraArg int, format string, args ...interface{}) {} // want "printf-like formatting function"I’ve added these test cases and tests have failed. Let’s fix our code:
params := funcDecl.Type.Params.List
if len(params) < 2 { // [0] must be format (string), [1] must be args (...interface{})
return
}
formatParamType, ok := params[len(params)-2].Type.(*ast.Ident)
if !ok { // first param type isn't identificator so it can't be of type "string"
return
}
if formatParamType.Name != "string" { // first param (format) type is not string
return
}
argsParamType, ok := params[len(params)-1].Type.(*ast.Ellipsis)
if !ok { // args are not ellipsis (...args)
return
}
elementType, ok := argsParamType.Elt.(*ast.InterfaceType)
if !ok { // args are not of interface type, but we need interface{}
return
}Now tests pass and we can move on.
Run on the real and large repositories
It’s important to test on real code on large codebases. Let’s run it on Go source code:
go install ./cmd/...
cd $GOROOT/src
go-printf-func-name ./...It reports about ~30 issues. Some of them are valid:
func (check *Checker) trace(pos token.Pos, format string, args ...interface{}) {
fmt.Printf("%s:\t%s%s\n",
check.fset.Position(pos),
strings.Repeat(". ", check.indent),
check.sprintf(format, args...),
)
}Some of them are false-positives:
func Sscanf(str string, format string, a ...interface{}) (n int, err error) {
return Fscanf((*stringReader)(&str), format, a...)
}
func (tx *Tx) Query(query string, args ...interface{}) (*Rows, error) {
return tx.QueryContext(context.Background(), query, args...)
}Let’s fix it. The proper fix is to trace all calls inside such functions and find out whether format argument goes to formatting function like fmt.Sprintf. Maybe we will do it in the next post. Now, let’s make a simple fix: require format argument name to be equal "format" and don’t allow return values. It will increase precision of our analysis but will decrease recall.
You can find fixed code and tests in linter’s GitHub repo.
I’ve run on $GOROOT again and got only 8 issues: all of them are valid. Also, I’ve run the linter on istio source code and it reported 8 valid issues.
We’ve tested our linter. The final step is to integrate it into linter runners.
Integrating
Integrate with go vet
It’s very simple:
go install ./cmd/...
go vet -vettool=$(which go-printf-func-name) ./...If you see flag provided but not defined: -unsafeptr add these lines to the main function:
// Don't use it: just to not crash on -unsafeptr flag from go vet
flag.Bool("unsafeptr", false, "")Such usage isn’t useful in practice: we’ve replaced default go vet checks by one our check. Instead, we can save go vet checks and add our linter. To do this copy the sample go vet main and add your analyzer to it.
Integrate with golangci-lint
Golangci-lint is a linters runner (I’m the author of it). It contains dozes of linters. Let’s add our linter to it. The main part of adding is:
func NewGoPrintfFuncName() *goanalysis.Linter {
return goanalysis.NewLinter(
"goprintffuncname",
"Checks that printf-like functions are named with `f` at the end",
[]*analysis.Analyzer{analyzer.Analyzer},
nil,
).WithLoadMode(goanalysis.LoadModeSyntax)
}In the end, don’t forget to do make vendor README.md.
The whole pull request to golangci-lint is small:
---
README.md | 3 +++
go.mod | 3 ++-
go.sum | 2 ++
pkg/golinters/goprintffuncname.go | 17 +++++++++++++++++
pkg/lint/lintersdb/manager.go | 3 +++
test/testdata/goprintffuncname.go | 5 +++++
vendor/modules.txt | 4 +++-
7 files changed, 35 insertions(+), 2 deletions(-)Let’s check it on istio repo:
~/istio ((1.3.5))$ golangci-lint run --no-config --disable-all -E goprintffuncname
pkg/test/framework/components/prometheus/kube.go:203:1: printf-like formatting function 'WaitForOneOrMoreOrFail' should be named 'WaitForOneOrMoreOrFailf' (goprintffuncname)
func (c *kubeComponent) WaitForOneOrMoreOrFail(t test.Failer, format string, args ...interface{}) {
^
mixer/pkg/runtime/config/ephemeral.go:914:1: printf-like formatting function 'appendErr' should be named 'appendErrf' (goprintffuncname)
func appendErr(errs *multierror.Error, field string, format string, a ...interface{}) {
^
mixer/pkg/lang/compiler/compiler.go:754:1: printf-like formatting function 'internalError' should be named 'internalErrorf' (goprintffuncname)
func (g *generator) internalError(format string, args ...interface{}) {
^
mixer/tools/codegen/pkg/modelgen/diag.go:80:1: printf-like formatting function 'addError' should be named 'addErrorf' (goprintffuncname)
func (m *Model) addError(file string, line string, format string, a ...interface{}) {
^
mixer/pkg/il/text/read.go:321:1: printf-like formatting function 'fail' should be named 'failf' (goprintffuncname)
func (p *parser) fail(format string, args ...interface{}) {
^
mixer/pkg/il/text/read.go:325:1: printf-like formatting function 'failLoc' should be named 'failLocf' (goprintffuncname)
func (p *parser) failLoc(l location, format string, args ...interface{}) {
^
mixer/pkg/runtime/testing/data/logger.go:35:1: printf-like formatting function 'WriteFormat' should be named 'WriteFormatf' (goprintffuncname)
func (l *Logger) WriteFormat(name string, format string, args ...interface{}) {
^
galley/pkg/testing/common/mocklog.go:29:1: printf-like formatting function 'Append' should be named 'Appendf' (goprintffuncname)
func (e *MockLog) Append(format string, args ...interface{}) {
^It works. The pull request to golangci-lint is here.
What’s next
We’ve written a real-world linter. It’s code is here. The linter has found issues in Go and Istio source code.
We’ve made a pull request to include it into golangci-lint.
The next step can be improving its recall by using facts and not checking that param name equals format.
