Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check for interface inheritance #15

Merged
merged 2 commits into from
May 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions logcheck/main_test.go
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
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
}
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