Fixed safe navigation into properties

[alexnask]

... after a lot of debugging :)

This is one of those fixes that appear trivial but are really insidious.

Here is an overview of the bug:

SafeNavigation creates a chain of ternary operations containing VariableAccesses compared to null and null literals.
Each step is obviously constructed from the previous one, as accesses get deeper and deeper.

The most obvious (as well as optimal) solution to do so is to reuse VariableAccesses as the expr field of the next VariableAccess.
However, it turns out that VariableAccess (correctly) tries to skip resolving if it has already been done.
This means that since every subsequent VariableAccess points to the current VariableAccess in the ternary chain, each unique property VariableAccess was only replaced with a function call once (leading to 1 property call per level as I pointed out on the issue).

The fix proposed is to let a VariableAccess resolve again if it was replaced by a property.
This makes sense since the only way to get resolved again is when we are lead through another Node and need to be replaced once more.
This could also stop other bugs from showing up under similar circumstances (many Nodes pointing to a VariableAccess -> fails when it is a property).

Another fix would be to clone the VariableAccesses in SafeNavigation when constructing the ternary chain but I believe this is the optimal solution.

I will be adding tests if you require them and probably bringing this to ooc-lang/rock (with tests).

[thomasfanell]

Thanks @shamanas, i'll start testing and reviewing this tomorrow. If you want, we would appreciate if you could write some simple test-cases (code samples) right here in this PR.

[marcusnaslund]

@thomasfanell There are test cases associated with this bounty already. :) https://github.com/magic-lang/bounty/blob/master/safe-navigation-operator/test.ooc

[alexnask]

@marcusnaslund
The bounty's testcase uses safe navigation into functions, which is not supported.
I will be providing some tests (and adding them to the sam test suite) later.

EDIT: Also, the bounty's test contains infinite recursion ;) (derivativeMember := Polynomial new(0) in the class's defaults)

[alexnask]

Here is a quick test using both covers and properties:

Bar: cover {
    calc ::= Foo new()
    nullFoo := null as Foo
}

Foo: class {
    bar: Bar

    init: func
}

foo := Foo new() $ bar calc $ bar calc $ bar nullFoo

raise(foo != null, "foo is not null")

[alexnask]

And here is one more akin to the bounty's testcase:

Polynomial: class {
    power: Int

    derivativeMember: This
    derivativeProperty ::= This new(this power - 1)

    init: func (=power) {
        derivativeMember = power > 0 ? This new(power - 1) : null as This
    }
}

basePoly := Polynomial new(8)

childA := basePoly $ derivativeProperty $ derivativeProperty $ derivativeProperty
childB := basePoly $ derivativeProperty $ derivativeMember $ derivativeProperty $ derivativeMember

raise(childA power != 5, "childA power is not 5")
raise(childB power != 4, "childB power is not 4")

[thomasfanell]

@shamanas how hard would it be to make this work with methods as well?

[alexnask]

@thomasfanell
I don't think it would be too difficult, just a couple of changes in nagaqueen and a little tweaking in rock.

[alexnask]

Although I would need to be careful about double evaluation.
I think it would be doable in a day though.

[vendethiel]

nice work :)

[alexnask]

@thomasfanell
I will probably implement safe navigation into methods tomorrow or on Thursday in another PR.

[thomasfanell]

Closing this in favour of your PR in #47