Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
fasterthanlime opened this issue Nov 17, 2015 · 12 comments
Closed

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

fasterthanlime opened this issue Nov 17, 2015 · 12 comments

Comments

@fasterthanlime
Copy link
Collaborator

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
Copy link
Collaborator Author

Originally reported by @davidhesselbom in #782

@fasterthanlime fasterthanlime changed the title Setting by-ref member of underlying doesn't work Setting by-ref member of underlying cover doesn't work Nov 17, 2015
@davidhesselbom
Copy link
Contributor

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
Copy link
Collaborator Author

@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
Copy link
Contributor

Oh.. right.

@davidhesselbom
Copy link
Contributor

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
Copy link
Contributor

I also tried modifying Point's constructor to

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

Same error: lvalue required...

@fasterthanlime
Copy link
Collaborator Author

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

@davidhesselbom
Copy link
Contributor

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:

  • Breaking up the last line into two lines:
foo := foos[0]
list add(foo bar)
  • Changing bar into a property
    bar: Int { get set }
  • Making Foo a class instead of a cover:
Foo: class {
    bar: Int
    init: func
}

@alexnask
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants