diff --git a/internal/configs/named_values.go b/internal/configs/named_values.go index 61c18a5854..13bcf9e41e 100644 --- a/internal/configs/named_values.go +++ b/internal/configs/named_values.go @@ -7,17 +7,21 @@ package configs import ( "fmt" + "maps" + "slices" "strings" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/ext/typeexpr" "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/hcl/v2/hclsyntax" - "github.com/opentofu/opentofu/internal/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" "github.com/opentofu/opentofu/internal/addrs" + "github.com/opentofu/opentofu/internal/didyoumean" + "github.com/opentofu/opentofu/internal/lang/lint" + "github.com/opentofu/opentofu/internal/tfdiags" ) // A consistent detail message for all "not a valid identifier" diagnostics. @@ -186,6 +190,10 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno }) val = cty.DynamicVal } + // We might generate some warnings even if the default value is + // technically valid, if it seems like anything is highly likely + // to be a mistake. + diags = append(diags, lintVariableDefaultValue(attr.Expr, v.ConstraintType)...) } if !v.Nullable && val.IsNull() { @@ -218,6 +226,51 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno return v, diags } +// lintVariableDefaultValue checks for situations where the expression used to +// set the default value for an input variable is highly likely to be a mistake +// despite being technically valid, and returns warnings for those cases. +// +// This function never returns error diagnostics. +func lintVariableDefaultValue(expr hcl.Expression, targetTy cty.Type) hcl.Diagnostics { + var diags hcl.Diagnostics + + // If the expression used to define the default vaue includes any object + // constructor expressions with attribute names that would definitely have + // been discarded during type conversion then we'll warn about that, because + // there's no useful reason to do that. + for unused := range lint.DiscardedObjectConstructorAttrs(expr, targetTy) { + // The final step of the path is the one representing the problem + // while any that appear before it are just context. + prePath, problemStep := unused.Path[:len(unused.Path)-1], unused.Path[len(unused.Path)-1] + attrName := problemStep.(cty.GetAttrStep).Name + + attrs := slices.Collect(maps.Keys(unused.TargetType.AttributeTypes())) + suggestion := "" + if similarName := didyoumean.NameSuggestion(attrName, attrs); similarName != "" { + suggestion = fmt.Sprintf(" Did you mean to set attribute %q instead?", similarName) + } + + var nounPhrase string + if len(prePath) != 0 { + nounPhrase = fmt.Sprintf("The object type for %s", tfdiags.FormatCtyPath(prePath)) + } else { + nounPhrase = "The object type for this variable" + } + + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Object attribute is ignored", + Detail: fmt.Sprintf( + "%s does not include an attribute named %q, so this definition is unused.%s", + nounPhrase, attrName, suggestion, + ), + Subject: unused.NameRange.ToHCL().Ptr(), + }) + } + + return diags +} + func decodeVariableType(expr hcl.Expression) (cty.Type, *typeexpr.Defaults, VariableParsingMode, hcl.Diagnostics) { if exprIsNativeQuotedString(expr) { // If a user provides the pre-0.12 form of variable type argument where diff --git a/internal/configs/testdata/invalid-files/variable-complex-bad-default-inner-obj.tf b/internal/configs/testdata/invalid-files/variable-complex-bad-default-inner-obj.tf index 053543f2cd..cacaa9879e 100644 --- a/internal/configs/testdata/invalid-files/variable-complex-bad-default-inner-obj.tf +++ b/internal/configs/testdata/invalid-files/variable-complex-bad-default-inner-obj.tf @@ -9,7 +9,6 @@ variable "bad_type_for_inner_field" { default = { "mykey" = { field = "not a bool" - dont = "mind me" } } -} \ No newline at end of file +} diff --git a/internal/configs/testdata/warning-files/unused_variable_default_attrs.tf b/internal/configs/testdata/warning-files/unused_variable_default_attrs.tf new file mode 100644 index 0000000000..5a0e29d319 --- /dev/null +++ b/internal/configs/testdata/warning-files/unused_variable_default_attrs.tf @@ -0,0 +1,13 @@ +variable "a" { + type = object({ + a = string + b = object({}) + }) + default = { + a = "boop" + b = { + nope = "..." # WARNING: Object attribute is ignored + } + c = "hello" # WARNING: Object attribute is ignored + } +} diff --git a/internal/lang/lint/doc.go b/internal/lang/lint/doc.go new file mode 100644 index 0000000000..740a0c9e7b --- /dev/null +++ b/internal/lang/lint/doc.go @@ -0,0 +1,13 @@ +// Copyright (c) The OpenTofu Authors +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2023 HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +// Package lint contains a collection of helpers for performing "lint-like" +// checks to try to detect configuration constructs that are valid but +// nonetheless very likely to be a mistake. +// +// A particular check only qualifies for inclusion here if its false-positive +// rate is very, very low. Incorrectly guessing that something was a mistake +// causes confusion. +package lint diff --git a/internal/lang/lint/object_extra_attrs.go b/internal/lang/lint/object_extra_attrs.go new file mode 100644 index 0000000000..b2f62a878b --- /dev/null +++ b/internal/lang/lint/object_extra_attrs.go @@ -0,0 +1,302 @@ +// Copyright (c) The OpenTofu Authors +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2023 HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package lint + +import ( + "iter" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/convert" + + "github.com/opentofu/opentofu/internal/tfdiags" +) + +// DiscardedObjectConstructorAttr is the result type for +// [DiscardedObjectConstructorAttrs]. +type DiscardedObjectConstructorAttr struct { + // Path is a description of the path from the top-level expression passed + // to [DiscardedObjectConstructorAttrs] to the attribute that was defined + // but would be discarded under type conversion. + // + // By definition the last step of this path will always be an attribute + // step referring to an attribute that is not included in the target type. + // + // The steps before the final element of this path, if any, describe where + // in a nested data structure the problematic definition appeared. This + // is important to include somehow in a resulting error message to help + // the reader understand which part of the data structure was affected. + // These path steps can potentially include index steps with unknown key + // values when a problem is being reported for all elements of a collection + // at once. [tfdiags.FormatCtyPath] has support for index + // steps with unknown values, so it's always valid to pass the value + // of this field to that function. + Path cty.Path + + // NameRange is the source range of the name part of the attribute + // definition that is being reported. + NameRange tfdiags.SourceRange + + // TargetType is the leaf object type that the affected object constructor + // was building a value for. This is the type that the last element of + // Path would traverse into, and which (by definition) does not have an + // attribute corresponding to that final traversal step. + TargetType cty.Type +} + +// DiscardedObjectConstructorAttrs compares the given HCL expression with the +// given target type that the result of the expression would be converted to, +// and returns a sequence of attributes defined within object constructor +// expressions that would be immediately discarded during type conversion, and +// so are therefore effectively useless. +// +// A common cause for this to arise is someone making a typo of the attribute +// name when the attribute they were intending to define is optional and +// therefore this doesn't cause an error from the failure to define a required +// attribute. +// +// This function makes a best effort to work recursively through certain other +// HCL expression types to find problems even in nested object constructor +// expressions, but it's primarily focused on the most common case where a +// nested structure is written out literally using nested object and tuple +// constructor expressions, because other expression types are harder to +// analyze reliably with only static analysis. +func DiscardedObjectConstructorAttrs(expr hcl.Expression, targetTy cty.Type) iter.Seq[DiscardedObjectConstructorAttr] { + return func(yield func(DiscardedObjectConstructorAttr) bool) { + if targetTy.IsPrimitiveType() || targetTy == cty.DynamicPseudoType { + // There's nothing useful we could do with these types, so we'll + // just return early to reduce overhead. + return + } + // We'll preallocate some capacity for extending path a few steps, + // so we can avoid reallocating this unless the given structure is + // particularly deep. We share the backing array of this buffer + // across calls, so whenever a path is reported as part of a result + // we must copy it first. + path := make(cty.Path, 0, 4) + yieldDiscardedObjectConstructorAttrs(expr, targetTy, path, yield) + } +} + +// yieldDiscardedObjectConstructorAttrs is the main recursive body of +// [DiscardedObjectConstructorAttrs], called for each relevant nested expression +// inside a given expression, starting with the topmost expression. +// +// Returns false if the caller should cease calling yield or passing yield to +// other functions that might call yield. +func yieldDiscardedObjectConstructorAttrs(expr hcl.Expression, targetTy cty.Type, path cty.Path, yield func(DiscardedObjectConstructorAttr) bool) bool { + // What expression types are interesting depends on what type the caller + // is intending to convert the expression result to. + switch { + case targetTy.IsObjectType(): + // This is our main case and the only one where we'll actually + // potentially generate results directly, rather than just finding + // more nested expressions to visit recursively. + return yieldDiscardedObjectConstructorAttrsObject(expr, targetTy, path, yield) + case targetTy.IsMapType(): + return yieldDiscardedObjectConstructorAttrsMap(expr, targetTy, path, yield) + case targetTy.IsListType() || targetTy.IsSetType(): + // In both of cases we can potentially find nested expressions + // to visit through either a tuple constructor or tuple-for expression. + return yieldDiscardedObjectConstructorAttrsListLike(expr, targetTy, path, yield) + case targetTy.IsTupleType(): + return yieldDiscardedObjectConstructorAttrsTuple(expr, targetTy, path, yield) + default: + // No other target types relate to an expression type we know how + // to analyze for nested object constructor expressions. + return true + } +} + +func yieldDiscardedObjectConstructorAttrsObject(expr hcl.Expression, targetTy cty.Type, path cty.Path, yield func(DiscardedObjectConstructorAttr) bool) bool { + switch expr := expr.(type) { + case *hclsyntax.ObjectConsExpr: + // This is the main part of this overall analysis: we are checking for + // any object constructor item which has a constant attribute name + // that doesn't appear in the target type. + for _, item := range expr.Items { + keyStr, ok := attributeNameFromObjectConsKey(item.KeyExpr) + if !ok { + continue + } + if !targetTy.HasAttribute(keyStr) { + // Because this particular path is going to be included in + // our result, we need to make sure it has its own private + // backing array separate from "path". + retPath := make(cty.Path, len(path), len(path)+1) + copy(retPath, path) + retPath = append(retPath, cty.GetAttrStep{Name: keyStr}) + result := DiscardedObjectConstructorAttr{ + Path: retPath, + NameRange: tfdiags.SourceRangeFromHCL(item.KeyExpr.Range()), + TargetType: targetTy, + } + if !yield(result) { + return false + } + } else { + // If the item _does_ correlate with an expected attribute + // then we might be able to find more nested problems. + elemTy := targetTy.AttributeType(keyStr) + childPath := append(path, cty.GetAttrStep{Name: keyStr}) + if !yieldDiscardedObjectConstructorAttrs(item.ValueExpr, elemTy, childPath, yield) { + return false + } + } + } + return true + + default: + // We don't have anything useful to do with any other expression types. + return true + } +} + +func yieldDiscardedObjectConstructorAttrsMap(expr hcl.Expression, targetTy cty.Type, path cty.Path, yield func(DiscardedObjectConstructorAttr) bool) bool { + switch expr := expr.(type) { + case *hclsyntax.ObjectConsExpr: + // When a map is defined by conversion of an object constructor result, + // we can recursively visit the definitions of any items that have + // a known and constant key, using the map element type for all + // attribute values because that's how the object attributes would + // eventually be converted. + elemTy := targetTy.ElementType() + for _, item := range expr.Items { + keyStr, ok := attributeNameFromObjectConsKey(item.KeyExpr) + if !ok { + continue + } + // Temporary extension of the path for our recursive call + childPath := append(path, cty.IndexStep{Key: cty.StringVal(keyStr)}) + if !yieldDiscardedObjectConstructorAttrs(item.ValueExpr, elemTy, childPath, yield) { + return false + } + } + return true + + case *hclsyntax.ForExpr: + if expr.KeyExpr == nil { + // This is a tuple-for expression, which is not a valid way + // to define a map and so we'll ignore it and let this fail + // type conversion when the caller tries. + return true + } + // In this case we're testing the result expression of the object-for + // imagining that it will become a value for each element of the + // resulting map, and therefore we can assume that any value that + // is produced will get converted to the map's element type. + elemTy := targetTy.ElementType() + // Temporary extension of the path for our recursive call. We + // are effectively testing all elements of the resulting map at + // once, so the placeholder key is an unknown string. + childPath := append(path, cty.IndexStep{Key: cty.UnknownVal(cty.String)}) + return yieldDiscardedObjectConstructorAttrs(expr.ValExpr, elemTy, childPath, yield) + + default: + // We don't have anything useful to do with any other expression types. + return true + } +} + +func yieldDiscardedObjectConstructorAttrsListLike(expr hcl.Expression, targetTy cty.Type, path cty.Path, yield func(DiscardedObjectConstructorAttr) bool) bool { + switch expr := expr.(type) { + case *hclsyntax.TupleConsExpr: + // When a list or set is defined by conversion of a tuple constructor + // result, we can recursively visit the definitions of any items, using + // the collection element type for all attribute values because that's + // how the tuple elements would eventually be converted. + elemTy := targetTy.ElementType() + for idx, childExpr := range expr.Exprs { + // Temporary extension of the path for our recursive call + childPath := append(path, cty.IndexStep{Key: cty.NumberIntVal(int64(idx))}) + if !yieldDiscardedObjectConstructorAttrs(childExpr, elemTy, childPath, yield) { + return false + } + } + return true + case *hclsyntax.ForExpr: + if expr.KeyExpr != nil { + // This is an object-for expression, which is not a valid way + // to define a list or set and so we'll ignore it and let this + // fail type conversion when the caller tries. + return true + } + // In this case we're testing the result expression of the tuple-for + // imagining that it will become a value for each element of the + // resulting list/set, and therefore we can assume that any value that + // is produced will get converted to the map's element type. + elemTy := targetTy.ElementType() + // Temporary extension of the path for our recursive call. We + // are effectively testing all elements of the resulting map at + // once, so the placeholder key is an unknown number. + childPath := append(path, cty.IndexStep{Key: cty.UnknownVal(cty.Number)}) + return yieldDiscardedObjectConstructorAttrs(expr.ValExpr, elemTy, childPath, yield) + default: + // We don't have anything useful to do with any other expression types. + return true + } +} + +func yieldDiscardedObjectConstructorAttrsTuple(expr hcl.Expression, targetTy cty.Type, path cty.Path, yield func(DiscardedObjectConstructorAttr) bool) bool { + switch expr := expr.(type) { + case *hclsyntax.TupleConsExpr: + // For a tuple type each element has its own type, so we'll visit + // pairs of child attribute type and child expression. + // We only do this when the tuple constructor has the same number of + // elements as the tuple type, because otherwise type conversion will + // ultimately fail anyway. + etys := targetTy.TupleElementTypes() + if len(expr.Exprs) != len(etys) { + return true + } + for idx, childExpr := range expr.Exprs { + elemTy := etys[idx] + // Temporary extension of the path for our recursive call + childPath := append(path, cty.IndexStep{Key: cty.NumberIntVal(int64(idx))}) + if !yieldDiscardedObjectConstructorAttrs(childExpr, elemTy, childPath, yield) { + return false + } + } + return true + default: + // We don't have anything useful to do with any other expression types. + // (We don't try to analyze tuple-for expressions when the target + // type is tuple because in that case we can't make any reliable + // correlation between the dynamically-decided result values and the + // statically-decided tuple element types.) + return true + } +} + +// attributeNameFromObjectConsKey tries to find a known, constant attribute +// name given the expression from the "key" part of an item in an object +// constructor expression. +// +// If the second result is false then the key expression is invalid or too +// complicated to analyze. +func attributeNameFromObjectConsKey(keyExpr hcl.Expression) (string, bool) { + // The following is a cut-down version of the logic that + // HCL itself uses to evaluate keys in an object constructor + // expression during evaluation, from + // [hclsyntax.ObjectConsExpr.Value]. + // + // This is a best-effort thing which only works for + // valid and literally-defined keys, since that's the common + // case we're trying to lint-check. We'll ignore anything that + // we can't trivially evaluate. + key, keyDiags := keyExpr.Value(nil) + if keyDiags.HasErrors() || !key.IsKnown() || key.IsNull() { + return "", false + } + key, _ = key.Unmark() + var err error + key, err = convert.Convert(key, cty.String) + if err != nil { + return "", false + } + return key.AsString(), true +} diff --git a/internal/lang/lint/object_extra_attrs_test.go b/internal/lang/lint/object_extra_attrs_test.go new file mode 100644 index 0000000000..608c8c8445 --- /dev/null +++ b/internal/lang/lint/object_extra_attrs_test.go @@ -0,0 +1,352 @@ +// Copyright (c) The OpenTofu Authors +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2023 HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package lint + +import ( + "slices" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/zclconf/go-cty-debug/ctydebug" + "github.com/zclconf/go-cty/cty" + + "github.com/opentofu/opentofu/internal/tfdiags" +) + +func TestDiscardedObjectConstructorAttrs(t *testing.T) { + tests := map[string]struct { + exprSrc string + targetTy cty.Type + + want []DiscardedObjectConstructorAttr + }{ + // Simple, shallow cases + "unexpected attr for empty object type": { + `{ + foo = "bar" + }`, + cty.EmptyObject, + []DiscardedObjectConstructorAttr{ + { + Path: cty.GetAttrPath("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 5, Byte: 6}, + End: tfdiags.SourcePos{Line: 2, Column: 8, Byte: 9}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "unexpected attr for non-empty object type": { + `{ + foo = "bar" + }`, + cty.Object(map[string]cty.Type{ + "bar": cty.String, + }), + []DiscardedObjectConstructorAttr{ + { + Path: cty.GetAttrPath("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 5, Byte: 6}, + End: tfdiags.SourcePos{Line: 2, Column: 8, Byte: 9}, + }, + TargetType: cty.Object(map[string]cty.Type{ + "bar": cty.String, + }), + }, + }, + }, + "empty constructor for empty object type": { + `{}`, + cty.EmptyObject, + nil, + }, + "primitive type": { + // This case is irrelevant to this particular lint, so we're just + // testing that it successfully returns nothing rather than doing + // something undesirable like panicking. + `"hello"`, + cty.String, + nil, + }, + + // Nested in list + "nested in list, tuple-cons": { + `[ + {foo = "bar"}, + {baz = "beep"}, + ]`, + cty.List(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexIntPath(0).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 6, Byte: 7}, + End: tfdiags.SourcePos{Line: 2, Column: 9, Byte: 10}, + }, + TargetType: cty.EmptyObject, + }, + { + Path: cty.IndexIntPath(1).GetAttr("baz"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 26}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 29}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in list, tuple-for": { + `[ + for x in [] : { + foo = "bar" + } + ]`, + cty.List(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexPath(cty.UnknownVal(cty.Number)).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 27}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 30}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in list, object-for": { + `{ + for x in [] : x => { + foo = "bar" + } + }`, + cty.List(cty.EmptyObject), + // Can't define a list value using object-for, so no results here + nil, + }, + + // Nested in set + "nested in set, tuple-cons": { + `[ + {foo = "bar"}, + {baz = "beep"}, + ]`, + cty.Set(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexIntPath(0).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 6, Byte: 7}, + End: tfdiags.SourcePos{Line: 2, Column: 9, Byte: 10}, + }, + TargetType: cty.EmptyObject, + }, + { + Path: cty.IndexIntPath(1).GetAttr("baz"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 26}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 29}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in set, tuple-for": { + `[ + for x in [] : { + foo = "bar" + } + ]`, + cty.Set(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexPath(cty.UnknownVal(cty.Number)).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 27}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 30}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in set, object-for": { + `{ + for x in [] : x => { + foo = "bar" + } + }`, + cty.Set(cty.EmptyObject), + // Can't define a list value using object-for, so no results here + nil, + }, + + // Nested in map + "nested in map, object-cons": { + `{ + a = {foo = "bar"}, + b = {baz = "beep"}, + }`, + cty.Map(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexStringPath("a").GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 10, Byte: 11}, + End: tfdiags.SourcePos{Line: 2, Column: 13, Byte: 14}, + }, + TargetType: cty.EmptyObject, + }, + { + Path: cty.IndexStringPath("b").GetAttr("baz"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 10, Byte: 34}, + End: tfdiags.SourcePos{Line: 3, Column: 13, Byte: 37}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in map, object-for": { + `{ + for x in [] : x => { + foo = "bar" + } + }`, + cty.Map(cty.EmptyObject), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexPath(cty.UnknownVal(cty.String)).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 32}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 35}, + }, + TargetType: cty.EmptyObject, + }, + }, + }, + "nested in map, tuple-for": { + `[ + for x in [] : { + foo = "bar" + } + ]`, + cty.Map(cty.EmptyObject), + // Can't define a map value using tuple-for, so no results here + nil, + }, + + // Nested in tuple + "nested in tuple, tuple-cons": { + `[ + {foo = "bar"}, + {baz = "beep"}, + ]`, + cty.Tuple([]cty.Type{ + cty.EmptyObject, + cty.Object(map[string]cty.Type{"not_baz": cty.String}), + }), + []DiscardedObjectConstructorAttr{ + { + Path: cty.IndexIntPath(0).GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 2, Column: 6, Byte: 7}, + End: tfdiags.SourcePos{Line: 2, Column: 9, Byte: 10}, + }, + TargetType: cty.EmptyObject, + }, + { + Path: cty.IndexIntPath(1).GetAttr("baz"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 26}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 29}, + }, + TargetType: cty.Object(map[string]cty.Type{"not_baz": cty.String}), + }, + }, + }, + "nested in tuple, tuple-for": { + `[ + for x in [] : { + foo = "bar" + } + ]`, + cty.Tuple([]cty.Type{ + cty.EmptyObject, + cty.Object(map[string]cty.Type{"not_foo": cty.String}), + }), + // We don't do any recursive analysis of a tuple type defined + // by a tuple-for expression, because we can't correlate the + // dynamic elements of a for expression with the static elements + // of a tuple type. + nil, + }, + + // Nested in another object + "nested in object-cons": { + `{ + a = { + foo = "bar" + } + b = { + baz = "beep" + } + c = {} + }`, + cty.Object(map[string]cty.Type{ + "a": cty.EmptyObject, + "b": cty.Object(map[string]cty.Type{"not_baz": cty.String}), + // "c" intentionally omitted; we're testing a mixture of + // both nested and non-nested at the same time. + }), + []DiscardedObjectConstructorAttr{ + { + Path: cty.GetAttrPath("a").GetAttr("foo"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 3, Column: 6, Byte: 17}, + End: tfdiags.SourcePos{Line: 3, Column: 9, Byte: 20}, + }, + TargetType: cty.EmptyObject, + }, + { + Path: cty.GetAttrPath("b").GetAttr("baz"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 6, Column: 6, Byte: 50}, + End: tfdiags.SourcePos{Line: 6, Column: 9, Byte: 53}, + }, + TargetType: cty.Object(map[string]cty.Type{"not_baz": cty.String}), + }, + { + Path: cty.GetAttrPath("c"), + NameRange: tfdiags.SourceRange{ + Start: tfdiags.SourcePos{Line: 8, Column: 5, Byte: 73}, + End: tfdiags.SourcePos{Line: 8, Column: 6, Byte: 74}, + }, + TargetType: cty.Object(map[string]cty.Type{ + "a": cty.EmptyObject, + "b": cty.Object(map[string]cty.Type{"not_baz": cty.String}), + }), + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + expr, hclDiags := hclsyntax.ParseExpression([]byte(test.exprSrc), "", hcl.InitialPos) + if hclDiags.HasErrors() { + t.Fatal("unexpected syntax errors: " + hclDiags.Error()) + } + + got := slices.Collect(DiscardedObjectConstructorAttrs(expr, test.targetTy)) + if diff := cmp.Diff(test.want, got, ctydebug.CmpOptions); diff != "" { + t.Error("wrong result\n" + diff) + } + }) + } +} diff --git a/internal/tofu/node_module_variable.go b/internal/tofu/node_module_variable.go index 82340ea062..3232f9b4ce 100644 --- a/internal/tofu/node_module_variable.go +++ b/internal/tofu/node_module_variable.go @@ -13,9 +13,7 @@ import ( "slices" "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/convert" "github.com/opentofu/opentofu/internal/addrs" "github.com/opentofu/opentofu/internal/configs" @@ -23,6 +21,7 @@ import ( "github.com/opentofu/opentofu/internal/didyoumean" "github.com/opentofu/opentofu/internal/instances" "github.com/opentofu/opentofu/internal/lang" + "github.com/opentofu/opentofu/internal/lang/lint" "github.com/opentofu/opentofu/internal/tfdiags" ) @@ -171,7 +170,7 @@ func (n *nodeModuleVariable) Execute(ctx context.Context, evalCtx EvalContext, o // that always happens before any other walk and so we'd generate // duplicate diagnostics if we produced this in later walks too. if op == walkValidate { - diags = diags.Append(n.warningDiags(ctx)) + diags = diags.Append(n.warningDiags()) } // Set values for arguments of a child module call, for later retrieval @@ -261,62 +260,39 @@ func (n *nodeModuleVariable) evalModuleVariable(ctx context.Context, evalCtx Eva // a mistake. // // This function never returns error diagnostics. -func (n *nodeModuleVariable) warningDiags(_ context.Context) tfdiags.Diagnostics { +func (n *nodeModuleVariable) warningDiags() tfdiags.Diagnostics { var diags tfdiags.Diagnostics - // If the definition directly uses object constructor syntax, meaning that - // the resulting object is definitely for this input variable and not - // also used in any other place, then any attributes that aren't included - // in the variable's object type constrant are very likely to be mistakes. - // (They would just be discarded during type conversion, and so there's - // no useful reason to include them.) - // - // We only do this for direct object constructor syntax because it's more - // reasonable to use an object defined elsewhere that intentionally has - // a superset of the expected attributes but is also used in a different - // place that relies on a different subset of its attributes. - if consExpr, ok := n.Expr.(*hclsyntax.ObjectConsExpr); ok { - ty := n.Config.ConstraintType - if ty != cty.NilType && ty.IsObjectType() { - for _, item := range consExpr.Items { - // The following is a cut-down version of the logic that - // HCL itself uses to evaluate keys in an object constructor - // expression during evaluation, from - // [hclsyntax.ObjectConsExpr.Value]. - // - // This is a best-effort thing which only works for - // valid and literally-defined keys, since that's the common - // case we're trying to lint-check. We'll ignore anything that - // we can't trivially evaluate. - key, keyDiags := item.KeyExpr.Value(nil) - if keyDiags.HasErrors() || !key.IsKnown() || key.IsNull() { - continue - } - key, _ = key.Unmark() - var err error - key, err = convert.Convert(key, cty.String) - if err != nil { - continue - } - keyStr := key.AsString() - if !ty.HasAttribute(keyStr) { - attrs := slices.Collect(maps.Keys(ty.AttributeTypes())) - suggestion := "" - if similarName := didyoumean.NameSuggestion(keyStr, attrs); similarName != "" { - suggestion = fmt.Sprintf(" Did you mean to set attribute %q instead?", similarName) - } - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Object attribute is ignored", - Detail: fmt.Sprintf( - "The object type for input variable %q does not include an attribute named %q, so this definition is unused.%s", - n.Addr.Variable.Name, keyStr, suggestion, - ), - Subject: item.KeyExpr.Range().Ptr(), - }) - } - } + // If the expression used to define the variable includes any object + // constructor expressions with attribute names that would definitely be + // discarded during type conversion then we'll warn about that, because + // there's no useful reason to do that. + for unused := range lint.DiscardedObjectConstructorAttrs(n.Expr, n.Config.ConstraintType) { + // The final step of the path is the one representing the problem + // while any that appear before it are just context. + prePath, problemStep := unused.Path[:len(unused.Path)-1], unused.Path[len(unused.Path)-1] + attrName := problemStep.(cty.GetAttrStep).Name + + attrs := slices.Collect(maps.Keys(unused.TargetType.AttributeTypes())) + suggestion := "" + if similarName := didyoumean.NameSuggestion(attrName, attrs); similarName != "" { + suggestion = fmt.Sprintf(" Did you mean to set attribute %q instead?", similarName) } + + var extraPathClause string + if len(prePath) != 0 { + extraPathClause = fmt.Sprintf(" nested value %s", tfdiags.FormatCtyPath(prePath)) + } + + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Object attribute is ignored", + Detail: fmt.Sprintf( + "The object type for input variable %q%s does not include an attribute named %q, so this definition is unused.%s", + n.Addr.Variable.Name, extraPathClause, attrName, suggestion, + ), + Subject: unused.NameRange.ToHCL().Ptr(), + }) } return diags diff --git a/internal/tofu/node_module_variable_test.go b/internal/tofu/node_module_variable_test.go index e6da515fcc..67dce73018 100644 --- a/internal/tofu/node_module_variable_test.go +++ b/internal/tofu/node_module_variable_test.go @@ -318,7 +318,7 @@ func TestNodeModuleVariable_warningDiags(t *testing.T) { Name: "foo", ConstraintType: cty.Object(map[string]cty.Type{ "foo": cty.String, - "bar": cty.String, + "bar": cty.Object(map[string]cty.Type{"nested": cty.EmptyObject}), }), }, Expr: &hclsyntax.ObjectConsExpr{ @@ -334,6 +334,29 @@ func TestNodeModuleVariable_warningDiags(t *testing.T) { Val: cty.StringVal("..."), }, }, + { + KeyExpr: &hclsyntax.LiteralValueExpr{ + Val: cty.StringVal("bar"), + SrcRange: hcl.Range{ + Filename: "test.tofu", + }, + }, + ValueExpr: &hclsyntax.ObjectConsExpr{ + Items: []hclsyntax.ObjectConsItem{ + { + KeyExpr: &hclsyntax.LiteralValueExpr{ + Val: cty.StringVal("beep"), + SrcRange: hcl.Range{ + Filename: "test.tofu", + }, + }, + ValueExpr: &hclsyntax.LiteralValueExpr{ + Val: cty.StringVal("..."), + }, + }, + }, + }, + }, }, }, ModuleInstance: addrs.RootModuleInstance, @@ -341,7 +364,7 @@ func TestNodeModuleVariable_warningDiags(t *testing.T) { // We use the "ForRPC" representation of the diagnostics just because // it's more friendly for comparison and we care only about the // user-facing information in the diagnostics, not their concrete types. - gotDiags := n.warningDiags(t.Context()).ForRPC() + gotDiags := n.warningDiags().ForRPC() var wantDiags tfdiags.Diagnostics wantDiags = wantDiags.Append(&hcl.Diagnostic{ Severity: hcl.DiagWarning, @@ -351,6 +374,14 @@ func TestNodeModuleVariable_warningDiags(t *testing.T) { Filename: "test.tofu", }, }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Object attribute is ignored", + Detail: `The object type for input variable "foo" nested value .bar does not include an attribute named "beep", so this definition is unused.`, + Subject: &hcl.Range{ + Filename: "test.tofu", + }, + }) wantDiags = wantDiags.ForRPC() if diff := cmp.Diff(wantDiags, gotDiags, ctydebug.CmpOptions); diff != "" { t.Error("wrong diagnostics\n" + diff)