atc: behaviour: config validation considers var scopes

The rules are as follows:

* There is a top-level scope for local vars
* Each across step creates a new scope
* If you redeclare a variable that's declared in the same scope, you get
  an error (repeated var name)
* If you redeclare a variable that's declared in a higher scope, you get
  a warning (shadows local var '...')

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

atc/configvalidate/validate_test.go

@@ -1790,7 +1790,7 @@ var _ = Describe("ValidateConfig", func() {
 
 				It("returns an error", func() {
 					Expect(errorMessages).To(HaveLen(1))
-					Expect(errorMessages[0]).To(ContainSubstring("jobs.some-other-job.plan.do[1].load_var(a-var): repeated name"))
+					Expect(errorMessages[0]).To(ContainSubstring("jobs.some-other-job.plan.do[1].load_var(a-var): repeated var name"))
 				})
 			})
 
@@ -1894,6 +1894,67 @@ var _ = Describe("ValidateConfig", func() {
 				})
 			})
 
+			Context("when an across step shadows a var name from a parent scope", func() {
+				BeforeEach(func() {
+					job.PlanSequence = append(job.PlanSequence,
+						atc.Step{Config: &atc.LoadVarStep{
+							Name: "var1",
+							File: "unused",
+						}},
+						atc.Step{
+							Config: &atc.AcrossStep{
+								Step: &atc.PutStep{
+									Name: "some-resource",
+								},
+								Vars: []atc.AcrossVarConfig{
+									{
+										Var: "var1",
+									},
+								},
+							},
+						})
+
+					config.Jobs = append(config.Jobs, job)
+				})
+
+				It("returns a warning", func() {
+					Expect(errorMessages).To(BeEmpty())
+					Expect(warnings).To(HaveLen(1))
+					Expect(warnings[0].Message).To(ContainSubstring("jobs.some-other-job.plan.do[1].across[0]: shadows local var 'var1'"))
+				})
+			})
+
+			Context("when a substep of the across step shadows a var name from a parent scope", func() {
+				BeforeEach(func() {
+					job.PlanSequence = append(job.PlanSequence,
+						atc.Step{Config: &atc.LoadVarStep{
+							Name: "a",
+							File: "unused",
+						}},
+						atc.Step{
+							Config: &atc.AcrossStep{
+								Step: &atc.LoadVarStep{
+									Name: "a",
+									File: "unused",
+								},
+								Vars: []atc.AcrossVarConfig{
+									{
+										Var: "b",
+									},
+								},
+							},
+						})
+
+					config.Jobs = append(config.Jobs, job)
+				})
+
+				It("returns a warning", func() {
+					Expect(errorMessages).To(BeEmpty())
+					Expect(warnings).To(HaveLen(1))
+					Expect(warnings[0].Message).To(ContainSubstring("jobs.some-other-job.plan.do[1].across.load_var(a): shadows local var 'a'"))
+				})
+			})
+
 			Context("when an across step has a non-positive limit", func() {
 				BeforeEach(func() {
 					job.PlanSequence = append(job.PlanSequence, atc.Step{

atc/step_validator.go

@@ -25,10 +25,12 @@ type StepValidator struct {
 	config  Config
 	context []string
 
-	seenGetName      map[string]bool
-	seenLocalVarName map[string]bool
+	seenGetName    scope
+	localVarScopes []scope
 }
 
+type scope map[string]bool
+
 // NewStepValidator is a constructor which initializes internal data.
 //
 // The Config specified is used to validate the existence of resources and jobs
@@ -39,10 +41,10 @@ type StepValidator struct {
 // errors like 'jobs(foo).plan.task(bar): blah blah'.
 func NewStepValidator(config Config, context []string) *StepValidator {
 	return &StepValidator{
-		config:           config,
-		context:          context,
-		seenGetName:      map[string]bool{},
-		seenLocalVarName: map[string]bool{},
+		config:         config,
+		context:        context,
+		seenGetName:    scope{},
+		localVarScopes: []scope{{}},
 	}
 }
 
@@ -203,11 +205,7 @@ func (validator *StepValidator) VisitLoadVar(step *LoadVarStep) error {
 		validator.recordWarning(*warning)
 	}
 
-	if validator.seenLocalVarName[step.Name] {
-		validator.recordError("repeated name")
-	}
-
-	validator.seenLocalVarName[step.Name] = true
+	validator.declareLocalVar(step.Name)
 
 	if step.File == "" {
 		validator.recordError("no file specified")
@@ -286,6 +284,9 @@ func (validator *StepValidator) VisitAcross(step *AcrossStep) error {
 	validator.pushContext(".across")
 	defer validator.popContext()
 
+	validator.pushLocalVarScope()
+	defer validator.popLocalVarScope()
+
 	if !EnableAcrossStep {
 		validator.recordError("the across step must be explicitly opted-in to using the `--enable-across-step` flag")
 	}
@@ -296,10 +297,8 @@ func (validator *StepValidator) VisitAcross(step *AcrossStep) error {
 
 	for i, v := range step.Vars {
 		validator.pushContext("[%d]", i)
-		if validator.seenLocalVarName[v.Var] {
-			validator.recordError("repeated var name")
-		}
-		validator.seenLocalVarName[v.Var] = true
+
+		validator.declareLocalVar(v.Var)
 
 		validator.pushContext(".max_in_flight")
 		if v.MaxInFlight != nil && !v.MaxInFlight.All && v.MaxInFlight.Limit <= 0 {
@@ -424,3 +423,37 @@ func (validator *StepValidator) pushContext(ctx string, args ...interface{}) {
 func (validator *StepValidator) popContext() {
 	validator.context = validator.context[0 : len(validator.context)-1]
 }
+
+func (validator *StepValidator) pushLocalVarScope() {
+	validator.localVarScopes = append(validator.localVarScopes, scope{})
+}
+
+func (validator *StepValidator) popLocalVarScope() {
+	validator.localVarScopes = validator.localVarScopes[0 : len(validator.localVarScopes)-1]
+}
+
+func (validator *StepValidator) currentLocalVarScope() scope {
+	return validator.localVarScopes[len(validator.localVarScopes)-1]
+}
+
+func (validator *StepValidator) localVarIsDeclared(name string) bool {
+	for _, scope := range validator.localVarScopes {
+		if scope[name] {
+			return true
+		}
+	}
+	return false
+}
+
+func (validator *StepValidator) declareLocalVar(name string) {
+	if validator.currentLocalVarScope()[name] {
+		validator.recordError("repeated var name")
+	} else if validator.localVarIsDeclared(name) {
+		validator.recordWarning(ConfigWarning{
+			Type:    "var_shadowed",
+			Message: validator.annotate(fmt.Sprintf("shadows local var '%s'", name)),
+		})
+	}
+
+	validator.currentLocalVarScope()[name] = true
+}