icmd/vet: improved checking for variadic Println-like functions
- Automatically determine the first argument to check.
- Skip checking matching non-variadic functions.
- Skip checking matching functions accepting non-interface{}
variadic arguments.
- Removed fragile 'magic' code for special cases such as math.Log
and error interface.
Fixes #15067
Fixes #15099
Change-Id: Ib313557f18b12b36daa493f4b02c598b9503b55b
Reviewed-on: https://go-review.googlesource.com/21513
Run-TryBot: Rob Pike <r@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
diff --git a/src/cmd/vet/print.go b/src/cmd/vet/print.go
index 4e3252f..07499e6 100644
--- a/src/cmd/vet/print.go
+++ b/src/cmd/vet/print.go
@@ -35,20 +35,18 @@
if len(name) == 0 {
flag.Usage()
}
- skip := 0
+
+ // Backwards compatibility: skip optional first argument
+ // index after the colon.
if colon := strings.LastIndex(name, ":"); colon > 0 {
- var err error
- skip, err = strconv.Atoi(name[colon+1:])
- if err != nil {
- errorf(`illegal format for "Func:N" argument %q; %s`, name, err)
- }
name = name[:colon]
}
+
name = strings.ToLower(name)
if name[len(name)-1] == 'f' {
isFormattedPrint[name] = true
} else {
- printList[name] = skip
+ isPrint[name] = true
}
}
}
@@ -65,17 +63,20 @@
"sprintf": true,
}
-// printList records the unformatted-print functions. The value is the location
-// of the first parameter to be printed. Names are lower-cased so the lookup is
-// case insensitive.
-var printList = map[string]int{
- "error": 0,
- "fatal": 0,
- "fprint": 1, "fprintln": 1,
- "log": 0,
- "panic": 0, "panicln": 0,
- "print": 0, "println": 0,
- "sprint": 0, "sprintln": 0,
+// isPrint records the unformatted-print functions. Names are lower-cased
+// so the lookup is case insensitive.
+var isPrint = map[string]bool{
+ "error": true,
+ "fatal": true,
+ "fprint": true,
+ "fprintln": true,
+ "log": true,
+ "panic": true,
+ "panicln": true,
+ "print": true,
+ "println": true,
+ "sprint": true,
+ "sprintln": true,
}
// formatString returns the format string argument and its index within
@@ -171,8 +172,8 @@
f.checkPrintf(call, Name)
return
}
- if skip, ok := printList[name]; ok {
- f.checkPrint(call, Name, skip)
+ if _, ok := isPrint[name]; ok {
+ f.checkPrint(call, Name)
return
}
}
@@ -583,25 +584,36 @@
}
// checkPrint checks a call to an unformatted print routine such as Println.
-// call.Args[firstArg] is the first argument to be printed.
-func (f *File) checkPrint(call *ast.CallExpr, name string, firstArg int) {
- isLn := strings.HasSuffix(name, "ln")
- isF := strings.HasPrefix(name, "F")
- args := call.Args
- if name == "Log" && len(args) > 0 {
- // Special case: Don't complain about math.Log or cmplx.Log.
- // Not strictly necessary because the only complaint likely is for Log("%d")
- // but it feels wrong to check that math.Log is a good print function.
- if sel, ok := args[0].(*ast.SelectorExpr); ok {
- if x, ok := sel.X.(*ast.Ident); ok {
- if x.Name == "math" || x.Name == "cmplx" {
- return
- }
+func (f *File) checkPrint(call *ast.CallExpr, name string) {
+ firstArg := 0
+ typ := f.pkg.types[call.Fun].Type
+ if typ != nil {
+ if sig, ok := typ.(*types.Signature); ok {
+ if !sig.Variadic() {
+ // Skip checking non-variadic functions.
+ return
+ }
+ params := sig.Params()
+ firstArg = params.Len() - 1
+
+ typ := params.At(firstArg).Type()
+ typ = typ.(*types.Slice).Elem()
+ it, ok := typ.(*types.Interface)
+ if !ok || !it.Empty() {
+ // Skip variadic functions accepting non-interface{} args.
+ return
}
}
}
+ args := call.Args
+ if len(args) <= firstArg {
+ // Skip calls without variadic args.
+ return
+ }
+ args = args[firstArg:]
+
// check for Println(os.Stderr, ...)
- if firstArg == 0 && !isF && len(args) > 0 {
+ if firstArg == 0 {
if sel, ok := args[0].(*ast.SelectorExpr); ok {
if x, ok := sel.X.(*ast.Ident); ok {
if x.Name == "os" && strings.HasPrefix(sel.Sel.Name, "Std") {
@@ -610,31 +622,15 @@
}
}
}
- if len(args) <= firstArg {
- // If we have a call to a method called Error that satisfies the Error interface,
- // then it's ok. Otherwise it's something like (*T).Error from the testing package
- // and we need to check it.
- if name == "Error" && f.isErrorMethodCall(call) {
- return
- }
- // If it's an Error call now, it's probably for printing errors.
- if !isLn {
- // Check the signature to be sure: there are niladic functions called "error".
- if firstArg != 0 || f.numArgsInSignature(call) != firstArg {
- f.Badf(call.Pos(), "no args in %s call", name)
- }
- }
- return
- }
- arg := args[firstArg]
+ arg := args[0]
if lit, ok := arg.(*ast.BasicLit); ok && lit.Kind == token.STRING {
if strings.Contains(lit.Value, "%") {
f.Badf(call.Pos(), "possible formatting directive in %s call", name)
}
}
- if isLn {
+ if strings.HasSuffix(name, "ln") {
// The last item, if a string, should not have a newline.
- arg = args[len(call.Args)-1]
+ arg = args[len(args)-1]
if lit, ok := arg.(*ast.BasicLit); ok && lit.Kind == token.STRING {
if strings.HasSuffix(lit.Value, `\n"`) {
f.Badf(call.Pos(), "%s call ends with newline", name)
