ooc-lang / rock

:ocean: self-hosted ooc compiler that generates c99
http://ooc-lang.org/
MIT License
403 stars 40 forks source link

Setting by-ref member of underlying cover doesn't work #949

Closed fasterthanlime closed 8 years ago

fasterthanlime commented 8 years ago

This works:

Foo: cover {
    bar: Int

    baz: Int {
        set (baz) { this bar = baz }
    }
}

f: Foo
f bar = 1
f baz = 2

if (f bar != 2) {
    raise("expected f bar == 2")
}

aka baz's setter accepts this by reference, correctly

But this doesn't:

UnderFoo: cover {
    bar: Int
}

Foo: cover from UnderFoo extends UnderFoo {
    baz: Int {
        set (baz) { this bar = baz }
    }
}

f: Foo
f bar = 1
f baz = 2

if (f bar != 2) {
    raise("expected f bar == 2")
}

because it generates code like:

void blah__Foo___setbaz__(blah__Foo* this, lang_Numbers__Int baz) {
    ((blah__UnderFoo)(*this)).bar = baz;
}

whereas it should be generating code like:

void blah__Foo___setbaz__(blah__Foo* this, lang_Numbers__Int baz) {
    *((blah__UnderFoo*)this).bar = baz;
}

I'm fairly confident at this point this is a ReferenceType, so we should look into what goes on when you cast a ReferenceType to another


edit: even this code should work in fact:

void blah__Foo___setbaz__(blah__Foo* this, lang_Numbers__Int baz) {
    *this.bar = baz;
}

because Foo is simply a typedef:

typedef blah__UnderFoo blah__Foo;

and we know that in the AST because it's a cover-from, so we could just go ahead and ignore the cast

fasterthanlime commented 8 years ago

Originally reported by @davidhesselbom in #782

davidhesselbom commented 8 years ago

Maybe it's not important or relevant, but don't forget that, by contrast,

Foo: cover from UnderFoo extends UnderFoo {
    baz: Int {
        get { this bar }
    }
}

works just fine.

fasterthanlime commented 8 years ago

@davidhesselbom a getter isn't by-ref, so the cast Foo -> UnderFoo works just fine (it's useless and ignored by the C compiler)

davidhesselbom commented 8 years ago

Oh.. right.

davidhesselbom commented 8 years ago

I also found this:

Vector: cover {
    x, y: Int
    init: func@ (=x, =y)
}
Point: cover from Vector extends Vector {
    init: func@ (=x, =y)
}

point := Point new(2, 3)
point x toString() println()

fails to compile with

error: lvalue required as left operand of assignment
     init: func@ (=x, =y)

in Point's constructor, but if I comment out Point's constructor, I get

No such function new(Int, Int) for `PointClass`

Is this also a bug?

davidhesselbom commented 8 years ago

I also tried modifying Point's constructor to

    init: func@ (x, y: Int ) { this x = x; this y = y }

Same error: lvalue required...

fasterthanlime commented 8 years ago

@davidhesselbom same bug: a method in Foo is trying to assign to a member of UnderFoo in an by-ref method

davidhesselbom commented 8 years ago

This (also posted in #940 now) doesn't give quite the same error, but is perhaps related:

import structs/ArrayList

Foos: class {
    init: func
    operator [] (index: Int) -> Foo {
        Foo new()
    }
}
Foo: cover {
    bar: Int
    init: func@
}

foos := Foos new()
list := ArrayList<Int> new()
list add(foos[0] bar)

This fails to build with the error

lvalue required as unary ‘&’ operand
list add(foos[0] bar)

However, any of the 3 following changes will independently fix the build error:

foo := foos[0]
list add(foo bar)
    bar: Int { get set }
Foo: class {
    bar: Int
    init: func
}
alexnask commented 8 years ago

Hmm, I wonder if the cast and deref happen in the middle end somewhere or are generated by the backend.

I wish we had a flag that dumped the AST after all transformations in some format that could be easily visualized.

Anyway, I'll look into this today, though it looks like one of those things that is really difficult to track.

alexnask commented 8 years ago

This is actually an issue with any assignment to an extended cover value.

The issue is the generated cast to a scalar type followed by the dot operator (which seems not to be an lvalue).

I like Amos' solution of just generating (*foo).bar (for by-ref accesses) or foo.bar for any cover access, I guess I'll have to refresh my backend knowledge :P

davidhesselbom commented 8 years ago

Is there a way I can work around it without making any of the changes I found to work, and without leading to a double deref when the compiler is fixed? Something like

list add((foos[0]*)@ bar) // yes, I'm just making things up here

?

alexnask commented 8 years ago

@fasterthanlime

It seems making rock never output casts for accesses to covers works and I think it is a correct thing to do.

I cannot think of any edgecase, since cover "hierarchy" is expressed through typedefs.
PR will be up in a bit.