lang: Fix import scoping issue with classes

When include-ing a class, we propagated the scope of the include into
the class instead of using the correct scope that existed when the class
was defined and instead propagating only the include arguments in.

This patch fixes the issue and adds a ton of tests as well. It also
propagates the scope into the include args, in case that is needed, and
adds a test for that as well.

Thanks to Nicolas Charles for the initial bug report.

lang/interpret_test/TestAstFunc1/importscope0.graph

@@ -0,0 +1,10 @@
+Edge: call:os.is_debian() -> if(call:os.is_debian()) # c
+Edge: if(call:os.is_debian()) -> var(aaa) # aaa
+Edge: str(bbb) -> if(call:os.is_debian()) # a
+Edge: str(ccc) -> if(call:os.is_debian()) # b
+Vertex: call:os.is_debian()
+Vertex: if(call:os.is_debian())
+Vertex: str(bbb)
+Vertex: str(ccc)
+Vertex: str(hello)
+Vertex: var(aaa)

lang/interpret_test/TestAstFunc1/importscope0/main.mcl

@@ -0,0 +1,3 @@
+import "second.mcl"
+
+include second.xclass

lang/interpret_test/TestAstFunc1/importscope0/second.mcl

@@ -0,0 +1,12 @@
+import "os"
+import "fmt"
+
+class xclass {
+	#import "os" # this should not be required, top-level should be enough
+
+	$aaa = if os.is_debian() { "bbb" }  else { "ccc" }
+
+	print "${aaa}" {
+		msg => "hello",
+	}
+}

lang/interpret_test/TestAstFunc1/importscope1.graph

@@ -0,0 +1 @@
+# err: err3: func `os.is_debian` does not exist in this scope

lang/interpret_test/TestAstFunc1/importscope1/main.mcl

@@ -0,0 +1,5 @@
+import "second.mcl"
+
+import "os" # this fixes it but the wrong way
+
+include second.xclass

lang/interpret_test/TestAstFunc1/importscope1/second.mcl

@@ -0,0 +1,10 @@
+import "fmt"
+
+class xclass {
+	# note that `os` is not imported here
+	$aaa = if os.is_debian() { "bbb" }  else { "ccc" }
+
+	print "${aaa}" {
+		msg => "hello",
+	}
+}

lang/interpret_test/TestAstFunc1/importscope2.graph

@@ -0,0 +1,10 @@
+Edge: call:os.is_debian() -> if(call:os.is_debian()) # c
+Edge: if(call:os.is_debian()) -> var(aaa) # aaa
+Edge: str(bbb) -> if(call:os.is_debian()) # a
+Edge: str(ccc) -> if(call:os.is_debian()) # b
+Vertex: call:os.is_debian()
+Vertex: if(call:os.is_debian())
+Vertex: str(bbb)
+Vertex: str(ccc)
+Vertex: str(hello)
+Vertex: var(aaa)

lang/interpret_test/TestAstFunc1/importscope2/main.mcl

@@ -0,0 +1,3 @@
+import "second.mcl"
+
+include second.xclass

lang/interpret_test/TestAstFunc1/importscope2/second.mcl

@@ -0,0 +1,11 @@
+import "fmt"
+
+class xclass {
+	import "os" # we can also use a scoped local import
+
+	$aaa = if os.is_debian() { "bbb" }  else { "ccc" }
+
+	print "${aaa}" {
+		msg => "hello",
+	}
+}

lang/interpret_test/TestAstFunc1/recursive_class1.graph

@@ -0,0 +1 @@
+# err: err2: recursive class `c1` found

lang/interpret_test/TestAstFunc1/recursive_class1/main.mcl

@@ -0,0 +1,12 @@
+import "fmt"
+$max = 3
+include c1(0) # start at zero
+class c1($count) {
+	if $count == $max {
+		test "done" {
+			stringptr => fmt.printf("count is %d", $count),
+		}
+	} else {
+		include c1($count + 1) # recursion not supported atm
+	}
+}

lang/interpret_test/TestAstFunc1/recursive_class2.graph

