Handle star expressions

[rocky]

From issue #27 :

rocky:

The ssa-interpreters can't handle pointers properly. I wonder if there is going to be a similar problem here.

carl:

no it wont be. in fact assuming a correctly type checked ast, the code would be so trivial I'd say you should give it a shot. Id start with the array indexing code, its a similar operation.

eval currently punts on type-check star expressions. In commit 67e0250 I tried to handle star expressions, but wasn't successful. So at least it now gives an error indicating that this isn't working.

The star.expr code is my attempt. It fails in some cases because I think there is automatic defeferencing going on in EvalExpr.

[0xfaded]

Because evalX now assumes typed checked input, there is no need to do additional type checking. Remove everything above

var x reflect.Value

And it should work, and the reflect package will panic if something goes awry.

Also, can you really dereference an interface? Carl 2014/01/06 2:58 "R. Bernstein" notifications@github.com:

From issue #27 https://github.com/0xfaded/eval/issues/27

rocky:

The ssa-interpreters can't handle pointers properly. I wonder if there is going to be a similar problem here.

carl:

no it wont be. in fact assuming a correctly type checked ast, the code would be so trivial I'd say you should give it a shot. Id start with the array indexing code, its a similar operation.

eval currently punts on type-check star expressions. In commit 67e0250https://github.com/0xfaded/eval/commit/67e0250I tried to handle star expressions, but wasn't successful. So at least it now gives an error indicating that this isn't working.

The star.expr codehttps://github.com/0xfaded/eval/blob/master/starexpr.gois my attempt. It fails in some cases because I think there is automatic defeferencing going on in EvalExpr.

— Reply to this email directly or view it on GitHubhttps://github.com/0xfaded/eval/issues/28 .

[rocky]

Because evalX now assumes typed checked input, there is no need to do additional type checking.

