rock picks the wrong variable when choosing between tuple component and instance variable #903

Closed
davidhesselbom opened this Issue Jul 10, 2015 · 12 comments

Projects

None yet

2 participants

@davidhesselbom
Contributor

Test code

// TupleTest.ooc
Foo: class {
    x, y: Int
    init: func (=x, =y)
    init: func ~fromInt (input: Int) {
        (x, y) := This makeXY(input)
        This new(x, y)
    }
    makeXY: static func (input: Int) -> (Int, Int) { (input, input) }
}

bar := Foo new(1337)
println("#{bar x}, #{bar y}")

Expected result

Print 1337, 1337

Current result

Print 0, 0

rock picks the instance variable x instead of the variable x in the tuple.

void ___TupleTest__Foo_init_fromInt(___TupleTest__Foo* this, lang_Numbers__Int input) {
    #line 9 "/home/dhesselbom/versioned/rock/./TupleTest.ooc"
    lang_Numbers__Int x;
    #line 9 "/home/dhesselbom/versioned/rock/./TupleTest.ooc"
    lang_Numbers__Int y;
    #line 9 "/home/dhesselbom/versioned/rock/./TupleTest.ooc"
    ___TupleTest__Foo_makeXY(&(x), &(y), input);
    #line 10 "/home/dhesselbom/versioned/rock/./TupleTest.ooc"
    ___TupleTest__Foo_new(this->x, this->y);
}

Suggested solution

Whenever a variable name e.g. x causes ambiguity between the local variable x and this x, rock should pick the local variable unless this is written explicitly. I think that's what happens normally — the problem described above only seems to occur when tuples are involved.

BTW, really cool to see so much action around here! :)

@fasterthanlime
Collaborator

Lil' note, now that sam-assert is used pretty much everywhere in rock, using it makes my life even easier when fixing bugs.

Adaptation of your example above:

// Test for https://github.com/fasterthanlime/rock/issues/903

describe("tuple variable decls should have precedence over members", ||
  bar := Foo new(1337)
  expect(1337, bar x)
  expect(1337, bar y)
)

// support code

Foo: class {
    x := 1
    y := 2
    init: func (=x, =y)
    init: func ~fromInt (input: Int) {
        (x, y) := This makeXY(input)
        This new(x, y)
    }
    makeXY: static func (input: Int) -> (Int, Int) { (input, input) }
}

Compile with rock --use=sam-assert something.ooc or sam test rock.use --test test/compiler/something.ooc. (Or add the use sam-assert yourself on top)

Tests like that are ready to be dropped into rock's test folder :)

@fasterthanlime
Collaborator

Also! Even though there's a real bug in there, your sample would fail anyway :) it's just building another instance of Foo. The correct version reads:

// *snip*
    init: func ~fromInt (input: Int) {
        (x, y) := This makeXY(input)
        init(x, y) // here
    }
// *snip*
@davidhesselbom
Contributor

The correct version reads:

Haha, oh. But... I'm pretty sure it "worked" (as in, it gave the expected output) when I replaced makeXY with makeX (which just returned an Int, so there was no tuple), while still calling This new. Can't verify right now though, since I'm on a rock-less machine. Is either This init or this init correct?

Tests like that are ready to be dropped into rock's test folder :)

Oh, cool! Does that mean that next time I run into something like this, I can (and/or should) make a pull request with a failing test in addition to the issue...? (I don't expect I'll have much luck trying to fix it myself)

Again, way cool to see so much going on here. This particular bug had me scratching my head for a long time, and not having to worry about (me or my co-workers) running into it again means fewer crappy days. Really glad to see it get fixed so quickly <3

@fasterthanlime
Collaborator

Haha, oh. But... I'm pretty sure it "worked" (as in, it gave the expected output) when I replaced makeXY with makeX (which just returned an Int, so there was no tuple).

But... but that's impossible :( Calling 'This new' doesn't act on the same instance (hopefully it's clear why?). Could it be another bug in disguise?

Just tested what I think you're talking about, and it definitely still fails for me:

screen shot 2015-07-10 at 20 30 29

Oh, cool! Does that mean that next time I run into something like this, I can (and/or should) make a pull request with a failing test in addition to the issue...?

Sure, that's a good idea! If it's only one test, copy-pasting is fine too. I like having them in the issue description, for some reason.

This particular bug had me scratching my head for a long time

Oh yeah, I can imagine. Never stumbled upon it myself, but wrong resolves are.. ugh. The worst. Hopefully my fix is solid enough, don't hesitate to open another issue if something similar rears its ugly head.

Really glad to see it get fixed so quickly <3

Well, I'm.. kind of on a roll this week, but don't get too used to it, gotta get back to work. Just want to cut a new release beforehand.

@davidhesselbom
Contributor

But... but that's impossible :( Calling 'This new' doesn't act on the same instance (hopefully it's clear why?). Could it be another bug in disguise?

Ugh. I really shouldn't try to recollect what happened. I'll try again on that same machine on Monday and see if my brain's just pulling its own leg...

@fasterthanlime fasterthanlime modified the milestone: 0.9.10 Jul 10, 2015
@davidhesselbom
Contributor

So, after pulling the latest version from the master branch, this

// TupleTest.ooc
Foo: class {
    x, y: Int
    init: func (=x, =y)
    init: func ~fromInt (input: Int) {
        (x, y) := This makeXY(input)
        init(x, y)
    }
    makeXY: static func (input: Int) -> (Int, Int) { (input, input) }
}

bar := Foo new(1337)
println("#{bar x}, #{bar y}")

still generates this

void TupleTest__Foo_init_fromInt(TupleTest__Foo* this, lang_Numbers__Int input) {
    #line 6 "/home/dhesselbom/versioned/rock/TupleTest.ooc"
    lang_Numbers__Int x;
    #line 6 "/home/dhesselbom/versioned/rock/TupleTest.ooc"
    lang_Numbers__Int y;
    #line 6 "/home/dhesselbom/versioned/rock/TupleTest.ooc"
    TupleTest__Foo_makeXY(&(x), &(y), input);
    #line 7 "/home/dhesselbom/versioned/rock/TupleTest.ooc"
    TupleTest__Foo_init((TupleTest__Foo*) this, this->x, this->y);
}

I also tried running sam test in the rock folder, after which 37 out of 169 tests (which all ran really fast) passed and 132 tests failed. What am I missing?

@davidhesselbom
Contributor

Uh, forgot to make install. Sorry about that... still, even with that dealt with, and rock --version printing rock 0.9.11-head codename sapporo, I still get poor results with sam test: 51 failures out of 169 (although it happens a lot slower now, about half a second for each test instead of 0 or 1 ms).

It seems most, if not all, of the failing tests fail because error No such function describe or error No such function expect.

@fasterthanlime
Collaborator

Yeah you need to update sam :)

@fasterthanlime
Collaborator

rock should probably err since it's not picking up sam-assert though

@davidhesselbom
Contributor

Oh. Right. Forgot I still had that lying around, heh.

@davidhesselbom
Contributor

Alright, everything seems to be working now, and you were right about This new above. I probably made that up unintentionally.

@fasterthanlime
Collaborator

Cool, good to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment