jprdonnelly/opentf
1
0
Fork 0
mirror of https://github.com/opentffoundation/opentf.git synced 2026-05-01 01:00:21 -04:00
Code Issues Packages Projects Releases Wiki Activity

lint: DiscardedObjectConstructorAttrs

Browse Source 
This generalizes the previously-added lint-like check for when an object
constructor is used to define an input variable and it contains a
definition for an attribute that isn't part of the target type, so that
now it also works for various nested structures that commonly arise in
real-world configurations.

Because this is now considerably more complicated I factored it out into
a new package called "lint" which could potentially grow to include other
similar "technically valid but probably a mistake" situations in future,
but for now it just introduced an opportunity to produce similar warning
messages for ignored attribute definitions in the default value for an
input variable.

It seems to me that there is actually no useful reason to include an
unexpected attribute definition in either of these two cases: that
attribute will never appear as part of any expression that any other part
of the configuration can use. Therefore I considered making these be
treated as errors rather than warnings, but turning something that was
previously valid into an error is risky so I'm suggesting that we start
with these as warnings and then consider upgrading them to errors in a
later release if we don't hear of anyone reporting a false-positive that
was _somehow_ actually useful. (I find that very unlikely, but still...)

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This commit is contained in:
Martin Atkins
2025-09-19 11:36:18 -07:00
parent d7c4487172
commit 0fb0a4b707
8 changed files with 800 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

55
internal/configs/named_values.go
View File

@@ -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

3
internal/configs/testdata/invalid-files/variable-complex-bad-default-inner-obj.tf vendored
View File

@@ -9,7 +9,6 @@ variable "bad_type_for_inner_field" {
default = {
"mykey" = {
field = "not a bool"
dont = "mind me"
}
}
}
}

13
internal/configs/testdata/warning-files/unused_variable_default_attrs.tf vendored Normal file
View File

@@ -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
}
}

13
internal/lang/lint/doc.go Normal file
View File

@@ -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

302
internal/lang/lint/object_extra_attrs.go Normal file
View File

@@ -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
}

352
internal/lang/lint/object_extra_attrs_test.go Normal file
View File

@@ -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)
}
})
}
}

88
internal/tofu/node_module_variable.go
View File

@@ -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

35
internal/tofu/node_module_variable_test.go
View File

@@ -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)