The call the checker was there do the recursive dance to convert this from an *ast.Expr to an Expr, not for the type checking per se (which isn't in fact done).

But anyway, as alluded to above, there is this dereferencing seems to be going on somewhere already for pointers and that's getting in the way:

In https://github.com/0xfaded/eval/blob/master/demo/repl.go#L157-L158 :

    var alice = Alice{1, "shhh"}
    alicePtr := &alice

No in go-repl:

go> alice
Kind = struct
Type = main.Alice
results[0] = {Bob: 1, Secret: "shhh",}
go> alicePtr
Kind = struct
Type = main.Alice
results[1] = {Bob: 1, Secret: "shhh",}
go> *alicePtr
eval error: invalid indirect (type main.Alice)

Also, can you really dereference an interface?

See http://godoc.org/reflect#Value.Elem

[0xfaded]

now that does look like my fault. one last possible outside cause, is alicePtr added to the env using reflect.ValueOf(&alicePtr)

Carl 2014/01/06 8:05 "R. Bernstein" notifications@github.com:

Because evalX now assumes typed checked input, there is no need to do additional type checking.

The call the checker was there do the recursive dance to convert this from an *ast.Expr to an Expr, not for the type checking per se (which isn't in fact done).

But anyway, as alluded to above, there is this dereferencing seems to be going on somewhere already for pointers and that's getting in the way:

In https://github.com/0xfaded/eval/blob/master/demo/repl.go#L157-L158 :

var alice = Alice{1, "shhh"}
alicePtr := &alice

No in go-repl:

go> alicealiceKind = structType = main.Aliceresults[0] = {Bob: 1, Secret: "shhh",}go> alicePtrKind = structType = main.Aliceresults[1] = {Bob: 1, Secret: "shhh",}go> *alicePtreval error: invalid indirect (type main.Alice)

Also, can you really dereference an interface?

See http://godoc.org/reflect#Value.Elem

— Reply to this email directly or view it on GitHubhttps://github.com/0xfaded/eval/issues/28#issuecomment-31615534 .

[rocky]

one last possible outside cause, is alicePtr added to the env using reflect.ValueOf(&alicePtr)

Were it only so simple. It turns out this definition isn't added via pointer. Two lines below the ones cited above are:

        global_vars["alice"]    = reflect.ValueOf(alice)
        global_vars["alicePtr"] = reflect.ValueOf(alicePtr)

Note: no ampersand. The line numbers cited above will change I clean up this code.

I looked at _makeenv.go and in fact there I do take the address for package variables.

I makes sense to use a pointer in the conversion to reflect value. It saves space. Also, if the value is updated it gets updated back where it belongs.

Using pointers would explain why automatic dereferencing code would be desirable.

As eval starts to cover more things, it looks like one has to be more precise and careful about such things, like automatic dereference.

Also I see that Go allows pointers to pointers. So unconditionally taking the address of package variables an ok thing to do. One just has to make sure the auto-deref is only done when accessing such a variable added to the environment this way.

[rocky]

After having written all of that preachy stuff, it looks like this is my fault. The problem is that I didn't enter alice in by address. Changing that, makes this work.

I'll leave this issue open pending further review.

[0xfaded]

The logic of using pointers for variables is exactly that, so the mutable values can be later updated by the interpreter. The automatic dereference happens at evalIdent for all variables. I'm surprised that accessing Alice, the non pointer, worked at all.

Carl 2014/01/06 11:18 "R. Bernstein" notifications@github.com:

After having written all of that preachy stuff, it looks like this is my fault. The problem is that I didn't enter alice in by address. Changing that, makes this work.

I'll leave this issue open pending further review.

— Reply to this email directly or view it on GitHubhttps://github.com/0xfaded/eval/issues/28#issuecomment-31620352 .

[rocky]

Looks like I did this in https://github.com/0xfaded/eval/commit/9fd16dd9

func DerefValue(v reflect.Value) reflect.Value {
    switch v.Kind() {
    case reflect.Interface, reflect.Ptr:
        return v.Elem()
    default:
        return v
    }
}

It might be interesting to allow both so one can choose to put something in the environment which is mutable or not.

[rocky]

67e0250d22a82fd66796fdfd6b76854e42136d4d

There is this hacky:

    if cexpr, errs = CheckExpr(ctx, starExpr.X, env); len(errs) != 0 {
        for _, cerr := range errs {
            fmt.Printf("%v\n", cerr)
        }
        return nil, false, errors.New("Something wrong checking * expression")
    }

[0xfaded]

Look, obviously this belongs in the checker, and it probably would be very straight forward to add checkstarexpr.go based on one of the other skeletons. Having said that, once the type checker gets integrated a lot of the evaluator condition checking logic will be gutted anyway. If its easier to make a quick stride with a hack then so be it, the evaluator can't really be looked at seriously until the type checker is ready anyway.

Also, if you want a full code review its best to throw it up as a pull request.

[rocky]

Ok. Thanks for the info. Hadn't realized you were concurrently working on the type checker. I agree this is key for now. Slice expressions is just something I've run into a bit so I wanted to add it into the code.

So I'd prefer keeping it there for now to have the functionality and then doing a serious refactor after the type checker is integrated, if that's okay with you.

I won't be adding further to eval until type check integration is done. (We might consider removing inspect.go now that there are lots of other go data structure printers as well as %+v and `%#v1.

On Sat, Jan 11, 2014 at 5:04 AM, Carl Chatfield notifications@github.comwrote:

Look, obviously this belongs in the checker, and it probably would be very straight forward to add checkstarexpr.go based on one of the other skeletons. Having said that, once the type checker gets integrated a lot of the evaluator condition checking logic will be gutted anyway. If its easier to make a quick stride with a hack then so be it, the evaluator can't really be looked at seriously until the type checker is ready anyway.

Also, if you want a full code review its best to throw it up as a pull request.

— Reply to this email directly or view it on GitHubhttps://github.com/0xfaded/eval/issues/28#issuecomment-32092111 .