mirror of
https://github.com/opentffoundation/opentf.git
synced 2025-12-19 17:59:05 -05:00
tofu: Warn if object literal includes unused attribute for input variable
We intentionally allow assigning object types with a superset of the attributes included in an input variable's object type constraints because it makes it possible to assign a whole object for which only some of the attributes are relevant for one input variable but a different subset might be relevant when the object value is used in a different part of the configuration. However, when the variable is defined using an object literal expression there is no possible way an unexpected attribute could be useful in a different part of the configuration, and so that's very very likely to be a mistake rather than intentional. Therefore we'll generate a "linter-like" warning in that case to help the author notice their mistake without introducing any new "strict-mode" language features, or other complexity that would be harder to maintain and evolve over time. Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This commit is contained in:
@@ -9,13 +9,18 @@ import (
|
||||
"context"
|
||||
"fmt"
|
||||
"log"
|
||||
"maps"
|
||||
"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"
|
||||
"github.com/opentofu/opentofu/internal/dag"
|
||||
"github.com/opentofu/opentofu/internal/didyoumean"
|
||||
"github.com/opentofu/opentofu/internal/instances"
|
||||
"github.com/opentofu/opentofu/internal/lang"
|
||||
"github.com/opentofu/opentofu/internal/tfdiags"
|
||||
@@ -160,6 +165,15 @@ func (n *nodeModuleVariable) Execute(ctx context.Context, evalCtx EvalContext, o
|
||||
return diags
|
||||
}
|
||||
|
||||
// We might generate some "linter-like" warnings for situations that
|
||||
// have a high likelihood of being a mistake even though they are
|
||||
// technically valid. We check these only in the validate walk because
|
||||
// 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))
|
||||
}
|
||||
|
||||
// Set values for arguments of a child module call, for later retrieval
|
||||
// during expression evaluation.
|
||||
_, call := n.Addr.Module.CallInstance()
|
||||
@@ -241,3 +255,69 @@ func (n *nodeModuleVariable) evalModuleVariable(ctx context.Context, evalCtx Eva
|
||||
|
||||
return finalVal, diags.ErrWithWarnings()
|
||||
}
|
||||
|
||||
// warningDiags detects "lint-like" problems with a variable's definition, where
|
||||
// the input is technically valid but nonetheless seems highly likely to be
|
||||
// a mistake.
|
||||
//
|
||||
// This function never returns error diagnostics.
|
||||
func (n *nodeModuleVariable) warningDiags(_ context.Context) 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(),
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return diags
|
||||
}
|
||||
|
||||
@@ -12,8 +12,10 @@ import (
|
||||
"testing"
|
||||
|
||||
"github.com/go-test/deep"
|
||||
"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/addrs"
|
||||
@@ -307,3 +309,51 @@ func TestNodeModuleVariableConstraints(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestNodeModuleVariable_warningDiags(t *testing.T) {
|
||||
t.Run("unused object attribute", func(t *testing.T) {
|
||||
n := &nodeModuleVariable{
|
||||
Addr: addrs.InputVariable{Name: "foo"}.Absolute(addrs.RootModuleInstance),
|
||||
Config: &configs.Variable{
|
||||
Name: "foo",
|
||||
ConstraintType: cty.Object(map[string]cty.Type{
|
||||
"foo": cty.String,
|
||||
"bar": cty.String,
|
||||
}),
|
||||
},
|
||||
Expr: &hclsyntax.ObjectConsExpr{
|
||||
Items: []hclsyntax.ObjectConsItem{
|
||||
{
|
||||
KeyExpr: &hclsyntax.LiteralValueExpr{
|
||||
Val: cty.StringVal("baz"),
|
||||
SrcRange: hcl.Range{
|
||||
Filename: "test.tofu",
|
||||
},
|
||||
},
|
||||
ValueExpr: &hclsyntax.LiteralValueExpr{
|
||||
Val: cty.StringVal("..."),
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
ModuleInstance: addrs.RootModuleInstance,
|
||||
}
|
||||
// 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()
|
||||
var wantDiags tfdiags.Diagnostics
|
||||
wantDiags = wantDiags.Append(&hcl.Diagnostic{
|
||||
Severity: hcl.DiagWarning,
|
||||
Summary: "Object attribute is ignored",
|
||||
Detail: `The object type for input variable "foo" does not include an attribute named "baz", so this definition is unused. Did you mean to set attribute "bar" instead?`,
|
||||
Subject: &hcl.Range{
|
||||
Filename: "test.tofu",
|
||||
},
|
||||
})
|
||||
wantDiags = wantDiags.ForRPC()
|
||||
if diff := cmp.Diff(wantDiags, gotDiags, ctydebug.CmpOptions); diff != "" {
|
||||
t.Error("wrong diagnostics\n" + diff)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user