@@ -0,0 +1 @@
+# err: err2: class `c1` does not exist in this scope

lang/interpret_test/TestAstFunc1/recursive_class2/main.mcl

@@ -0,0 +1,24 @@
+# this currently fails with: "class `c1` does not exist in this scope"
+# instead of: "recursive class `c1` found" or "recursive class `c2` found"
+# ideally, we'd consider allowing finite (static) recursion such as this...
+import "fmt"
+$max = 5
+include c1(0) # start at zero
+class c1($count) {
+	if $count == $max {
+		test "done" {
+			stringptr => fmt.printf("count is %d", $count),
+		}
+	} else {
+		include c2($count + 1) # recursion not supported atm
+	}
+}
+class c2($count) {
+	if $count == $max {
+		test "done" {
+			stringptr => fmt.printf("count is %d", $count),
+		}
+	} else {
+		include c1($count + 1) # recursion not supported atm
+	}
+}

lang/lang_test.go

@@ -588,6 +588,32 @@ func TestInterpretMany(t *testing.T) {
 			graph: graph,
 		})
 	}
+	{
+		graph, _ := pgraph.NewGraph("g")
+		r1, _ := engine.NewNamedResource("test", "hey1")
+		r2, _ := engine.NewNamedResource("test", "hey2")
+		x1 := r1.(*resources.TestRes)
+		x2 := r2.(*resources.TestRes)
+		s1, s2 := "hey", "hey"
+		x1.StringPtr = &s1
+		x2.StringPtr = &s2
+		graph.AddVertex(x1, x2)
+		testCases = append(testCases, test{
+			name: "double include with out of order variable in parent scope and scoped var",
+			code: `
+			include c1($foo + "1")
+			include c1($foo + "2")
+			class c1($a) {
+				test $a {
+					stringptr => $foo,
+				}
+			}
+			$foo = "hey"
+			`,
+			fail:  false,
+			graph: graph,
+		})
+	}
 	{
 		graph, _ := pgraph.NewGraph("g")
 		r1, _ := engine.NewNamedResource("test", "t1")

lang/structs.go

@@ -1990,7 +1990,6 @@ func (obj *StmtIf) Output() (*interfaces.Output, error) {
 // their order of definition.
 type StmtProg struct {
 	data  *interfaces.Data
-	// XXX: should this be copied when we run Interpolate here or elsewhere?
 	scope *interfaces.Scope // store for use by imports
 
 	// TODO: should this be a map? if so, how would we sort it to loop it?
@@ -2054,6 +2053,7 @@ func (obj *StmtProg) Interpolate() (interfaces.Stmt, error) {
 	}
 	return &StmtProg{
 		data:        obj.data,
+		scope:       obj.scope,
 		importProgs: obj.importProgs, // TODO: do we even need this here?
 		importFiles: obj.importFiles,
 		Prog:        prog,
@@ -2451,7 +2451,11 @@ func (obj *StmtProg) importScopeWithInputs(s string, scope *interfaces.Scope, pa
 // can't follow one (perhaps it wasn't downloaded yet, and is missing) then it
 // leaves some information about these missing imports in the AST and errors, so
 // that a subsequent AST traversal (usually via Apply) can collect this detailed
-// information to be used by the downloader.
+// information to be used by the downloader. When it propagates the scope
+// downwards, it first pushes it into all the classes, and then into everything
+// else (including the include stmt's) because the include statements require
+// that the scope already be known so that it can be combined with the include
+// args.
 func (obj *StmtProg) SetScope(scope *interfaces.Scope) error {
 	newScope := scope.Copy()
 
@@ -2617,9 +2621,20 @@ func (obj *StmtProg) SetScope(scope *interfaces.Scope) error {
 
 	obj.scope = newScope // save a reference in case we're read by an import
 
+	// first set the scope on the classes, since it gets used in include...
+	for _, x := range obj.Prog {
+		if _, ok := x.(*StmtClass); !ok {
+			continue
+		}
+		if err := x.SetScope(newScope); err != nil {
+			return err
+		}
+	}
+
 	// now set the child scopes (even on bind...)
 	for _, x := range obj.Prog {
-		// skip over *StmtClass here (essential for recursive classes)
+		// NOTE: We used to skip over *StmtClass here for recursion...
+		// Skip over *StmtClass here, since we already did it above...
 		if _, ok := x.(*StmtClass); ok {
 			continue
 		}
@@ -2853,6 +2868,8 @@ func (obj *StmtFunc) Output() (*interfaces.Output, error) {
 // TODO: We don't currently support defining polymorphic classes (eg: different
 // signatures for the same class name) but it might be something to consider.
 type StmtClass struct {
+	scope *interfaces.Scope // store for referencing this later
+
 	Name string
 	Args []*Arg
 	Body interfaces.Stmt // probably a *StmtProg
@@ -2896,6 +2913,7 @@ func (obj *StmtClass) Interpolate() (interfaces.Stmt, error) {
 	}
 
 	return &StmtClass{
+		scope: obj.scope,
 		Name:  obj.Name,
 		Args:  args, // ensure this has length == 0 instead of nil
 		Body:  interpolated,
@@ -2906,6 +2924,7 @@ func (obj *StmtClass) Interpolate() (interfaces.Stmt, error) {
 // necessary in order to reach this, in particular in situations when a bound
 // expression points to a previously bound expression.
 func (obj *StmtClass) SetScope(scope *interfaces.Scope) error {
+	obj.scope = scope // store for later
 	return obj.Body.SetScope(scope)
 }
 
@@ -3038,6 +3057,15 @@ func (obj *StmtInclude) SetScope(scope *interfaces.Scope) error {
 		return fmt.Errorf("include already contains a class pointer")
 	}
 
+	// make sure to propagate the scope to our input args!
+	if obj.Args != nil {
+		for _, x := range obj.Args {
+			if err := x.SetScope(scope); err != nil {
+				return err
+			}
+		}
+	}
+
 	for i := len(scope.Chain) - 1; i >= 0; i-- { // reverse order
 		x, ok := scope.Chain[i].(*StmtInclude)
 		if !ok {
@@ -3073,7 +3101,10 @@ func (obj *StmtInclude) SetScope(scope *interfaces.Scope) error {
 	}
 	obj.class = copied
 
-	newScope := scope.Copy()
+	// We start with the scope that the class had, and we augment it with
+	// our parameterized arg variables, which will be needed in that scope.
+	newScope := obj.class.scope.Copy()
+	// Add our args `include foo(42, "bar", true)` into the class scope.
 	for i, arg := range obj.class.Args { // copy
 		newScope.Variables[arg.Name] = obj.Args[i]
 	}
@@ -3082,6 +3113,12 @@ func (obj *StmtInclude) SetScope(scope *interfaces.Scope) error {
 	newScope.Chain = append(newScope.Chain, obj.orig) // add stmt to list
 	newScope.Classes[obj.Name] = copied               // overwrite with new pointer
 
+	// NOTE: This would overwrite the scope that was previously set here,
+	// which would break the scoping rules. Scopes are propagated into
+	// class definitions, but not into include definitions. Which is why we
+	// need to use the original scope of the class as it was set as the
+	// basis for this scope, so that we overwrite it only with the arg
+	// changes.
 	if err := obj.class.SetScope(newScope); err != nil {
 		return err
 	}
@@ -5056,6 +5093,7 @@ func (obj *ExprCall) Interpolate() (interfaces.Expr, error) {
 		args = append(args, interpolated)
 	}
 	return &ExprCall{
+		scope: obj.scope,
 		typ:   obj.typ,
 		Name:  obj.Name,
 		Args:  args,
@@ -5446,6 +5484,7 @@ func (obj *ExprVar) Init(*interfaces.Data) error { return nil }
 // support variable, variables or anything crazy like that.
 func (obj *ExprVar) Interpolate() (interfaces.Expr, error) {
 	return &ExprVar{
+		scope: obj.scope,
 		Name:  obj.Name,
 	}, nil
 }