diff --git a/lang/interpret_test/TestAstFunc1/importscope0.graph b/lang/interpret_test/TestAstFunc1/importscope0.graph new file mode 100644 index 00000000..da5a1e06 --- /dev/null +++ b/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) diff --git a/lang/interpret_test/TestAstFunc1/importscope0/main.mcl b/lang/interpret_test/TestAstFunc1/importscope0/main.mcl new file mode 100644 index 00000000..3d43fe44 --- /dev/null +++ b/lang/interpret_test/TestAstFunc1/importscope0/main.mcl @@ -0,0 +1,3 @@ +import "second.mcl" + +include second.xclass diff --git a/lang/interpret_test/TestAstFunc1/importscope0/second.mcl b/lang/interpret_test/TestAstFunc1/importscope0/second.mcl new file mode 100644 index 00000000..1aa594f0 --- /dev/null +++ b/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", + } +} diff --git a/lang/interpret_test/TestAstFunc1/importscope1.graph b/lang/interpret_test/TestAstFunc1/importscope1.graph new file mode 100644 index 00000000..f70eee55 --- /dev/null +++ b/lang/interpret_test/TestAstFunc1/importscope1.graph @@ -0,0 +1 @@ +# err: err3: func `os.is_debian` does not exist in this scope diff --git a/lang/interpret_test/TestAstFunc1/importscope1/main.mcl b/lang/interpret_test/TestAstFunc1/importscope1/main.mcl new file mode 100644 index 00000000..6623616f --- /dev/null +++ b/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 diff --git a/lang/interpret_test/TestAstFunc1/importscope1/second.mcl b/lang/interpret_test/TestAstFunc1/importscope1/second.mcl new file mode 100644 index 00000000..71373c5a --- /dev/null +++ b/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", + } +} diff --git a/lang/interpret_test/TestAstFunc1/importscope2.graph b/lang/interpret_test/TestAstFunc1/importscope2.graph new file mode 100644 index 00000000..da5a1e06 --- /dev/null +++ b/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) diff --git a/lang/interpret_test/TestAstFunc1/importscope2/main.mcl b/lang/interpret_test/TestAstFunc1/importscope2/main.mcl new file mode 100644 index 00000000..3d43fe44 --- /dev/null +++ b/lang/interpret_test/TestAstFunc1/importscope2/main.mcl @@ -0,0 +1,3 @@ +import "second.mcl" + +include second.xclass diff --git a/lang/interpret_test/TestAstFunc1/importscope2/second.mcl b/lang/interpret_test/TestAstFunc1/importscope2/second.mcl new file mode 100644 index 00000000..a64a1cf4 --- /dev/null +++ b/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", + } +} diff --git a/lang/interpret_test/TestAstFunc1/recursive_class1.graph b/lang/interpret_test/TestAstFunc1/recursive_class1.graph new file mode 100644 index 00000000..57f8b7bc --- /dev/null +++ b/lang/interpret_test/TestAstFunc1/recursive_class1.graph @@ -0,0 +1 @@ +# err: err2: recursive class `c1` found diff --git a/lang/interpret_test/TestAstFunc1/recursive_class1/main.mcl b/lang/interpret_test/TestAstFunc1/recursive_class1/main.mcl new file mode 100644 index 00000000..b3cff2c6 --- /dev/null +++ b/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 + } +} diff --git a/lang/interpret_test/TestAstFunc1/recursive_class2.graph b/lang/interpret_test/TestAstFunc1/recursive_class2.graph new file mode 100644 index 00000000..639a6750 --- /dev/null +++ b/lang/interpret_test/TestAstFunc1/recursive_class2.graph @@ -0,0 +1 @@ +# err: err2: class `c1` does not exist in this scope diff --git a/lang/interpret_test/TestAstFunc1/recursive_class2/main.mcl b/lang/interpret_test/TestAstFunc1/recursive_class2/main.mcl new file mode 100644 index 00000000..fc652155 --- /dev/null +++ b/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 + } +} diff --git a/lang/lang_test.go b/lang/lang_test.go index b3198cd9..11bd3974 100644 --- a/lang/lang_test.go +++ b/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") diff --git a/lang/structs.go b/lang/structs.go index 419bdf28..261206da 100644 --- a/lang/structs.go +++ b/lang/structs.go @@ -1989,8 +1989,7 @@ func (obj *StmtIf) Output() (*interfaces.Output, error) { // the bind statement's are correctly applied in this scope, and irrespective of // their order of definition. type StmtProg struct { - data *interfaces.Data - // XXX: should this be copied when we run Interpolate here or elsewhere? + data *interfaces.Data 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,9 +2913,10 @@ func (obj *StmtClass) Interpolate() (interfaces.Stmt, error) { } return &StmtClass{ - Name: obj.Name, - Args: args, // ensure this has length == 0 instead of nil - Body: interpolated, + scope: obj.scope, + Name: obj.Name, + Args: args, // ensure this has length == 0 instead of nil + Body: interpolated, }, nil } @@ -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,9 +5093,10 @@ func (obj *ExprCall) Interpolate() (interfaces.Expr, error) { args = append(args, interpolated) } return &ExprCall{ - typ: obj.typ, - Name: obj.Name, - Args: args, + scope: obj.scope, + typ: obj.typ, + Name: obj.Name, + Args: args, }, nil } @@ -5446,7 +5484,8 @@ 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{ - Name: obj.Name, + scope: obj.scope, + Name: obj.Name, }, nil }