lang: ast: ExprBind is now monomorphic

[gelisam]

An attempt at fixing #722

[gelisam]

3 more fail with field arg0 does not exist, but the MCL code doesn't mention arg0 anywhere. what's arg0?

1. deploy-readfile0.txtar
2. deploy-readfile1.txtar
3. slow-unification2.txtar

[gelisam]

The last 3 fail because I've somehow broken SetScope's recursion detector. I am guessing that $x = $x + 1 no longer triggers the recursion detector because one of those two $xs is wrapped in an ExprTopLevel and the other isn't, something like that. What can you tell me about SetScope's recursion detector?

[purpleidea]

Looking at the code, I don't understand why these tests expect two errors. Does it seem acceptable to change the expected error to be 1 unconsumed generators?

Yes absolutely. Something has clearly changed how the type unification solver sees the world, but this is not necessarily an issue or bad. The important thing is the test still fails with a type unification error as we expect. You can safely update these!

[purpleidea]

what's arg0?

I suspect it's coming from here...

https://github.com/purpleidea/mgmt/blob/a6d22a5a4b4e8a4fa25bf5f68f113dd686d72e00/lang/ast/structs.go#L7825

In this instance, it looks like we don't know what to name the args, which are used to build the types for unification, so we invent an arbitrary name... Maybe this should use the standard a, b, c, convention instead...

This shouldn't affect unification though because func(a str) bool should match func (arg0 str) bool just fine.

So maybe this is a unification bug elsewhere?

[purpleidea]

what's arg0? (continued)

deploy-readfile0.txtar
deploy-readfile1.txtar
slow-unification2.txtar

Of note, all of these use import to pull in a subsequent file... So maybe there's an issue with that and how the stuff pulled in gets scoped... This happens in SetScope from StmtProg...

[purpleidea]

What can you tell me about SetScope's recursion detector?

Well, there are two ways to find this. The first: look for TopologicalSort! (in ast/structs.go of course) you might want the first one for import related stuff:

https://github.com/purpleidea/mgmt/blob/a6d22a5a4b4e8a4fa25bf5f68f113dd686d72e00/lang/ast/structs.go#L3089

Or this one for the ordering stage stuff (more likely $x = $x I think):

https://github.com/purpleidea/mgmt/blob/a6d22a5a4b4e8a4fa25bf5f68f113dd686d72e00/lang/ast/structs.go#L3711

Lastly for class recursion it's here:

https://github.com/purpleidea/mgmt/blob/a6d22a5a4b4e8a4fa25bf5f68f113dd686d72e00/lang/ast/structs.go#L4490

[gelisam]

So maybe this is a unification bug elsewhere?

I have just fixed two unification bugs introduced by ExprTopLevel, so that seems likely!

[gelisam]

My fix for the loop detection makes me think that polymorphic code might still evade detection. Do we have tests for loops like func f() {f()} and class c() {include c()}?

[purpleidea]

Do we have tests for loops like func f() {f()} and class c() {include c()}?

$ ack 'include' -l
class-capture1.txtar
class-capture2.txtar
class-capture3.txtar
class-capture4.txtar
class-capture5.txtar
class-capture6.txtar
class-capture7.txtar
class-capture8.txtar
class-recursion1.txtar
class-recursion2.txtar
class-shadowing1.txtar
class-shadowing2.txtar
class-shadowing3.txtar
func-gen1.txtar
func-gen2.txtar
slow-unification1.txtar
slow-unification2.txtar

$ ack '#err' class-* func-gen* slow-unification*

(no results)

Apparently not for classes!

And I can't recall doing any recursive funcs!

[purpleidea]

Woops, typo:

ack '# err' class-* func-gen* slow-unification*
class-capture7.txtar
12:# err: errSetScope: class `bar` does not exist in this scope

class-recursion1.txtar
9:# err: errSetScope: recursive reference while setting scope: not a dag

