From 708003b03598331cf70329b03696ea9a77dde51f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 30 Jun 2021 09:22:02 -0700 Subject: [PATCH] configs: For Moved blocks, use addrs.MoveEndpoint instead of addrs.Target Although addrs.Target can in principle capture the information we need to represent move endpoints, it's semantically confusing because addrs.Targetable uses addrs.Abs... types which are typically for absolute addresses, but we were using them for relative addresses here. We now have specialized address types for representing moves and probably other things which have similar requirements later on. These types largely communicate the same information in the end, but aim to do so in a way that's explicit about which addresses are relative and which are absolute, to make it less likely that we'd inadvertently misuse these addresses. --- internal/addrs/move_endpoint.go | 30 ++++++++++++++++++++++++++++++ internal/configs/moved.go | 16 +++++++++------- internal/configs/moved_test.go | 28 ++++++++++++++-------------- 3 files changed, 53 insertions(+), 21 deletions(-) diff --git a/internal/addrs/move_endpoint.go b/internal/addrs/move_endpoint.go index 70172a080..b7c529dd5 100644 --- a/internal/addrs/move_endpoint.go +++ b/internal/addrs/move_endpoint.go @@ -47,6 +47,36 @@ func (e *MoveEndpoint) String() string { return e.relSubject.String() } +func (e *MoveEndpoint) Equal(other *MoveEndpoint) bool { + switch { + case (e == nil) != (other == nil): + return false + case e == nil: + return true + default: + // Since we only use ModuleInstance and AbsResourceInstance in our + // string representation, we have no ambiguity between address types + // and can safely just compare the string representations to + // compare the relSubject values. + return e.String() == other.String() && e.SourceRange == other.SourceRange + } +} + +// MightUnifyWith returns true if it is possible that a later call to +// UnifyMoveEndpoints might succeed if given the reciever and the other +// given endpoint. +// +// This is intended for early static validation of obviously-wrong situations, +// although there are still various semantic errors that this cannot catch. +func (e *MoveEndpoint) MightUnifyWith(other *MoveEndpoint) bool { + // For our purposes here we'll just do a unify without a base module + // address, because the rules for whether unify can succeed depend + // only on the relative part of the addresses, not on which module + // they were declared in. + from, to := UnifyMoveEndpoints(RootModuleInstance, e, other) + return from != nil && to != nil +} + // ConfigMovable transforms the reciever into a ConfigMovable by resolving it // relative to the given base module, which should be the module where // the MoveEndpoint expression was found. diff --git a/internal/configs/moved.go b/internal/configs/moved.go index 891636a73..5cfbd5dfb 100644 --- a/internal/configs/moved.go +++ b/internal/configs/moved.go @@ -6,8 +6,8 @@ import ( ) type Moved struct { - From *addrs.Target - To *addrs.Target + From *addrs.MoveEndpoint + To *addrs.MoveEndpoint DeclRange hcl.Range } @@ -25,7 +25,7 @@ func decodeMovedBlock(block *hcl.Block) (*Moved, hcl.Diagnostics) { from, traversalDiags := hcl.AbsTraversalForExpr(attr.Expr) diags = append(diags, traversalDiags...) if !traversalDiags.HasErrors() { - from, fromDiags := addrs.ParseTarget(from) + from, fromDiags := addrs.ParseMoveEndpoint(from) diags = append(diags, fromDiags.ToHCL()...) moved.From = from } @@ -35,7 +35,7 @@ func decodeMovedBlock(block *hcl.Block) (*Moved, hcl.Diagnostics) { to, traversalDiags := hcl.AbsTraversalForExpr(attr.Expr) diags = append(diags, traversalDiags...) if !traversalDiags.HasErrors() { - to, toDiags := addrs.ParseTarget(to) + to, toDiags := addrs.ParseMoveEndpoint(to) diags = append(diags, toDiags.ToHCL()...) moved.To = to } @@ -43,11 +43,13 @@ func decodeMovedBlock(block *hcl.Block) (*Moved, hcl.Diagnostics) { // we can only move from a module to a module, resource to resource, etc. if !diags.HasErrors() { - if moved.To.Subject.AddrType() != moved.From.Subject.AddrType() { + if !moved.From.MightUnifyWith(moved.To) { + // We can catch some obviously-wrong combinations early here, + // but we still have other dynamic validation to do at runtime. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Invalid \"moved\" targets", - Detail: "The \"from\" and \"to\" targets must be the same address type", + Summary: "Invalid \"moved\" addresses", + Detail: "The \"from\" and \"to\" addresses must either both refer to resources or both refer to modules.", Subject: &moved.DeclRange, }) } diff --git a/internal/configs/moved_test.go b/internal/configs/moved_test.go index c662dae95..f2110de1a 100644 --- a/internal/configs/moved_test.go +++ b/internal/configs/moved_test.go @@ -48,8 +48,8 @@ func TestDecodeMovedBlock(t *testing.T) { DefRange: blockRange, }, &Moved{ - From: mustTargetFromExpr(foo_expr), - To: mustTargetFromExpr(bar_expr), + From: mustMoveEndpointFromExpr(foo_expr), + To: mustMoveEndpointFromExpr(bar_expr), DeclRange: blockRange, }, ``, @@ -72,8 +72,8 @@ func TestDecodeMovedBlock(t *testing.T) { DefRange: blockRange, }, &Moved{ - From: mustTargetFromExpr(foo_index_expr), - To: mustTargetFromExpr(bar_index_expr), + From: mustMoveEndpointFromExpr(foo_index_expr), + To: mustMoveEndpointFromExpr(bar_index_expr), DeclRange: blockRange, }, ``, @@ -96,8 +96,8 @@ func TestDecodeMovedBlock(t *testing.T) { DefRange: blockRange, }, &Moved{ - From: mustTargetFromExpr(mod_foo_expr), - To: mustTargetFromExpr(mod_bar_expr), + From: mustMoveEndpointFromExpr(mod_foo_expr), + To: mustMoveEndpointFromExpr(mod_bar_expr), DeclRange: blockRange, }, ``, @@ -116,7 +116,7 @@ func TestDecodeMovedBlock(t *testing.T) { DefRange: blockRange, }, &Moved{ - From: mustTargetFromExpr(foo_expr), + From: mustMoveEndpointFromExpr(foo_expr), DeclRange: blockRange, }, "Missing required argument", @@ -139,11 +139,11 @@ func TestDecodeMovedBlock(t *testing.T) { DefRange: blockRange, }, &Moved{ - To: mustTargetFromExpr(foo_expr), - From: mustTargetFromExpr(mod_foo_expr), + To: mustMoveEndpointFromExpr(foo_expr), + From: mustMoveEndpointFromExpr(mod_foo_expr), DeclRange: blockRange, }, - "Invalid \"moved\" targets", + "Invalid \"moved\" addresses", }, } @@ -162,23 +162,23 @@ func TestDecodeMovedBlock(t *testing.T) { t.Fatal("expected error") } - if !cmp.Equal(got, test.want) { + if !cmp.Equal(got, test.want, cmp.AllowUnexported(addrs.MoveEndpoint{})) { t.Fatalf("wrong result: %s", cmp.Diff(got, test.want)) } }) } } -func mustTargetFromExpr(expr hcl.Expression) *addrs.Target { +func mustMoveEndpointFromExpr(expr hcl.Expression) *addrs.MoveEndpoint { traversal, hcldiags := hcl.AbsTraversalForExpr(expr) if hcldiags.HasErrors() { panic(hcldiags.Errs()) } - target, diags := addrs.ParseTarget(traversal) + ep, diags := addrs.ParseMoveEndpoint(traversal) if diags.HasErrors() { panic(diags.Err()) } - return target + return ep }