New Checks: Additional Custom SchemaValidateFunc Checks for SDK Valid…

…ation Functions (#231)

Reference: #218

CHANGELOG.md

@@ -5,6 +5,10 @@ FEATURES
 * **New Check:** `AT010`: check for `TestCase` including `IDRefreshName` implementation
 * **New Check:** `AT011`: check for `TestCase` including `IDRefreshIgnore` implementation without `IDRefreshName`
 * **New Check:** `AT012`: check for files containing multiple acceptance test function name prefixes
+* **New Check:** `V011`: check for custom `SchemaValidateFunc` that implement `validation.StringLenBetween()`
+* **New Check:** `V012`: check for custom `SchemaValidateFunc` that implement `validation.IntAtLeast()`, `validation.IntAtMost()`, or `validation.IntBetween()`
+* **New Check:** `V013`: check for custom `SchemaValidateFunc` that implement `validation.StringInSlice()` or `validation.StringNotInSlice()`
+* **New Check:** `V014`: check for custom `SchemaValidateFunc` that implement `validation.IntInSlice()` or `validation.IntNotInSlice()`
 
 # v0.23.0
 

README.md

@@ -187,6 +187,10 @@ Standard lint checks are enabled by default in the `tfproviderlint` tool. Opt-in
 | [V008](passes/V008) | check for deprecated `ValidateRFC3339TimeString` validation function usage | AST |
 | [V009](passes/V009) | check for `validation.StringMatch()` call with empty message argument | AST |
 | [V010](passes/V010) | check for `validation.StringDoesNotMatch()` call with empty message argument | AST |
+| [V011](passes/V011) | check for custom `SchemaValidateFunc` that implement `validation.StringLenBetween()` | AST |
+| [V012](passes/V012) | check for custom `SchemaValidateFunc` that implement `validation.IntAtLeast()`, `validation.IntAtMost()`, or `validation.IntBetween()` | AST |
+| [V013](passes/V013) | check for custom `SchemaValidateFunc` that implement `validation.StringInSlice()` or `validation.StringNotInSlice()` | AST |
+| [V014](passes/V014) | check for custom `SchemaValidateFunc` that implement `validation.IntInSlice()` or `validation.IntNotInSlice()` | AST |
 
 ## Extra Lint Checks
 

passes/V011/README.md

@@ -0,0 +1,50 @@
+# V011
+
+The V011 analyzer reports when custom SchemaValidateFunc declarations can be
+replaced with [validation.StringLenBetween()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation#StringLenBetween).
+
+## Flagged Code
+
+```go
+func validateExampleThing(v interface{}, k string) (ws []string, errors []error) {
+  value := v.(string)
+
+  if len(value) < 1 || len(value) > 255 {
+    errors = append(errors, fmt.Errorf("%q must be between 1 and 255 characters: %q", k, value))
+  }
+
+  return
+}
+```
+
+## Passing Code
+
+```go
+// directly in Schema
+ValidateFunc: validation.StringLenBetween(1, 255),
+
+// or saving as a variable
+var validateExampleThing = validation.StringLenBetween(1, 255)
+
+// or replacing the function
+func validateExampleThing() schema.SchemaValidateFunc {
+  return validation.StringLenBetween(1, 255)
+}
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:V011` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:V011
+func validateExampleThing(v interface{}, k string) (ws []string, errors []error) {
+  value := v.(string)
+
+  if len(value) < 1 || len(value) > 255 {
+    errors = append(errors, fmt.Errorf("%q must be between 1 and 255 characters: %q", k, value))
+  }
+
+  return
+}
+```

passes/V011/V011.go

@@ -0,0 +1,139 @@
+package V011
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"github.com/bflad/tfproviderlint/passes/helper/schema/schemavalidatefuncinfo"
+	"golang.org/x/tools/go/analysis"
+)
+
+const Doc = `check for custom SchemaValidateFunc that implement validation.StringLenBetween()
+
+The V011 analyzer reports when custom SchemaValidateFunc declarations can be
+replaced with validation.StringLenBetween().`
+
+const analyzerName = "V011"
+
+var Analyzer = &analysis.Analyzer{
+	Name: analyzerName,
+	Doc:  Doc,
+	Requires: []*analysis.Analyzer{
+		commentignore.Analyzer,
+		schemavalidatefuncinfo.Analyzer,
+	},
+	Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	schemaValidateFuncs := pass.ResultOf[schemavalidatefuncinfo.Analyzer].([]*schema.SchemaValidateFuncInfo)
+
+	for _, schemaValidateFunc := range schemaValidateFuncs {
+		if ignorer.ShouldIgnore(analyzerName, schemaValidateFunc.Node) {
+			continue
+		}
+
+		if !hasIfStringLenCheck(schemaValidateFunc.Body, pass.TypesInfo) {
+			continue
+		}
+
+		pass.Reportf(schemaValidateFunc.Pos, "%s: custom SchemaValidateFunc should be replaced with validation.StringLenBetween()", analyzerName)
+	}
+
+	return nil, nil
+}
+
+func hasIfStringLenCheck(node ast.Node, info *types.Info) bool {
+	result := false
+
+	ast.Inspect(node, func(n ast.Node) bool {
+		switch n := n.(type) {
+		default:
+			return true
+		case *ast.IfStmt:
+			if !hasStringLenCheck(n, info) {
+				return true
+			}
+
+			result = true
+
+			return false
+		}
+	})
+
+	return result
+}
+
+func hasStringLenCheck(node ast.Node, info *types.Info) bool {
+	result := false
+
+	ast.Inspect(node, func(n ast.Node) bool {
+		binaryExpr, ok := n.(*ast.BinaryExpr)
+
+		if !ok {
+			return true
+		}
+
+		if !exprIsStringLenCallExpr(binaryExpr.X, info) && !exprIsStringLenCallExpr(binaryExpr.Y, info) {
+			return true
+		}
+
+		if !tokenIsLenCheck(binaryExpr.Op) {
+			return true
+		}
+
+		result = true
+
+		return false
+	})
+
+	return result
+}
+
+func exprIsStringLenCallExpr(e ast.Expr, info *types.Info) bool {
+	switch e := e.(type) {
+	default:
+		return false
+	case *ast.CallExpr:
+		switch fun := e.Fun.(type) {
+		default:
+			return false
+		case *ast.Ident:
+			if fun.Name != "len" {
+				return false
+			}
+		}
+
+		if len(e.Args) != 1 {
+			return false
+		}
+
+		switch arg := info.TypeOf(e.Args[0]).Underlying().(type) {
+		default:
+			return false
+		case *types.Basic:
+			return arg.Kind() == types.String
+		}
+	}
+}
+
+func tokenIsLenCheck(t token.Token) bool {
+	validTokens := []token.Token{
+		token.GEQ, // >=
+		token.GTR, // >
+		token.LEQ, // <=
+		token.LSS, // <
+	}
+
+	for _, validToken := range validTokens {
+		if t == validToken {
+			return true
+		}
+	}
+
+	return false
+}

passes/V011/V011_test.go

@@ -0,0 +1,13 @@
+package V011_test
+
+import (
+	"testing"
+
+	"github.com/bflad/tfproviderlint/passes/V011"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestV011(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, V011.Analyzer, "a")
+}

passes/V011/testdata/src/a/main.go

@@ -0,0 +1,113 @@
+package a
+
+import (
+	"errors"
+)
+
+//lintignore:V011
+func commentIgnore(v interface{}, k string) (warns []string, errs []error) {
+	value := v.(string)
+
+	if len(value) > 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func failingAnonymousFunction() {
+	_ = func(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+		value := v.(string)
+
+		if len(value) > 255 {
+			errs = append(errs, errors.New("example"))
+		}
+
+		return
+	}
+}
+
+func failingSchemaValidateFuncGreaterThan(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+	value := v.(string)
+
+	if len(value) > 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func failingSchemaValidateFuncGreaterThanOrEqual(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+	value := v.(string)
+
+	if len(value) >= 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func failingSchemaValidateFuncLessThan(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+	value := v.(string)
+
+	if len(value) < 3 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func failingSchemaValidateFuncLessThanOrEqual(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+	value := v.(string)
+
+	if len(value) <= 3 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func failingSchemaValidateFuncMultiple(v interface{}, k string) (warns []string, errs []error) { // want "custom SchemaValidateFunc should be replaced with validation.StringLenBetween\\(\\)"
+	value := v.(string)
+
+	if len(value) < 3 || len(value) > 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func passingAnonymousFunction() {
+	_ = func(v interface{}, k string) (warns []string, errs []error) {
+		return
+	}
+}
+
+func passingLen() {
+	var value string
+	_ = len(value) > 255
+}
+
+func passingSchemaValidateFunc(v interface{}, k string) (warns []string, errs []error) {
+	return
+}
+
+func passingSchemaValidateFuncNonString1(v interface{}, k string) (warns []string, errs []error) {
+	var test []string
+
+	if len(test) > 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}
+
+func passingSchemaValidateFuncNonString2(v interface{}, k string) (warns []string, errs []error) {
+	var test map[string]string
+
+	if len(test) > 255 {
+		errs = append(errs, errors.New("example"))
+	}
+
+	return
+}

passes/V012/README.md

@@ -0,0 +1,50 @@
+# V012
+
+The V012 analyzer reports when custom SchemaValidateFunc declarations can be
+replaced with [validation.IntAtLeast()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation#IntAtLeast), [validation.IntAtMost()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation#IntAtMost), or [validation.IntBetween()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation#IntBetween).
+
+## Flagged Code
+
+```go
+func validateExampleThing(v interface{}, k string) (ws []string, errors []error) {
+  value := v.(int)
+
+  if value < 1 || value > 255 {
+    errors = append(errors, fmt.Errorf("%q must be between 1 and 255: %d", k, value))
+  }
+
+  return
+}
+```
+
+## Passing Code
+
+```go
+// directly in Schema
+ValidateFunc: validation.IntBetween(1, 255),
+
+// or saving as a variable
+var validateExampleThing = validation.IntBetween(1, 255)
+
+// or replacing the function
+func validateExampleThing() schema.SchemaValidateFunc {
+  return validation.IntBetween(1, 255)
+}
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:V012` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:V012
+func validateExampleThing(v interface{}, k string) (ws []string, errors []error) {
+  value := v.(int)
+
+  if value < 1 || value > 255 {
+    errors = append(errors, fmt.Errorf("%q must be between 1 and 255: %d", k, value))
+  }
+
+  return
+}
+```