From 36e43e30ee72ed2efc9e9365fe72bef98c0a5adb Mon Sep 17 00:00:00 2001
From: Lucas Bajolet <lucas.bajolet@hashicorp.com>
Date: Thu, 13 Jun 2024 16:12:42 -0400
Subject: [PATCH] hcl2template: detect duplicate locals during parse

Previously duplicate detection for local variables happened during
`Initialise`, through a call to `checkForDuplicateLocalDefinition`.

This works in a majority of cases, but for commands like `console`, this
was not detected as the return diagnostics for `Initialise` are ignored.

That check can be done as early as during parsing however, as the names
of blocks are not dynamic in the slightest (no interpolation possible),
so we move that detection logic into `Parse`, so that the behaviour is
coherent between all commands.
---
 hcl2template/parser.go                        |  3 ++-
 hcl2template/types.packer_config.go           | 25 +++++++++++--------
 packer_test/local_eval_test.go                | 17 +++++++++++++
 .../templates/locals_duplicate.pkr.hcl        | 18 +++++++++++++
 4 files changed, 52 insertions(+), 11 deletions(-)
 create mode 100644 packer_test/templates/locals_duplicate.pkr.hcl

diff --git a/hcl2template/parser.go b/hcl2template/parser.go
index a1d23f640..72d5c1634 100644
--- a/hcl2template/parser.go
+++ b/hcl2template/parser.go
@@ -190,6 +190,8 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 			diags = append(diags, morediags...)
 			cfg.LocalBlocks = append(cfg.LocalBlocks, moreLocals...)
 		}
+
+		diags = diags.Extend(cfg.checkForDuplicateLocalDefinition())
 	}
 
 	// parse var files
@@ -296,7 +298,6 @@ func filterVarsFromLogs(inputOrLocal Variables) {
 func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnostics {
 	diags := cfg.InputVariables.ValidateValues()
 	diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...)
-	diags = append(diags, checkForDuplicateLocalDefinition(cfg.LocalBlocks)...)
 	diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...)
 
 	filterVarsFromLogs(cfg.InputVariables)
diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go
index 4c69c4467..51608b741 100644
--- a/hcl2template/types.packer_config.go
+++ b/hcl2template/types.packer_config.go
@@ -280,24 +280,29 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnost
 	return diags
 }
 
-func checkForDuplicateLocalDefinition(locals []*LocalBlock) hcl.Diagnostics {
+// checkForDuplicateLocalDefinition walks through the list of defined variables
+// in order to detect duplicate locals definitions.
+func (c *PackerConfig) checkForDuplicateLocalDefinition() hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
-	// we could sort by name and then check contiguous names to use less memory,
-	// but using a map sounds good enough.
-	names := map[string]struct{}{}
-	for _, local := range locals {
-		if _, found := names[local.Name]; found {
-			diags = append(diags, &hcl.Diagnostic{
+	localNames := map[string]*LocalBlock{}
+
+	for _, block := range c.LocalBlocks {
+		loc, ok := localNames[block.Name]
+		if ok {
+			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Duplicate local definition",
-				Detail:   "Duplicate " + local.Name + " definition found.",
-				Subject:  local.Expr.Range().Ptr(),
+				Detail: fmt.Sprintf("Local variable %q is defined twice in your templates. Other definition found at %q",
+					block.Name, loc.Expr.Range()),
+				Subject: block.Expr.Range().Ptr(),
 			})
 			continue
 		}
-		names[local.Name] = struct{}{}
+
+		localNames[block.Name] = block
 	}
+
 	return diags
 }
 
diff --git a/packer_test/local_eval_test.go b/packer_test/local_eval_test.go
index 97cecf0ef..dec5b1fdb 100644
--- a/packer_test/local_eval_test.go
+++ b/packer_test/local_eval_test.go
@@ -1,5 +1,7 @@
 package packer_test
 
+import "fmt"
+
 func (ts *PackerTestSuite) TestEvalLocalsOrder() {
 	ts.SkipNoAcc()
 
@@ -12,3 +14,18 @@ func (ts *PackerTestSuite) TestEvalLocalsOrder() {
 		SetArgs("console", "./templates/locals_no_order.pkr.hcl").
 		Assert(MustSucceed(), Grep("\\[\\]", grepStdout, grepInvert))
 }
+
+func (ts *PackerTestSuite) TestLocalDuplicates() {
+	pluginDir, cleanup := ts.MakePluginDir()
+	defer cleanup()
+
+	for _, cmd := range []string{"console", "validate", "build"} {
+		ts.Run(fmt.Sprintf("duplicate local detection with %s command - expect error", cmd), func() {
+			ts.PackerCommand().UsePluginDir(pluginDir).
+				SetArgs(cmd, "./templates/locals_duplicate.pkr.hcl").
+				Assert(MustFail(),
+					Grep("Duplicate local definition"),
+					Grep("Local variable \"test\" is defined twice"))
+		})
+	}
+}
diff --git a/packer_test/templates/locals_duplicate.pkr.hcl b/packer_test/templates/locals_duplicate.pkr.hcl
new file mode 100644
index 000000000..fe91e3b77
--- /dev/null
+++ b/packer_test/templates/locals_duplicate.pkr.hcl
@@ -0,0 +1,18 @@
+local "test" {
+    expression = "two"
+    sensitive = true
+}
+
+locals {
+    test = local.test
+}
+
+variable "test" {
+    type = string
+    default = "home"
+}
+source "null" "example" {}
+
+build {
+    sources = ["source.null.example"]
+}