From dd20bd548687e480cf6539d53d46c41681b1cf78 Mon Sep 17 00:00:00 2001 From: James Shubin Date: Sat, 20 Jan 2024 14:41:56 -0500 Subject: [PATCH] lang: ast: Improve ordering to eliminate false positives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Ordering and DAG detection code is challenging because we need Ordering to do SetScope, but Ordering itself needs to know about scopes. This improved variant should hopefully catch all the scenarios of identically named variables causing invalid loops. Co-authored-by: Samuel Gélineau --- lang/ast/structs.go | 112 +++++++++++++----- .../TestAstFunc2/scope-ordering-dag1.txtar | 13 ++ .../TestAstFunc2/scope-ordering-dag2.txtar | 13 ++ .../TestAstFunc2/scope-ordering-dag3.txtar | 9 ++ .../TestAstFunc2/scope-ordering-dag4.txtar | 9 ++ .../TestAstFunc2/scope-ordering-dag5.txtar | 10 ++ .../TestAstFunc2/scope-ordering-dag6.txtar | 16 +++ 7 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag1.txtar create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag2.txtar create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag3.txtar create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag4.txtar create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag5.txtar create mode 100644 lang/interpret_test/TestAstFunc2/scope-ordering-dag6.txtar diff --git a/lang/ast/structs.go b/lang/ast/structs.go index b9887374..73864231 100644 --- a/lang/ast/structs.go +++ b/lang/ast/structs.go @@ -3173,7 +3173,17 @@ func (obj *StmtProg) Ordering(produces map[string]interfaces.Node) (*pgraph.Grap } } - return graph, cons, nil + // The consumes which have already been matched to one of our produces + // must not be also matched to a produce from our caller. Is that clear? + newCons := make(map[interfaces.Node]string) // don't modify the input map! + for k, v := range cons { + if _, exists := prod[v]; exists { + continue + } + newCons[k] = v // "remaining" values from cons + } + + return graph, newCons, nil } // importScope is a helper function called from SetScope. If it can't find a @@ -4295,6 +4305,18 @@ func (obj *StmtFunc) Ordering(produces map[string]interfaces.Node) (*pgraph.Grap graph.AddEdge(n, k, edge) } + // The consumes which have already been matched to one of our produces + // must not be also matched to a produce from our caller. Is that clear? + //newCons := make(map[interfaces.Node]string) // don't modify the input map! + //for k, v := range cons { + // if _, exists := prod[v]; exists { + // continue + // } + // newCons[k] = v // "remaining" values from cons + //} + // + //return graph, newCons, nil + return graph, cons, nil } @@ -4441,34 +4463,47 @@ func (obj *StmtClass) Ordering(produces map[string]interfaces.Node) (*pgraph.Gra } graph.AddVertex(obj) + prod := make(map[string]interfaces.Node) + for _, arg := range obj.Args { + uid := varOrderingPrefix + arg.Name // ordering id + //node, exists := produces[uid] + //if exists { + // edge := &pgraph.SimpleEdge{Name: "stmtclassarg"} + // graph.AddEdge(node, obj, edge) // prod -> cons + //} + prod[uid] = &ExprParam{Name: arg.Name} // placeholder + } + + newProduces := CopyNodeMapping(produces) // don't modify the input map! + + // Overwrite anything in this scope with the shadowed parent variable! + for key, val := range prod { + newProduces[key] = val // copy, and overwrite (shadow) any parent var + } + // additional constraint... edge := &pgraph.SimpleEdge{Name: "stmtclassbody"} graph.AddEdge(obj.Body, obj, edge) // prod -> cons cons := make(map[interfaces.Node]string) - g, c, err := obj.Body.Ordering(produces) + g, cons, err := obj.Body.Ordering(newProduces) if err != nil { return nil, nil, err } graph.AddGraph(g) // add in the child graph - for k, v := range c { // c is consumes - x, exists := cons[k] - if exists && v != x { - return nil, nil, fmt.Errorf("consumed value is different, got `%+v`, expected `%+v`", x, v) - } - cons[k] = v // add to map - - n, exists := produces[v] - if !exists { + // The consumes which have already been matched to one of our produces + // must not be also matched to a produce from our caller. Is that clear? + newCons := make(map[interfaces.Node]string) // don't modify the input map! + for k, v := range cons { + if _, exists := prod[v]; exists { continue } - edge := &pgraph.SimpleEdge{Name: "stmtclass"} - graph.AddEdge(n, k, edge) + newCons[k] = v // "remaining" values from cons } - return graph, cons, nil + return graph, newCons, nil } // SetScope sets the scope of the child expression bound to it. It seems this is @@ -7182,11 +7217,29 @@ func (obj *ExprFunc) Ordering(produces map[string]interfaces.Node) (*pgraph.Grap } graph.AddVertex(obj) + prod := make(map[string]interfaces.Node) + for _, arg := range obj.Args { + uid := varOrderingPrefix + arg.Name // ordering id + //node, exists := produces[uid] + //if exists { + // edge := &pgraph.SimpleEdge{Name: "stmtexprfuncarg"} + // graph.AddEdge(node, obj, edge) // prod -> cons + //} + prod[uid] = &ExprParam{Name: arg.Name} // placeholder + } + + newProduces := CopyNodeMapping(produces) // don't modify the input map! + + // Overwrite anything in this scope with the shadowed parent variable! + for key, val := range prod { + newProduces[key] = val // copy, and overwrite (shadow) any parent var + } + cons := make(map[interfaces.Node]string) - // TODO: do we need ordering for other aspects of ExprFunc ? + // XXX: do we need ordering for other aspects of ExprFunc ? if obj.Body != nil { - g, c, err := obj.Body.Ordering(produces) + g, c, err := obj.Body.Ordering(newProduces) if err != nil { return nil, nil, err } @@ -7196,23 +7249,20 @@ func (obj *ExprFunc) Ordering(produces map[string]interfaces.Node) (*pgraph.Grap edge := &pgraph.SimpleEdge{Name: "exprfuncbody"} graph.AddEdge(obj.Body, obj, edge) // prod -> cons - for k, v := range c { // c is consumes - x, exists := cons[k] - if exists && v != x { - return nil, nil, fmt.Errorf("consumed value is different, got `%+v`, expected `%+v`", x, v) - } - cons[k] = v // add to map - - n, exists := produces[v] - if !exists { - continue - } - edge := &pgraph.SimpleEdge{Name: "exprfunc"} - graph.AddEdge(n, k, edge) - } + cons = c } - return graph, cons, nil + // The consumes which have already been matched to one of our produces + // must not be also matched to a produce from our caller. Is that clear? + newCons := make(map[interfaces.Node]string) // don't modify the input map! + for k, v := range cons { + if _, exists := prod[v]; exists { + continue + } + newCons[k] = v // "remaining" values from cons + } + + return graph, newCons, nil } // SetScope stores the scope for later use in this resource and its children, diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag1.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag1.txtar new file mode 100644 index 00000000..3094d09f --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag1.txtar @@ -0,0 +1,13 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +class foo() { + $x = "hello" + $y = $x + test $y + "a" {} +} +include foo() as c +$x = $c.y +test $x + "b" {} +-- OUTPUT -- +Vertex: test[helloa] +Vertex: test[hellob] diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag2.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag2.txtar new file mode 100644 index 00000000..d8c78e6c --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag2.txtar @@ -0,0 +1,13 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +class foo() { + $y = $x # would this also work with the Ordering(depth) idea? + test $y + "a" {} +} +$x = "hello" +include foo() as c +$z = $c.y +test $z + "b" {} +-- OUTPUT -- +Vertex: test[helloa] +Vertex: test[hellob] diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag3.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag3.txtar new file mode 100644 index 00000000..df9762ca --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag3.txtar @@ -0,0 +1,9 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +$id = func($x) { + $x +} +$x = $id("hello") +test $x {} +-- OUTPUT -- +Vertex: test[hello] diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag4.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag4.txtar new file mode 100644 index 00000000..2460527d --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag4.txtar @@ -0,0 +1,9 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +func id($x) { + $x +} +$x = id("hello") +test $x {} +-- OUTPUT -- +Vertex: test[hello] diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag5.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag5.txtar new file mode 100644 index 00000000..688207b6 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag5.txtar @@ -0,0 +1,10 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +class c1($x) { + $y = $x +} +include c1("hello") as c +$x = $c.y +test $x {} +-- OUTPUT -- +Vertex: test[hello] diff --git a/lang/interpret_test/TestAstFunc2/scope-ordering-dag6.txtar b/lang/interpret_test/TestAstFunc2/scope-ordering-dag6.txtar new file mode 100644 index 00000000..156a2135 --- /dev/null +++ b/lang/interpret_test/TestAstFunc2/scope-ordering-dag6.txtar @@ -0,0 +1,16 @@ +-- main.mcl -- +# test that ordering works and doesn't produce a dag in this tricky scenario +class bar($z) { + $zz = $z +} +class foo() { + include bar("hello") as x # bind to x + $y = $x.zz + test $y + "a" {} +} +include foo() as c +include bar($c.y) as x +test $x.zz + "b" {} +-- OUTPUT -- +Vertex: test[helloa] +Vertex: test[hellob]