From e4a7b8241231d38a4795ba24d7f0ec430074d0e4 Mon Sep 17 00:00:00 2001 From: kt Date: Wed, 9 Sep 2026 20:36:39 -0700 Subject: [PATCH] add AZS009: computed-only fields must not set input-only schema attributes --- CHANGELOG.md | 1 + README.md | 1 + checks/AZR/AZR009_lifecycle_logging/AZR009.go | 36 +-- checks/AZS/AZS.go | 2 + .../AZS009.go | 251 ++++++++++++++++++ .../AZS009_test.go | 16 ++ .../README.md | 114 ++++++++ .../testdata/src/azs009/azs009.go | 177 ++++++++++++ .../testdata/src/azs009/azs009.go.golden | 164 ++++++++++++ .../src/azs009/pluginsdk/pluginsdk.go | 17 ++ .../testdata/src/azs009/schema/schema.go | 62 +++++ lib/astx/edit.go | 36 +++ 12 files changed, 846 insertions(+), 31 deletions(-) create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/AZS009.go create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/AZS009_test.go create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/README.md create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go.golden create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/pluginsdk/pluginsdk.go create mode 100644 checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/schema/schema.go create mode 100644 lib/astx/edit.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e303fd3..6f8ac81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- add rule `AZS009`: computed-only schema fields must not set input-only attributes (`ValidateFunc`, `MaxItems`, `Default`, `ConflictsWith`, ...) and fields nested in a computed-only block or a typed resource's `Attributes()` must not be `Optional`/`Required`; the plugin SDK misses the nested cases; fixable with `-fix` - add rule `AZR009`: no lifecycle narration logging (`log.Printf("[DEBUG] Creating %s", id)`, `metadata.Logger.Info("Decoding state..")`) inside Create/Read/Update/Delete functions — hashicorp/terraform-provider-azurerm#32423 removed the pattern; ID-rewrite, not-found, and waiting messages are kept; fixable with `-fix`, which also drops a `log` import left unused; `lib/lifecycle` finds lifecycle functions for AZR003 and AZR009 - ci: dev tools pinned in `.tools/go.mod` and built into `.tools/bin` by make (golangci-lint, actionlint, gofumpt; shellcheck and yamllint pinned in the makefile); `make lint` runs a custom golangci-lint with this repo's own AZG rules compiled in; revive enabled with justified opt-outs; new `actionlint`, `yamllint`, `shellcheck` targets and workflows; `check-all` runs everything - ci: GitHub-owned actions pinned by commit hash; CodeQL scoped to `contents: read`; release `contents: write` scoped to the goreleaser job; `pr-*` workflow names; depscheck also verifies `.tools` tidy and the golangci version match with `.custom-gcl.yml` diff --git a/README.md b/README.md index 489ec7c..70c183f 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,7 @@ Rules are named `AZ`, aligned with [tfproviderlint](htt | [AZS006](checks/AZS/AZS006_data_source_missing_properties) | data sources must expose their same-named resource's properties | | [AZS007](checks/AZS/AZS007_optional_computed_missing_comment) | optional+computed fields must have a Note: O+C comment | | [AZS008](checks/AZS/AZS008_registration_entries_sorted) | registration entries must be sorted alphabetically | +| [AZS009](checks/AZS/AZS009_computed_only_field_input_attributes) | computed-only fields must not set input-only schema attributes | ### AZC — Clients & SDK Usage diff --git a/checks/AZR/AZR009_lifecycle_logging/AZR009.go b/checks/AZR/AZR009_lifecycle_logging/AZR009.go index d677f6f..33aec69 100644 --- a/checks/AZR/AZR009_lifecycle_logging/AZR009.go +++ b/checks/AZR/AZR009_lifecycle_logging/AZR009.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" + "github.com/katbyte/azproviderlint/lib/astx" "github.com/katbyte/azproviderlint/lib/lifecycle" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -41,7 +42,7 @@ var ( narration = regexp.MustCompile(`(?i)^(preparing (the )?arguments|creating|updating|deleting|retrieving|reading|importing|import check|decoding( the)? state|checking for( the| an| a)? (presence|existence|existing)|created|updated|deleted|retrieved)\b`) // keep matches messages that carry information beyond narration: ID rewrites (a state // migration), state removal, polling, and a step being skipped. - keep = regexp.MustCompile(`(?i)\bid\b|not found|removing from state|does not exist|waiting|skipp`) + keep = regexp.MustCompile(`(?i)\bid\b|not found|removing from state|does not exist|waiting|skip`) ) func run(pass *analysis.Pass) (any, error) { @@ -100,7 +101,7 @@ func run(pass *analysis.Pass) (any, error) { } } for i, f := range findings { - edits := []analysis.TextEdit{deleteLine(pass, f.stmt)} + edits := []analysis.TextEdit{astx.DeleteLine(pass, f.stmt)} if i == len(findings)-1 && importEdit != nil { edits = append(edits, *importEdit) } @@ -167,43 +168,16 @@ func deleteLogImport(pass *analysis.Pass, file *ast.File) *analysis.TextEdit { continue } if len(gen.Specs) == 1 { - e := deleteLine(pass, gen) + e := astx.DeleteLine(pass, gen) return &e } - e := deleteLine(pass, imp) + e := astx.DeleteLine(pass, imp) return &e } } return nil } -// deleteLine returns the edit removing node together with the rest of its line (a trailing -// comment included), so no blank line is left behind; when other code shares the line only -// the node itself goes. -func deleteLine(pass *analysis.Pass, node ast.Node) analysis.TextEdit { - tf := pass.Fset.File(node.Pos()) - start, end := node.Pos(), node.End() - if tf != nil { - line := tf.Line(start) - lineStart := tf.LineStart(line) - var lineEnd token.Pos - if line < tf.LineCount() { - lineEnd = tf.LineStart(line + 1) - } else { - lineEnd = token.Pos(tf.Base() + tf.Size()) - } - if content, err := pass.ReadFile(tf.Name()); err == nil { - before := strings.TrimSpace(string(content[tf.Offset(lineStart):tf.Offset(start)])) - after := strings.TrimSpace(string(content[tf.Offset(end):tf.Offset(lineEnd)])) - // a trailing comment belongs to the statement and goes with it - if before == "" && (after == "" || strings.HasPrefix(after, "//")) { - start, end = lineStart, lineEnd - } - } - } - return analysis.TextEdit{Pos: start, End: end} -} - // enclosingFile returns the file in the pass containing pos. func enclosingFile(pass *analysis.Pass, pos token.Pos) *ast.File { for _, f := range pass.Files { diff --git a/checks/AZS/AZS.go b/checks/AZS/AZS.go index 34ea745..f6054bc 100644 --- a/checks/AZS/AZS.go +++ b/checks/AZS/AZS.go @@ -12,6 +12,7 @@ import ( AZS006 "github.com/katbyte/azproviderlint/checks/AZS/AZS006_data_source_missing_properties" AZS007 "github.com/katbyte/azproviderlint/checks/AZS/AZS007_optional_computed_missing_comment" AZS008 "github.com/katbyte/azproviderlint/checks/AZS/AZS008_registration_entries_sorted" + AZS009 "github.com/katbyte/azproviderlint/checks/AZS/AZS009_computed_only_field_input_attributes" ) // Checks contains all AZS (schema & typed SDK model) analyzers. @@ -24,4 +25,5 @@ var Checks = []*analysis.Analyzer{ AZS006.Analyzer, AZS007.Analyzer, AZS008.Analyzer, + AZS009.Analyzer, } diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009.go b/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009.go new file mode 100644 index 0000000..9b2ad15 --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009.go @@ -0,0 +1,251 @@ +// Package AZS009 defines an analyzer that reports computed-only schema fields setting +// attributes that only apply to user input, and Optional/Required fields nested inside a +// computed-only block. +package AZS009 + +import ( + "fmt" + "go/ast" + "go/constant" + "go/token" + "go/types" + "slices" + + "github.com/katbyte/azproviderlint/lib/astx" + "github.com/katbyte/azproviderlint/lib/tf" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Analyzer checks schema fields that are computed-only (Computed without Optional or Required, +// anything nested in such a block, and everything in a typed resource's Attributes()) for +// attributes that describe user input: validation, defaults, item counts, conflicts, and the +// Optional/Required flags themselves. The plugin SDK rejects most of these at provider start, +// but it does not see fields nested inside a computed-only block, or an element schema's +// ValidateFunc, and a lint catches all of them at the desk. +var Analyzer = &analysis.Analyzer{ + Name: "AZS009", + Doc: "check for computed-only schema fields that set input-only attributes (ValidateFunc, MaxItems, Default, ...) or nest Optional/Required fields", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZS/AZS009_computed_only_field_input_attributes/README.md", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +// inputOnly lists the Schema fields that only mean something for a value the user writes: how it +// is validated, defaulted, normalised, counted, or constrained against other arguments. It is the +// plugin SDK's own computed-only rejection list plus RequiredWith and WriteOnly. +var inputOnly = []string{ + "Default", "DefaultFunc", "InputDefault", + "ValidateFunc", "ValidateDiagFunc", + "DiffSuppressFunc", "DiffSuppressOnRefresh", "StateFunc", + "MaxItems", "MinItems", + "AtLeastOneOf", "ExactlyOneOf", "ConflictsWith", "RequiredWith", + "WriteOnly", +} + +const ( + onField = "on a computed-only field" + inBlock = "inside a computed-only block" + onElem = "on the element of a computed-only field" + inAttributes = "in Attributes(), where every field is computed" +) + +func run(pass *analysis.Pass) (any, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, nil + } + + // nested literals are handled from the block that makes them computed-only, so the + // walk must not visit them again on their own + visited := map[*ast.CompositeLit]bool{} + + insp.WithStack([]ast.Node{(*ast.CompositeLit)(nil)}, func(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return false + } + cl, ok := n.(*ast.CompositeLit) + if !ok || visited[cl] || !tf.IsSchemaHelperType(pass, cl, "Schema") { + return true + } + kvs := keyValues(cl) + switch { + case astx.IsTrueConstant(pass, value(kvs, "Computed")) && + !astx.IsTrueConstant(pass, value(kvs, "Optional")) && + !astx.IsTrueConstant(pass, value(kvs, "Required")): + check(pass, cl, visited, onField, false) + case inAttributesMethod(pass, stack): + check(pass, cl, visited, inAttributes, true) + } + return true + }) + + return nil, nil +} + +// check reports what cl sets that a computed-only field has no use for, then descends into +// its Elem: everything under a computed-only block is computed-only too. flags also reports +// Optional/Required on cl itself, which is only wrong where cl is not the field that said +// Computed. +func check(pass *analysis.Pass, cl *ast.CompositeLit, visited map[*ast.CompositeLit]bool, where string, flags bool) { + visited[cl] = true + kvs := keyValues(cl) + + if flags { + for _, name := range []string{"Optional", "Required"} { + kv := kvs[name] + if kv == nil || !astx.IsTrueConstant(pass, kv.Value) { + continue + } + d := analysis.Diagnostic{ + Pos: kv.Key.Pos(), + Message: fmt.Sprintf("%s has no effect %s - use Computed", name, where), + } + // in Attributes() the field belongs in Arguments() instead, which is a person's call + if where != inAttributes { + if edit, ok := flagFix(pass, kvs, kv); ok { + d.SuggestedFixes = []analysis.SuggestedFix{{Message: "Replace with Computed", TextEdits: []analysis.TextEdit{edit}}} + } + } + pass.Report(d) + } + } + + for _, name := range inputOnly { + kv := kvs[name] + if kv == nil || !isSet(pass, kv.Value) { + continue + } + pass.Report(analysis.Diagnostic{ + Pos: kv.Key.Pos(), + Message: fmt.Sprintf("%s has no effect %s - remove it", name, where), + SuggestedFixes: []analysis.SuggestedFix{{Message: "Remove " + name, TextEdits: []analysis.TextEdit{astx.DeleteLine(pass, kv)}}}, + }) + } + if kv := kvs["ConfigMode"]; kv != nil && selectorName(kv.Value) == "SchemaConfigModeBlock" { + pass.Report(analysis.Diagnostic{ + Pos: kv.Key.Pos(), + Message: fmt.Sprintf("ConfigMode: SchemaConfigModeBlock has no effect %s - remove it", where), + SuggestedFixes: []analysis.SuggestedFix{{Message: "Remove ConfigMode", TextEdits: []analysis.TextEdit{astx.DeleteLine(pass, kv)}}}, + }) + } + + elem, ok := stripAddr(value(kvs, "Elem")).(*ast.CompositeLit) + if !ok { + return + } + switch { + case tf.IsSchemaHelperType(pass, elem, "Schema"): + check(pass, elem, visited, onElem, false) + case tf.IsSchemaHelperType(pass, elem, "Resource"): + props, ok := value(keyValues(elem), "Schema").(*ast.CompositeLit) + if !ok { + return + } + for _, elt := range props.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + if prop, ok := stripAddr(kv.Value).(*ast.CompositeLit); ok && tf.IsSchemaHelperType(pass, prop, "Schema") { + check(pass, prop, visited, inBlock, true) + } + } + } +} + +// flagFix turns a nested field's Optional/Required into Computed: the key is renamed when the +// literal has no Computed yet, and the line goes when it already does. +func flagFix(pass *analysis.Pass, kvs map[string]*ast.KeyValueExpr, kv *ast.KeyValueExpr) (analysis.TextEdit, bool) { + if kvs["Computed"] != nil { + return astx.DeleteLine(pass, kv), true + } + if id, ok := kv.Value.(*ast.Ident); !ok || id.Name != "true" { + return analysis.TextEdit{}, false + } + return analysis.TextEdit{Pos: kv.Key.Pos(), End: kv.Key.End(), NewText: []byte("Computed")}, true +} + +// inAttributesMethod reports whether the stack sits inside a method named Attributes returning +// a map of schema pointers: azurerm's typed SDK marks every entry Computed at start-up. +func inAttributesMethod(pass *analysis.Pass, stack []ast.Node) bool { + for _, n := range slices.Backward(stack) { + fn, ok := n.(*ast.FuncDecl) + if !ok { + continue + } + if fn.Name.Name != "Attributes" || fn.Recv == nil || fn.Type.Results == nil || len(fn.Type.Results.List) != 1 { + return false + } + m, ok := types.Unalias(pass.TypesInfo.TypeOf(fn.Type.Results.List[0].Type)).(*types.Map) + if !ok { + return false + } + ptr, ok := types.Unalias(m.Elem()).(*types.Pointer) + if !ok { + return false + } + named, ok := types.Unalias(ptr.Elem()).(*types.Named) + return ok && named.Obj().Name() == "Schema" && named.Obj().Pkg() != nil && named.Obj().Pkg().Name() == "schema" + } + return false +} + +// isSet reports whether e gives the attribute a value: not nil, and not a zero constant. +func isSet(pass *analysis.Pass, e ast.Expr) bool { + tv := pass.TypesInfo.Types[e] + if tv.IsNil() { + return false + } + if tv.Value == nil { + return true + } + switch tv.Value.Kind() { + case constant.Bool: + return constant.BoolVal(tv.Value) + case constant.String: + return constant.StringVal(tv.Value) != "" + case constant.Int, constant.Float: + return constant.Sign(tv.Value) != 0 + case constant.Complex, constant.Unknown: + return true + } + return true +} + +func keyValues(cl *ast.CompositeLit) map[string]*ast.KeyValueExpr { + kvs := make(map[string]*ast.KeyValueExpr, len(cl.Elts)) + for _, elt := range cl.Elts { + if kv, ok := elt.(*ast.KeyValueExpr); ok { + if key, ok := kv.Key.(*ast.Ident); ok { + kvs[key.Name] = kv + } + } + } + return kvs +} + +func value(kvs map[string]*ast.KeyValueExpr, name string) ast.Expr { + if kv := kvs[name]; kv != nil { + return kv.Value + } + return nil +} + +func stripAddr(e ast.Expr) ast.Expr { + if u, ok := e.(*ast.UnaryExpr); ok && u.Op == token.AND { + return u.X + } + return e +} + +func selectorName(e ast.Expr) string { + switch v := e.(type) { + case *ast.SelectorExpr: + return v.Sel.Name + case *ast.Ident: + return v.Name + } + return "" +} diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009_test.go b/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009_test.go new file mode 100644 index 0000000..9bcfbda --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/AZS009_test.go @@ -0,0 +1,16 @@ +package AZS009 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZS009(t *testing.T) { + t.Parallel() + + _, filename, _, _ := runtime.Caller(0) + analysistest.RunWithSuggestedFixes(t, filepath.Join(filepath.Dir(filename), "testdata"), Analyzer, "azs009") +} diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/README.md b/checks/AZS/AZS009_computed_only_field_input_attributes/README.md new file mode 100644 index 0000000..afd592d --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/README.md @@ -0,0 +1,114 @@ +# AZS009 - computed-only fields must not set input-only schema attributes + +AZS009 reports a computed-only schema field that sets something only user input can use, such as `ValidateFunc`, `MaxItems`, or `Default`. It also reports `Optional` or `Required` on a field nested inside a computed-only block. + +A computed-only field (`Computed: true` with no `Optional` or `Required`) is filled in by the provider from the API. The user never writes it, so there is nothing to validate, default, count, or check for conflicts. Those attributes are dead weight at best. The plugin SDK rejects most of them when the provider starts, but only on the field that says `Computed`. It does not notice that everything inside a computed-only block is computed-only too, and it does not look at an element schema's `ValidateFunc`. This check does. + +## Flagged Code + +```go +"status": { + Type: pluginsdk.TypeString, + Computed: true, + ValidateFunc: validation.StringIsNotEmpty, +}, +``` + +```go +"network": { + Type: pluginsdk.TypeList, + Computed: true, + MaxItems: 1, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "subnet_id": { + Type: pluginsdk.TypeString, + Optional: true, // nothing in this block can be configured + ValidateFunc: commonids.ValidateSubnetID, + }, + }, + }, +}, +``` + +```go +func (r ExampleResource) Attributes() map[string]*pluginsdk.Schema { + return map[string]*pluginsdk.Schema{ + "endpoint": { + Type: pluginsdk.TypeString, + ValidateFunc: validation.IsURLWithHTTPS, // Attributes() are all computed + }, + } +} +``` + +## Passing Code + +```go +"status": { + Type: pluginsdk.TypeString, + Computed: true, +}, +``` + +```go +"network": { + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "subnet_id": { + Type: pluginsdk.TypeString, + Computed: true, + }, + }, + }, +}, +``` + +```go +// optional+computed is configurable, so validation belongs +"name": { + Type: pluginsdk.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validation.StringIsNotEmpty, +}, +``` + +## What counts + +A field is computed-only when `Computed: true` is a constant and neither `Optional` nor `Required` is. Everything reached through its `Elem`, at any depth, is computed-only as well. In a typed resource, every field in `Attributes()` is computed-only because azurerm's SDK wrapper sets `Computed` on all of them at start-up. + +Reported on any such field: + +- `Default`, `DefaultFunc`, `InputDefault` +- `ValidateFunc`, `ValidateDiagFunc` +- `DiffSuppressFunc`, `DiffSuppressOnRefresh`, `StateFunc` +- `MaxItems`, `MinItems` +- `AtLeastOneOf`, `ExactlyOneOf`, `ConflictsWith`, `RequiredWith` +- `WriteOnly` +- `ConfigMode: SchemaConfigModeBlock` + +Reported on a field nested in a computed-only block, or listed in `Attributes()`: `Optional: true` and `Required: true`. + +Not reported: + +- attributes set to a zero value (`MaxItems: 0`, `Default: nil`) +- `Computed` set from a variable rather than a constant, since the field may not be computed-only +- an `Elem` that comes from a function call rather than a literal +- `ForceNew`, `Sensitive`, `Description`, `Deprecated`, and `Set`, which describe state and are fine on computed fields + +## The fix + +`-fix` deletes the attribute's line. For a nested `Optional` or `Required` it renames the key to `Computed`, or deletes the line when the field already says `Computed`. An `Optional` inside `Attributes()` gets no fix, because the right move is usually to the `Arguments()` map, and that is a person's call. + +## Ignoring Reports + +Put `//azignore:AZS009 - ` at the end of the attribute's line, or on the line above it. The reason is required. + +```go +ValidateFunc: validation.StringIsNotEmpty, //azignore:AZS009 - +``` + +Under golangci-lint, `//nolint:azproviderlint` in the same place also works, but it silences every azproviderlint check on that line. diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go new file mode 100644 index 0000000..4abd600 --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go @@ -0,0 +1,177 @@ +package azs009 + +import ( + "azs009/pluginsdk" + "azs009/schema" +) + +func validateString(any, string) ([]string, []error) { return nil, nil } + +func suppress(string, string, string, any) bool { return false } + +func flaggedTopLevel() { + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + ValidateFunc: validateString, // want `ValidateFunc has no effect on a computed-only field - remove it` + } + + _ = &pluginsdk.Schema{ + Type: pluginsdk.TypeList, + Computed: true, + MaxItems: 1, // want `MaxItems has no effect on a computed-only field - remove it` + MinItems: 1, // want `MinItems has no effect on a computed-only field - remove it` + Elem: &pluginsdk.Schema{Type: pluginsdk.TypeString}, + } + + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + Default: "x", // want `Default has no effect on a computed-only field - remove it` + DiffSuppressFunc: suppress, // want `DiffSuppressFunc has no effect on a computed-only field - remove it` + ConflictsWith: []string{"other"}, // want `ConflictsWith has no effect on a computed-only field - remove it` + WriteOnly: true, // want `WriteOnly has no effect on a computed-only field - remove it` + } + + _ = &schema.Schema{ + Type: schema.TypeList, + Computed: true, + ConfigMode: schema.SchemaConfigModeBlock, // want `ConfigMode: SchemaConfigModeBlock has no effect on a computed-only field - remove it` + Elem: &schema.Resource{Schema: map[string]*schema.Schema{}}, + } +} + +func flaggedNested() { + _ = &pluginsdk.Schema{ + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "name": { + Type: pluginsdk.TypeString, + Optional: true, // want `Optional has no effect inside a computed-only block - use Computed` + ValidateFunc: validateString, // want `ValidateFunc has no effect inside a computed-only block - remove it` + }, + "enabled": { + Type: pluginsdk.TypeBool, + Optional: true, // want `Optional has no effect inside a computed-only block - use Computed` + Computed: true, + }, + "count": { + Type: pluginsdk.TypeInt, + Required: true, // want `Required has no effect inside a computed-only block - use Computed` + }, + "inner": { + Type: pluginsdk.TypeList, + Computed: true, + MaxItems: 1, // want `MaxItems has no effect inside a computed-only block - remove it` + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "deep": { + Type: pluginsdk.TypeString, + Optional: true, // want `Optional has no effect inside a computed-only block - use Computed` + }, + }, + }, + }, + }, + }, + } + + _ = &schema.Schema{ + Type: schema.TypeSet, + Computed: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateFunc: validateString, // want `ValidateFunc has no effect on the element of a computed-only field - remove it` + }, + } +} + +type typedResource struct{} + +func (typedResource) Arguments() map[string]*pluginsdk.Schema { + return map[string]*pluginsdk.Schema{ + "name": { + Type: pluginsdk.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validateString, + }, + } +} + +func (typedResource) Attributes() map[string]*pluginsdk.Schema { + return map[string]*pluginsdk.Schema{ + "id": { + Type: pluginsdk.TypeString, + ValidateFunc: validateString, // want `ValidateFunc has no effect in Attributes\(\), where every field is computed - remove it` + }, + "misplaced": { + Type: pluginsdk.TypeString, + Optional: true, // want `Optional has no effect in Attributes\(\), where every field is computed - use Computed` + }, + "block": { + Type: pluginsdk.TypeList, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "port": { + Type: pluginsdk.TypeInt, + Optional: true, // want `Optional has no effect inside a computed-only block - use Computed` + }, + }, + }, + }, + } +} + +func passing(dynamic bool) { + // optional+computed is configurable, everything is allowed + _ = &schema.Schema{ + Type: schema.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validateString, + } + + // an optional block may nest optional fields + _ = &schema.Schema{ + Type: schema.TypeList, + Optional: true, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "name": { + Type: schema.TypeString, + Optional: true, + ValidateFunc: validateString, + }, + }, + }, + } + + // zero values are not set + _ = &schema.Schema{ + Type: schema.TypeList, + Computed: true, + MaxItems: 0, + Default: nil, + ConflictsWith: nil, + Elem: &schema.Schema{Type: schema.TypeString}, + } + + // computed under a non-constant flag is not provably computed-only + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: dynamic, + ValidateFunc: validateString, + } + + // plain computed fields with output-side attributes are fine + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + Sensitive: true, + Description: "set by the API", + } +} diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go.golden b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go.golden new file mode 100644 index 0000000..0de1b7d --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/azs009.go.golden @@ -0,0 +1,164 @@ +package azs009 + +import ( + "azs009/pluginsdk" + "azs009/schema" +) + +func validateString(any, string) ([]string, []error) { return nil, nil } + +func suppress(string, string, string, any) bool { return false } + +func flaggedTopLevel() { + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + } + + _ = &pluginsdk.Schema{ + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Schema{Type: pluginsdk.TypeString}, + } + + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + } + + _ = &schema.Schema{ + Type: schema.TypeList, + Computed: true, + Elem: &schema.Resource{Schema: map[string]*schema.Schema{}}, + } +} + +func flaggedNested() { + _ = &pluginsdk.Schema{ + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "name": { + Type: pluginsdk.TypeString, + Computed: true, // want `Optional has no effect inside a computed-only block - use Computed` + }, + "enabled": { + Type: pluginsdk.TypeBool, + Computed: true, + }, + "count": { + Type: pluginsdk.TypeInt, + Computed: true, // want `Required has no effect inside a computed-only block - use Computed` + }, + "inner": { + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "deep": { + Type: pluginsdk.TypeString, + Computed: true, // want `Optional has no effect inside a computed-only block - use Computed` + }, + }, + }, + }, + }, + }, + } + + _ = &schema.Schema{ + Type: schema.TypeSet, + Computed: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + } +} + +type typedResource struct{} + +func (typedResource) Arguments() map[string]*pluginsdk.Schema { + return map[string]*pluginsdk.Schema{ + "name": { + Type: pluginsdk.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validateString, + }, + } +} + +func (typedResource) Attributes() map[string]*pluginsdk.Schema { + return map[string]*pluginsdk.Schema{ + "id": { + Type: pluginsdk.TypeString, + }, + "misplaced": { + Type: pluginsdk.TypeString, + Optional: true, // want `Optional has no effect in Attributes\(\), where every field is computed - use Computed` + }, + "block": { + Type: pluginsdk.TypeList, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "port": { + Type: pluginsdk.TypeInt, + Computed: true, // want `Optional has no effect inside a computed-only block - use Computed` + }, + }, + }, + }, + } +} + +func passing(dynamic bool) { + // optional+computed is configurable, everything is allowed + _ = &schema.Schema{ + Type: schema.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validateString, + } + + // an optional block may nest optional fields + _ = &schema.Schema{ + Type: schema.TypeList, + Optional: true, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "name": { + Type: schema.TypeString, + Optional: true, + ValidateFunc: validateString, + }, + }, + }, + } + + // zero values are not set + _ = &schema.Schema{ + Type: schema.TypeList, + Computed: true, + MaxItems: 0, + Default: nil, + ConflictsWith: nil, + Elem: &schema.Schema{Type: schema.TypeString}, + } + + // computed under a non-constant flag is not provably computed-only + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: dynamic, + ValidateFunc: validateString, + } + + // plain computed fields with output-side attributes are fine + _ = &schema.Schema{ + Type: schema.TypeString, + Computed: true, + Sensitive: true, + Description: "set by the API", + } +} diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/pluginsdk/pluginsdk.go b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/pluginsdk/pluginsdk.go new file mode 100644 index 0000000..26bb97a --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/pluginsdk/pluginsdk.go @@ -0,0 +1,17 @@ +// Package pluginsdk mirrors terraform-provider-azurerm's internal/tf/pluginsdk type aliases. +package pluginsdk + +import "azs009/schema" + +type ( + Schema = schema.Schema + Resource = schema.Resource +) + +const ( + TypeBool = schema.TypeBool + TypeInt = schema.TypeInt + TypeString = schema.TypeString + TypeList = schema.TypeList + TypeSet = schema.TypeSet +) diff --git a/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/schema/schema.go b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/schema/schema.go new file mode 100644 index 0000000..b9c898f --- /dev/null +++ b/checks/AZS/AZS009_computed_only_field_input_attributes/testdata/src/azs009/schema/schema.go @@ -0,0 +1,62 @@ +// Package schema is a minimal stub of the plugin SDK's helper/schema for the analyzer tests. +package schema + +type ValueType int + +const ( + TypeInvalid ValueType = iota + TypeBool + TypeInt + TypeFloat + TypeString + TypeList + TypeMap + TypeSet +) + +type SchemaConfigMode int + +const ( + SchemaConfigModeAuto SchemaConfigMode = iota + SchemaConfigModeAttr + SchemaConfigModeBlock +) + +type ( + SchemaDefaultFunc func() (any, error) + SchemaValidateFunc func(any, string) ([]string, []error) + SchemaValidateDiagFunc func(any, string) error + SchemaDiffSuppressFunc func(string, string, string, any) bool + SchemaStateFunc func(any) string +) + +type Schema struct { + Type ValueType + ConfigMode SchemaConfigMode + Optional bool + Required bool + Computed bool + ForceNew bool + Sensitive bool + WriteOnly bool + Description string + Default any + DefaultFunc SchemaDefaultFunc + InputDefault string + ValidateFunc SchemaValidateFunc + ValidateDiagFunc SchemaValidateDiagFunc + DiffSuppressFunc SchemaDiffSuppressFunc + DiffSuppressOnRefresh bool + StateFunc SchemaStateFunc + MaxItems int + MinItems int + AtLeastOneOf []string + ExactlyOneOf []string + ConflictsWith []string + RequiredWith []string + Elem any +} + +type Resource struct { + Schema map[string]*Schema +} diff --git a/lib/astx/edit.go b/lib/astx/edit.go new file mode 100644 index 0000000..7af6ff3 --- /dev/null +++ b/lib/astx/edit.go @@ -0,0 +1,36 @@ +package astx + +import ( + "go/ast" + "go/token" + "strings" + + "golang.org/x/tools/go/analysis" +) + +// DeleteLine returns the edit removing node together with the rest of its line (a trailing +// comma and comment included), so no blank line is left behind; when other code shares the +// line only the node itself goes. +func DeleteLine(pass *analysis.Pass, node ast.Node) analysis.TextEdit { + tf := pass.Fset.File(node.Pos()) + start, end := node.Pos(), node.End() + if tf != nil { + line := tf.Line(start) + lineStart := tf.LineStart(line) + var lineEnd token.Pos + if line < tf.LineCount() { + lineEnd = tf.LineStart(line + 1) + } else { + lineEnd = token.Pos(tf.Base() + tf.Size()) + } + if content, err := pass.ReadFile(tf.Name()); err == nil { + before := strings.TrimSpace(string(content[tf.Offset(lineStart):tf.Offset(start)])) + after := strings.TrimSpace(string(content[tf.Offset(end):tf.Offset(lineEnd)])) + after = strings.TrimSpace(strings.TrimPrefix(after, ",")) + if before == "" && (after == "" || strings.HasPrefix(after, "//")) { + start, end = lineStart, lineEnd + } + } + } + return analysis.TextEdit{Pos: start, End: end} +}