class-recursion2.txtar
12:# err: errSetScope: recursive reference while setting scope: not a dag

class-shadowing3.txtar
15:# error prone, and also require a higher-order FRP, which would add complexity

func-gen1.txtar
14:# err: errSetScope: func `fun1` does not exist in this scope

func-gen2.txtar
17:# err: errSetScope: func `fun2` does not exist in this scope

[purpleidea]

So yes a few basic ones for classes, and none for functions that I know of.

[purpleidea]

Wait, I was wrong again:

ls *recur*
class-recursion1.txtar             stmtfunc-recursive-double.txtar
class-recursion2.txtar             stmtfunc-recursive-no-operators.txtar
lambdafunc-recursive-double.txtar  stmtfunc-recursive.txtar
lambdafunc-recursive.txtar

[gelisam]

Excellent, the two cases I was worried about are covered then.

It's pretty strange that stmtfunc-recursive-double.txtar and stmtfunc-recursive-no-operators.txtar have the exact same contents, probably a mistake?

[gelisam]

Only 4 failing tests left!

test #27 FAIL TestAstFunc2 27   (const1.txtar) could not unify types: only recursive solutions left
test #30 FAIL TestAstFunc2 30   (deploy-readfile0.txtar) field arg0 does not exist
test #31 FAIL TestAstFunc2 31   (deploy-readfile1.txtar) field arg0 does not exist
test #124 FAIL TestAstFunc2 124 (slow-unification2.txtar) field arg0 does not exist

Let's focus on the unification error and hope that it solves the other 3. The const1 test is very short:

# this magic string variable should exist and be "exists"
test $const.res.file.state.exists {}

the constraints on master are correct:

        0xc000685100 var(const.res.file.state.exists) :: ?1
        0xc000684940 str("exists") :: ?2
        ?2 = str
        ?1 = ?2

whereas on this branch the ?2 = str is missing:

        0xc00030fcc0 var(const.res.file.state.exists) :: ?1
        0xc00030e7e0 str("exists") :: ?2
        ?1 = ?2

I don't really understand what const.res.file.state.exists is supposed to be. Builtins like printf require an import "fmt", but this test has no imports, so I guess $const is a special builtin which doesn't need to be imported? The comment certainly implies so.

I am guessing that magic variables use a separate codepath which bypasses the code I have added for the non-magic codepath. Maintaining both codepaths is of course more effort than supporting a single codepath; is this extra effort worth it? What is this magic variable providing over an equivalent imported variable, and where are magic variables defined?

[gelisam]

The slow-unification2.txtar failure is very unusual! I tried to simplify the test file, and this version:

-- main.mcl --
import "datetime"

$now = 0 
print "test1" { 
        msg => if datetime.weekday($now) == "friday" { "TGIF YAY!" } else { "meh..." },
} 
print "test2" { 
        msg => "FLIP",
} 
-- OUTPUT --
Vertex: print[test1]
Vertex: print[test2]

Passes 55% of the time. The original version only passes 10% of the time.

Since none of my changes in this MR involve threads, I am guessing that this is an existing race condition which used to work fine because the timings of the threads was just right, and this MR changes the timings just enough to expose the race condition. The timings probably vary from machine to machine, too, so I bet you and CI will get different results than me.

I am thus inclined to disregard the field arg0 does not exist failures as existing bugs which already exist on master, even though the timings are such that master passes ~100% of the time.

[purpleidea]

It's pretty strange that stmtfunc-recursive-double.txtar and stmtfunc-recursive-no-operators.txtar have the exact same contents, probably a mistake?

Yeah definitely. I probably meant to change the operators in the no-operators version. We could either delete stmtfunc-recursive-no-operators.txtar or change it if you find something valuable to do so.

[purpleidea]

I don't really understand what const.res.file.state.exists is supposed to be. Builtins like printf require an import "fmt", but this test has no imports, so I guess $const is a special builtin which doesn't need to be imported? The comment certainly implies so.

