Skip to content

Commit

Permalink
Merge pull request #15 from pohly/check-stringer
Browse files Browse the repository at this point in the history 
check for interface inheritance
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored May 11, 2023
2 parents 4ac3afd + 137dd42 commit ff0f1e9
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 107 deletions.
7 changes: 7 additions & 0 deletions logcheck/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ func TestAnalyzer(t *testing.T) {
},
testPackage: "allowBadkeysLogs",
},
{
name: "Detect incomplete fmt.Stringer",
enabled: map[string]string{
"value": "true",
},
testPackage: "stringer",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
Expand Down
233 changes: 126 additions & 107 deletions logcheck/pkg/logcheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const (
withHelpersCheck = "with-helpers"
verbosityZeroCheck = "verbosity-zero"
keyCheck = "key"
valueCheck = "value"
deprecationsCheck = "deprecations"
)

Expand All @@ -63,6 +64,7 @@ func Analyser() *analysis.Analyzer {
withHelpersCheck: new(bool),
verbosityZeroCheck: new(bool),
keyCheck: new(bool),
valueCheck: new(bool),
deprecationsCheck: new(bool),
},
}
Expand All @@ -79,6 +81,7 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`)
logcheckFlags.BoolVar(c.enabled[withHelpersCheck], prefix+withHelpersCheck, false, `When true, logcheck will warn about direct calls to WithName, WithValues and NewContext.`)
logcheckFlags.BoolVar(c.enabled[verbosityZeroCheck], prefix+verbosityZeroCheck, true, `When true, logcheck will check whether the parameter for V() is 0.`)
logcheckFlags.BoolVar(c.enabled[keyCheck], prefix+keyCheck, true, `When true, logcheck will check whether name arguments are valid keys according to the guidelines in (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments).`)
logcheckFlags.BoolVar(c.enabled[valueCheck], prefix+valueCheck, false, `When true, logcheck will check for problematic values (for example, types that have an incomplete fmt.Stringer implementation).`)
logcheckFlags.BoolVar(c.enabled[deprecationsCheck], prefix+deprecationsCheck, true, `When true, logcheck will analyze the usage of deprecated Klog function calls.`)
logcheckFlags.Var(&c.fileOverrides, "config", `A file which overrides the global settings for checks on a per-file basis via regular expressions.`)

Expand Down Expand Up @@ -145,6 +148,9 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
fName := selExpr.Sel.Name

filename := pass.Pkg.Path() + "/" + path.Base(pass.Fset.Position(fexpr.Pos()).Filename)
valueCheckEnabled := c.isEnabled(valueCheck, filename)
keyCheckEnabled := c.isEnabled(keyCheck, filename)
parametersCheckEnabled := c.isEnabled(parametersCheck, filename)

// Now we need to determine whether it is coming from klog.
if isKlog(selExpr.X, pass) {
Expand Down Expand Up @@ -176,50 +182,37 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
return
}

if c.isEnabled(parametersCheck, filename) {
if keyCheckEnabled || parametersCheckEnabled || valueCheckEnabled {
// if format specifier is used, check for arg length will most probably fail
// so check for format specifier first and skip if found
if checkForFormatSpecifier(fexpr, pass) {
if parametersCheckEnabled && checkForFormatSpecifier(fexpr, pass) {
return
}
if fName == "InfoS" {
isKeysValid(args[1:], fun, pass, fName)
} else if fName == "ErrorS" {
isKeysValid(args[2:], fun, pass, fName)
}

// Also check structured calls.
if c.isEnabled(parametersCheck, filename) {
checkForFormatSpecifier(fexpr, pass)
switch fName {
case "InfoS":
kvCheck(args[1:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled)
case "ErrorS":
kvCheck(args[2:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled)
}
}
// verbosity Zero Check
if c.isEnabled(verbosityZeroCheck, filename) {
checkForVerbosityZero(fexpr, pass)
}
// key Check
if c.isEnabled(keyCheck, filename) {
} else if isGoLogger(selExpr.X, pass) {
if keyCheckEnabled || parametersCheckEnabled || valueCheckEnabled {
// if format specifier is used, check for arg length will most probably fail
// so check for format specifier first and skip if found
if checkFormatSpecifier(fexpr, pass) {
if parametersCheckEnabled && checkForFormatSpecifier(fexpr, pass) {
return
}
if fName == "InfoS" {
keysCheck(args[1:], fun, pass, fName)
} else if fName == "ErrorS" {
keysCheck(args[2:], fun, pass, fName)
}
}
} else if isGoLogger(selExpr.X, pass) {
if c.isEnabled(parametersCheck, filename) {
checkForFormatSpecifier(fexpr, pass)
switch fName {
case "WithValues":
isKeysValid(args, fun, pass, fName)
kvCheck(args, fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled)
case "Info":
isKeysValid(args[1:], fun, pass, fName)
kvCheck(args[1:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled)
case "Error":
isKeysValid(args[2:], fun, pass, fName)
kvCheck(args[2:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled)
}
}
if c.isEnabled(withHelpersCheck, filename) {
Expand All @@ -235,22 +228,6 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
if c.isEnabled(verbosityZeroCheck, filename) {
checkForVerbosityZero(fexpr, pass)
}
// key Check
if c.isEnabled(keyCheck, filename) {
// if format specifier is used, check for arg length will most probably fail
// so check for format specifier first and skip if found
if checkFormatSpecifier(fexpr, pass) {
return
}
switch fName {
case "WithValues":
keysCheck(args, fun, pass, fName)
case "Info":
keysCheck(args[1:], fun, pass, fName)
case "Error":
keysCheck(args[2:], fun, pass, fName)
}
}
} else if fName == "NewContext" &&
isPackage(selExpr.X, "github.com/go-logr/logr", pass) &&
c.isEnabled(withHelpersCheck, filename) {
Expand Down Expand Up @@ -395,45 +372,6 @@ func isContextualCall(fName string) bool {
return false
}

// isKeysValid check if all keys in keyAndValues is string type
func isKeysValid(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string) {
if len(keyValues)%2 != 0 {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Additional arguments to %s should always be Key Value pairs. Please check if there is any key or value missing.", funName),
})
return
}

for index, arg := range keyValues {
if index%2 != 0 {
continue
}
lit, ok := arg.(*ast.BasicLit)
if !ok {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg),
})
continue
}
if lit.Kind != token.STRING {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value),
})
continue
}
isASCII := utf8string.NewString(lit.Value).IsASCII()
if !isASCII {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Key positional arguments %s are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.", lit.Value),
})
}
}
}

func checkForFormatSpecifier(expr *ast.CallExpr, pass *analysis.Pass) bool {
if selExpr, ok := expr.Fun.(*ast.SelectorExpr); ok {
// extracting function Name like Infof
Expand Down Expand Up @@ -615,17 +553,9 @@ func isVerbosityZero(expr ast.Expr) bool {
return false
}

func checkFormatSpecifier(expr *ast.CallExpr, pass *analysis.Pass) bool {
if _, ok := expr.Fun.(*ast.SelectorExpr); ok {
if _, found := hasFormatSpecifier(expr.Args); found {
return true
}
}
return false
}

// keysCheck check if all keys in keyAndValues are valid keys according to the guidelines.
func keysCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string) {
// kvCheck check if all keys in keyAndValues are valid keys according to the guidelines
// and that the values can be formatted.
func kvCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled bool) {
if len(keyValues)%2 != 0 {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Expand All @@ -635,31 +565,120 @@ func keysCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName
}

for index, arg := range keyValues {
if index%2 != 0 {
continue
}
lit, ok := arg.(*ast.BasicLit)
if !ok {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg),
})
continue
switch index % 2 {
case 0:
// Key in key/value pair.
checkKey(arg, pass, keyCheckEnabled, parametersCheckEnabled)
case 1:
// Value in key/value pair.
checkValue(arg, pass, valueCheckEnabled)
}
if lit.Kind != token.STRING {
}
}

// checkKey checks the key in a key/value pair.
func checkKey(arg ast.Expr, pass *analysis.Pass, keyCheckEnabled, parametersCheckEnabled bool) {
if !keyCheckEnabled && !parametersCheckEnabled {
return
}

lit, ok := arg.(*ast.BasicLit)
if !ok {
pass.Report(analysis.Diagnostic{
Pos: arg.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg),
})
return
}

if lit.Kind != token.STRING {
pass.Report(analysis.Diagnostic{
Pos: arg.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value),
})
return
}

switch {
case parametersCheckEnabled:
// This is the less strict check.
isASCII := utf8string.NewString(lit.Value).IsASCII()
if !isASCII {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value),
Pos: arg.Pos(),
Message: fmt.Sprintf("Key positional arguments %s are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.", lit.Value),
})
continue
}
case keyCheckEnabled:
// This is the stricter check.
keyMatchRe := regexp.MustCompile(`(^[A-Z]{2,}|^[a-z])[[:alnum:]]*$`)
match := keyMatchRe.Match([]byte(strings.Trim(lit.Value, "\"")))
if !match {
pass.Report(analysis.Diagnostic{
Pos: fun.Pos(),
Pos: arg.Pos(),
Message: fmt.Sprintf("Key positional arguments %s are expected to be alphanumeric and start with either one lowercase or two uppercase letters. Please refer to https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments.", lit.Value),
})
}
}
}

// checkValue checks the value in a key/value pair.
func checkValue(arg ast.Expr, pass *analysis.Pass, valueCheckEnabled bool) {
if !valueCheckEnabled {
return
}

// Check the type.
if typeAndValue, ok := pass.TypesInfo.Types[arg]; ok {
if obj, index, _ := types.LookupFieldOrMethod(typeAndValue.Type, typeAndValue.Addressable(), nil /* package */, "String"); obj != nil {
if function, ok := obj.(*types.Func); ok && isFmtString(function) && len(index) > 1 && !isWrapperStruct(typeAndValue.Type) {
pass.Report(analysis.Diagnostic{
Pos: arg.Pos(),
Message: fmt.Sprintf("The type %s inherits %s as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO.", typeAndValue.Type.String(), function.FullName()), // TODO: https://github.com/kubernetes/kubernetes/pull/116952
})
}
}
}
}

// isFmtString checks whether the function has the "func() string" signature.
func isFmtString(function *types.Func) bool {
signature, ok := function.Type().(*types.Signature)
if !ok {
return false
}
params := signature.Params()
if params != nil && params.Len() != 0 {
return false
}
results := signature.Results()
if results == nil || results.Len() != 1 {
return false
}
result := results.At(0)
basic, ok := result.Type().(*types.Basic)
if !ok {
return false
}
if basic.Kind() != types.String {
return false
}
return true
}

// isWrapperStruct returns true for types that are a struct with a single field
// or a pointer to one, like for example
// https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Time.
func isWrapperStruct(t types.Type) bool {
if ptr, ok := t.(*types.Pointer); ok {
t = ptr.Elem()
}
if named, ok := t.(*types.Named); ok {
t = named.Underlying()
}
if strct, ok := t.(*types.Struct); ok {
return strct.NumFields() == 1
}

return false
}
40 changes: 40 additions & 0 deletions logcheck/testdata/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
Copyright 2023 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1

import (
"fmt"
"strings"
)

type TypeMeta struct {
Kind string `json:"kind,omitempty" protobuf:"bytes,1,opt,name=kind"`
APIVersion string `json:"apiVersion,omitempty" protobuf:"bytes,2,opt,name=apiVersion"`
}

// String is generated (https://github.com/kubernetes/apimachinery/blob/v0.26.3/pkg/apis/meta/v1/generated.pb.go#L4757).
func (this *TypeMeta) String() string {
if this == nil {
return "nil"
}
s := strings.Join([]string{`&TypeMeta{`,
`Kind:` + fmt.Sprintf("%v", this.Kind) + `,`,
`APIVersion:` + fmt.Sprintf("%v", this.APIVersion) + `,`,
`}`,
}, "")
return s
}
Loading

0 comments on commit ff0f1e9

Please sign in to comment.