magic-lang / rock

ooc compiler written in ooc
http://ooc-lang.org/
MIT License
14 stars 4 forks source link

Added method support to safe navigation operator #47

Closed alexnask closed 8 years ago

alexnask commented 8 years ago

Also contains the property fix introduced in #38

For a description of the property fix, please refer to the (closed) PR linked above.

The way this works is exactly as you would expect:
Safe navigation sections now accept accesses and function calls.
To achieve this, each section is a "chain" of accesses and method calls constructed in AstBuilder.

The base expression as well as sections that contain functions each generate a variable declaration and are replaced with an assignment of that variable to its original expression to avoid double evaluation.

Each section is chained to the previous one (either to the new generated variable or to the access chain).
Then, we generate a chain of ternary expressions that check if each section is equal to NULL and return NULL in that case or the next ternary expression if it is not.

marcusnaslund commented 8 years ago

Really nice. I'm so happy to see this. :)

alexnask commented 8 years ago

FYI I don't think I will be bringing that to ooc-lang, which is pretty much in feature freeze.

Also, here is a test, I'm going to be doing a lot more now:

Bar: cover {
    foo: Foo
}

Foo: class {
    init: func

    bar: Bar
    baz ::= This new()

    print: func (str: String) -> This {
        str println()
        This new()
    }
}

Foo new() $ print("Hello") baz $ print("World") bar foo $ print("NOT PRINTED YO")

This includes: methods, regular fields, covers, properties.

alexnask commented 8 years ago

The property safe navigation bounty almost compiles as is, here is the correct working test:

Polynomial: class {
    power: Int
    derivativeMember: This
    derivativeProperty ::= This new(this power - 1)
    derivativeFunc: func -> Polynomial { this power > 0 ? This new(this power - 1) : null }
    init: func (=power) {
        derivativeMember = derivativeFunc()
    }
}

somePolynomial := Polynomial new(8)
thirdDerivativeA: Polynomial
thirdDerivativeB: Polynomial
thirdDerivativeC: Polynomial

thirdDerivativeA = somePolynomial $ derivativeMember $ derivativeMember $ derivativeMember
thirdDerivativeB = somePolynomial $ derivativeProperty $ derivativeProperty $ derivativeProperty
thirdDerivativeC = somePolynomial $ derivativeFunc() $ derivativeFunc() $ derivativeFunc()

raise(thirdDerivativeA == null, "thirdDerivativeA is null")
raise(thirdDerivativeB power != 5, "thirdDerivativeB power is not 5")
raise(thirdDerivativeC power != 5, "thirdDerivativeC power is not 5")

// This should not crash
thirdDerivativeD := Polynomial new(2) $ derivativeFunc() $ derivativeFunc() $ derivativeFunc() $ derivativeFunc() 
raise(thirdDerivativeD != null, "thirdDerivativeD is not null")

(The last raise used to check for power while thirdDerivativeD becomes null)

alexnask commented 8 years ago

According to my testing, this is really solid (and there is no reason why it shouldn't be since all it does is be replaced with existing AST constructs).
However, there is one potential issue with it: It doesn't avoid property double evaluation.
This is due to the way it is structured but I do think it is not a huge issue since property getters should probably not be computationally expensive or have huge side effects (if they do, you should probably use functions instead).

alexnask commented 8 years ago

Adding a proper description.

thomasfanell commented 8 years ago

Yeah, I'm pretty happy with this and I am close to signing off on it. @marcusnaslund @fredrikbryntesson any final words?

marcusnaslund commented 8 years ago

I can see a potential problem in our code if properties are double-evaluated.

alexnask commented 8 years ago

@marcusnaslund
I do think it's a weakness of the current implementation.
I can attempt a fix to this later today.

Basically what I need to do is resolve the child expressions in SafeNavigation and correctly handle replacing them, then the current code is pretty much correct (since it already avoids double evaluation of functions, which the properties will be replaced with).

Additionally, I will be able to implement better error messages (rather than relying on the ternary expression error messages) and type inference (findCommonRoot).

On it.

marcusnaslund commented 8 years ago

On it.

Awesome. :) I'm so happy to see things moving along fast in the ooc world.

alexnask commented 8 years ago

Seems to do the trick, time for some more testing!

Initial test:

Bar: cover {
    foo: Foo
}

Foo: class {
    init: func

    me: This {
        get {
            count := static 1
            "SIDE EFFECTS! #{count}" println()
            count += 1
            this
        }
    }

    bar: Bar
    baz ::= This new()

    print: func (str: String) -> This {
        str println()
        This new()
    }
}

Foo new() $ me me me print("Hello") baz $ print("World") bar foo $ print("NOT PRINTED YO")

Correctly prints:

SIDE EFFECTS! 1
SIDE EFFECTS! 2
SIDE EFFECTS! 3
Hello
World

Produces this beautiful C code (/s obviously):

(__test_safeNavExpr2 = test__Foo_new(), __test_safeNavExpr2 != (test__Foo*) NULL ? ((test__Foo*) ((__test_safeNavExpr5 = test__Foo___getbaz__(test__Foo_print(test__Foo___getme__(test__Foo___getme__(test__Foo___getme__(__test_safeNavExpr2))), (void*) String__makeStringLiteral("Hello", 5)))) != (test__Foo*) NULL ? ((test__Foo*) ((__test_safeNavExpr6 = test__Foo_print(__test_safeNavExpr5, (void*) String__makeStringLiteral("World", 5))->bar.foo) != (test__Foo*) NULL ? ((test__Foo*) ((__test_safeNavExpr7 = test__Foo_print(__test_safeNavExpr6, (void*) String__makeStringLiteral("NOT PRINTED YO", 14))))) : NULL)) : NULL)) : NULL);
alexnask commented 8 years ago

Passes anything I throw at it.

thomasfanell commented 8 years ago

@shamanas superb! Do you have anything else on the way? Otherwise this looks like a done deal to me. Great work!

alexnask commented 8 years ago

@thomasfanell
I think it is done indeed :)

marcusnaslund commented 8 years ago

Excellent work.

fredrikbryntesson commented 8 years ago

@shamanas Send me a mail at fredrik.bryntesson@vidhance.com to collect bounty :)

thomasfanell commented 8 years ago

Merging via #49

alexnask commented 8 years ago

@fredrikbryntesson
Sent you an email from alex@naskos.email (just posting here for confirmation)

vendethiel commented 8 years ago

FYI I don't think I will be bringing that to ooc-lang, which is pretty much in feature freeze.

Does it means we'll have to choose between using ooc-lang (GC but not as feature-complete) or magic-lang (feature-complete but manual memory management)?

alexnask commented 8 years ago

@vendethiel
Well, when you say it like that, it does feel like ooc should get it too.

Actually, I do think it is fine for this specific change to be brought back to ooc-lang.
However, if some day additional major non-bugfix changes do happen, ooc-lang will probably not adapt them.

(I will take care of this and potentially the tuple changes as well as the correct version logical operator changes in the weekend)

vendethiel commented 8 years ago

that's fine, it can be decided on a case-by-case basis :).

alexnask commented 8 years ago

Sure, in general I thing extending current features in reasonable degrees would be fine for ooc-lang.

Now, if magic-lang ever gets, for example, additional memory management features baked into the language, they would (probably?) not make it into ooc-lang.

vendethiel commented 8 years ago

right, that definitely makes sense :). Those are particular needs for magic-lang, like I think __onheap__.