So $const is a magic hierarchy of variables... The one in question is simply a string "exists". If that test fails with a raw string "exists", then I don't know what's wrong. Something you changed I guess. However if it works with a raw string, then we know where to narrow things down... This would be in the importing variables code path.

The libraries for that is here: https://github.com/purpleidea/mgmt/blob/master/lang/funcs/vars/vars.go

Each of them is basically any constant types.Value... And then this function: VarPrefixToVariablesScope is run in lang/lang.go (or lang/interpret_test.go for tests) and that gets used in the top-level scope.

So if it's not working, SetScope is broken.

[purpleidea]

I guess $const is a special builtin which doesn't need to be imported? The comment certainly implies so.

It's part of the top-level scope. It's actually an ExprStr or and ExprBool or whatever the appropriate one is.

Maintaining both codepaths is of course more effort than supporting a single codepath

Are you saying it would be smarter to do "import const" and then const.whatever... ? I really like this idea.

[purpleidea]

Passes 55% of the time. The original version only passes 10% of the time.

There is indeed a race with the function engine unfortunately. I have to spend some time to debug it :(

the field arg0 does not exist failures

But I wouldn't expect to see this error as the intermittent failure. Is that what you see?

[gelisam]

Are you saying it would be smarter to do "import const" and then const.whatever... ? I really like this idea.

In this MR, I added ExprTopLevel to FuncPrefixToFunctionsScope, I think that's the printf codepath? Thus, when I saw that fmt.printf worked but $const didn't, my first thought was that $const probably doesn't use FuncPrefixToFunctionsScope, so that's why it's broken.

I know you added a secret $purpleidea variable, using a completely separate codepath from every other variable, so I bet that variable is broken as well. I assumed that const and $purpleidea were using the same codepath, but now that you mention VarPrefixToVariablesScope, I see that the more important difference between fmt.printf and $const isn't the fmt., it's the $. So the fix must be in VarPrefixToVariablesScope!

[gelisam]

Oh, I know what's wrong! ExprVar used to copy its target and then call Unify() on it. Now, it no longer copies its target, and also it no longer calls Unify() on it, because that would cause the constraints to be added once per use site rather than once per definition. Conversely, StmtBind used not to call Unify() on its definition, and now it does. So for variables bound by StmtBind, the Unify() call is now made in a different spot, but importantly, the call is still made.

The variables introduced by VarPrefixToVariablesScope are not introduced by StmtBind, so there is currently nothing which calls Unify() on them. How was Unify() called on them before the mcl-functions branch? Does it make sense to call Unify() on all the magic variables? If so, from where? Does the caller of the top-level StmtProg.Unify() have access to the list of all the magic Exprs? If it does not make sense to call Unify() an the unused magic variables, is there a function which lists the magic variables used by the program?

[purpleidea]

How was Unify() called on them before the mcl-functions branch?

They were in the scope, so it got called when one used one.

Does it make sense to call Unify() on all the magic variables?

Just the ones that are consumed somewhere I think.

If so, from where?

Whatever pulls them in from the scope calls Unify, no?

Does the caller of the top-level StmtProg.Unify() have access to the list of all the magic Exprs?

Unsure.

If it does not make sense to call Unify() an the unused magic variables,

It does not, I believe...

is there a function which lists the magic variables used by the program?

I mean, the premise of a top-level scope, that we propagate through... Is this not right? If it is, then why can't the top-level scope contain something. If not, then well... I don't know how imports and everything else work...

So I still think it makes sense to have some top-level variables. $hostname being an important one, and for most of them, it probably make sense to move them to a "const" package.

[gelisam]

They were in the scope, so it got called when one used one. [...] Whatever pulls them in from the scope calls Unify

Ok, I'll change it back so that ExprVar calls unify instead of StmtBind. Don't complain that there is one copy of the Unify() constraints for each use site!

[purpleidea]

This is merged in #725 \o/