Things are happening in ternary expressions and x&&y||z even if the condition isn't fulfilled. (varargs and closures) #311

Closed
duckinator opened this Issue Sep 2, 2011 · 61 comments

Projects

None yet

4 participants

@duckinator
Collaborator

With #239 we settled on adding a warning to ternary expressions due to them running code when they shouldn't, this is a followup because it's a bit more serious than that:

vargarg: func (args: ...) -> Bool {
    false
}

f: func -> String {
    "f was called." println()
    "yay"
}

one: func {
    false ?  vargarg(f()) : true
}

two: func {
    false && vargarg(f()) || true
}

main: func {
    one()
    two()
}

generates:

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3 = { 
        lang_String__String_class(), 
        test__f()
    };
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? test__vargarg(____va4) : true;
}

void test__two() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args5 = { 
        lang_String__String_class(), 
        test__f()
    };
    lang_VarArgs__VarArgs ____va6 = (lang_VarArgs__VarArgs) { 
        &(____va_args5), 
        NULL, 
        1
    };
    false && test__vargarg(____va6) || true;
}

As well as this:

f: func -> Func -> Bool {
    "f was called." println()
    g
}

g: func -> Bool { true }

one: func {
    false ?  f()() : true
}

two: func {
    false && f()() || true
}

main: func {
    one()
    two()
}

->

void test2__one() {
    lang_types__Closure test2____comboRoot1 = test2__f();
    false ? ((lang_types__Bool (*)(void*)) test2____comboRoot1.thunk)(test2____comboRoot1.context) : true;
}

void test2__two() {
    lang_types__Closure test2____comboRoot2 = test2__f();
    false && ((lang_types__Bool (*)(void*)) test2____comboRoot2.thunk)(test2____comboRoot2.context) || true;
}

This is quite a big problem, and I don't know enough about rock's internals to fix it, BUT I will look into what C code needs to be generated for this to work.

@duckinator
Collaborator

So, I seem to have gotten the C code working, you can try the following to test my fix:

$ mkdir test; cd test
$ curl -L http://is.gd/XSlrGP > test.ooc
$ curl -L http://is.gd/B85AcU > gen_rebuild.sh
$ chmod +x gen_rebuild.sh
$ ./gen_rebuild.sh
[... trimmed out warnings and such ...]
$ ./test
# You should get "f was called." twice
$ mv rock_tmp/test.c rock_tmp/test.c.bak
$ curl -L http://is.gd/5t3LaB > rock_tmp/test.c
$ ./rebuild.sh
$ ./test

Original test.c: https://gist.github.com/1189494
My updated test.c: https://gist.github.com/1189448

If rock can somehow generate that, it should work.
NOTE: in the updated one, all definitions of ____wrap1() will need to be done one line before they are used
EDIT/NOTE2: In case it's not obvious, i'm not positive this works, hence the following poke.

@nddrylliog
Member

Yes, that is pretty bad. No time to look at it right now, but in rock it's simply a case of 'unwrap-and-addBeforeInScope'. Which should probably be unrolled to an if - but again! Not easy.

@duckinator
Collaborator

btw, I added this bug so we can close the 1.9.2-related one once fred adds the warning for ternary -- that's a good stopgap measure, imo, since &&|| is hardly ever used as I used it ;P

@nddrylliog
Member

0.9.2* ! We're not Ruby.

@duckinator
Collaborator

that was an actual typo, not a brainfart
numpad ft(w|l)? :P

@shamanas
Collaborator
shamanas commented Sep 3, 2011

@duckinator: so basically, your opinion is that we should wrap the expressions in x&&y||z into functions (btw local functions are not standard and do not work with plenty of compilers) ? How should it be decided wich to wrap and wich not? Just wrap vararg functions ? :/

@duckinator
Collaborator

That's just the only way I could get them to work :( It's kinda shitty, but if you have an alternative that works feel free to mention it ;P

@shamanas
Collaborator
shamanas commented Sep 3, 2011

hm no not really :/ i was just asking to start thinking about a fix

@fredreichbier
Member

I'm working on the warning workaround. Will do the rest of it tonight. :)

@fredreichbier fredreichbier was assigned Sep 3, 2011
@duckinator
Collaborator

@shamanas since @fredreichbier is doing the warning and closing #239, we have a bit of time to work on this particular bug.

The embedded functions are a potential method, but it brings portability issues. But at the same time, I feel that unwrapping x&&y||z into an if/else is...well...about equivalent to molesting code...

EDIT: and it's also prone to exploding in your face and causing kittens to die

@shamanas
Collaborator
shamanas commented Sep 3, 2011

There must be some other way :/

@duckinator
Collaborator

I really really really really really really hope there is. There probably is, but, like I said: we have plenty of time to work on this, fred is putting in a stopgap measure just so people know to avoid it in the meantime :)

@nddrylliog
Member

There is no need for embedded functions, you can just add an ooc ACS (ie.
closure)

@duckinator
Collaborator

ooc closures have the same exact bug

@shamanas
Collaborator
shamanas commented Sep 3, 2011

We are discussing this on IRC with duck, we came to the same conclusion, although there MAY be a way that this will work with no additional closure

@duckinator
Collaborator

Heyyy, fuckin' awesome!

original:

void test__three() {
    lang_types__Closure test____comboRoot1 = test__f2();
    false ? ((lang_types__Bool (*)(void*)) test____comboRoot1.thunk)(test____comboRoot1.context) : true;
}

new:

void test__three() {
    lang_types__Closure test____comboRoot1;
    false ? ((lang_types__Bool (*)(void*))( test____comboRoot1 = test__f2()).thunk)(test____comboRoot1.context) : true;
}

and it works :)
99.99% sure that's valid c99 and possibly even earlier.

So I have it working for closures. :)

EDIT: it works for closures + the &&|| thing, as well. So now just for varargs!

@shamanas
Collaborator
shamanas commented Sep 3, 2011

It is c1 compatible dont worry :)
Brainstorming on IRC is good for issue solving :D

@duckinator
Collaborator

https://gist.github.com/1191631 is what i have atm, but i have to go. bbl o/

@shamanas
Collaborator
shamanas commented Sep 3, 2011

Comments -> before
Uncommented -> hopefully after

/*
lang_types__Closure test2____comboRoot1 = c__f();
false ? ((lang_types__Bool (*)(void*)) __comboRoot1.thunk)(__comboRoot1.context) : true;
*/

lang_types__Closure test2____comboRoot1;
false ? ((lang_types__Bool (*)(void*))( __comboRoot1 = c__f()).thunk)(__comboRoot1.context) : true;

    /*struct {
lang_types__Pointer __f1;
lang_String__String* __f2;
} ____va_args6 = {
lang_String__String_class(),
test__f()
};
lang_VarArgs__VarArgs ____va7 = (lang_VarArgs__VarArgs) {
&(____va_args6),
NULL,
1
};
false ? test__vargarg(____va7) : true;*/
    false ? test__vargarg((lang_VarArgs__VarArgs) {
                            &((struct {
                                lang_types__Pointer __f1;
                                lang_String__String* __f2;
                            }) {
                                lang_String__String_class(),
                                test__f()
                            }), NULL, 1}) : true;
@shamanas
Collaborator
shamanas commented Sep 3, 2011

I have no idea how to implement these changes atm, I am confused, too much ssue-solving/brainstorming today. Ill have some more time tomorrow, will focus on this :)

@nddrylliog
Member

Tricky :)

However, I would prefer a version using the ',' C operator, which guarantees execution order. It would be a bit longer but cleaner. Don't you think?

Something like (assign, use)

(Note that in C, a declaration is not a statement, but an assignment is)

I wouldn't want -O2/-O3 to bite us :)

@duckinator
Collaborator

ah! yea that should be fairly simple if you're thinking what i'm thinking.
you mean something like this? (untested):

lang_types__Closure test2____comboRoot1;
false ? ( __comboRoot1 = c__f(), ((lang_types__Bool (*)(void*))(__comboRoot1).thunk)(__comboRoot1.context)) : true;
@nddrylliog
Member

Yes, precisely!

@duckinator
Collaborator

Awesome! I'll test it in a bit.
Any similar ideas for vararg stuff?

@nddrylliog
Member

Same: you could predeclare the struct and only assign in the parenthesis,
using a struct literal

@fredreichbier fredreichbier added a commit to fredreichbier/rock that referenced this issue Sep 3, 2011
@fredreichbier fredreichbier Stage 1 for a workaround for #239/#311: A warning for varargs-inside-…
…ternary-expressions-or-boolean-binaryops.
6347c02
@duckinator
Collaborator

Sounds reasonable, I'll look into it later! \o/

@duckinator
Collaborator

...and by later, I mean tomorrow. Someone @ me so I have an email reminding me to try it :P

@shamanas
Collaborator
shamanas commented Sep 4, 2011
lang_types__Closure test2____comboRoot1;
false ? ( __comboRoot1 = c__f(), ((lang_types__Bool (*)(void*))(__comboRoot1).thunk)(__comboRoot1.context)) : true;

Is that really valid? :O
So (a = c, b) == b ?
Edit: Nevermind, I looked it up :)

@duckinator
Collaborator

It should be! But I'm going to verify tomorrow.
Now, it's 2:20am
OFF TO BED FOR ME

@shamanas
Collaborator
shamanas commented Sep 4, 2011

Good night :)

@nddrylliog
Member

@duckinator reminder!

@duckinator
Collaborator

Hi @nddrylliog & @shamanas, I got them all defined before use, now o/

Old:

void test__two() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args8 = { 
        lang_String__String_class(), 
        test__f()
    };
    lang_VarArgs__VarArgs ____va9 = (lang_VarArgs__VarArgs) { 
        &(____va_args8), 
        NULL, 
        1
    };
    false && test__vargarg(____va9) || true;
}

void test__three() {
    lang_types__Closure test____comboRoot1 = test__f2();
    false ? ((lang_types__Bool (*)(void*)) test____comboRoot1.thunk)(test____comboRoot1.context) : true;
}

New:

void test__two() {
    struct ____va_args8_s {
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    };
    false && test__vargarg((lang_VarArgs__VarArgs) {
                            &((struct ____va_args8_s) {
                                lang_String__String_class(),
                                test__f()
                            }), NULL, 1}) || true;
}

void test__three() {
    lang_types__Closure test____comboRoot1;
    false ? (test____comboRoot1 = test__f2(), (lang_types__Bool (*)(void*))(test____comboRoot1).thunk)(test____comboRoot1.context) : true;
}
@shamanas
Collaborator
shamanas commented Sep 4, 2011

Ok we got the optimal code to generate figured out but we gotta IMPLEMENT this :P Tried to make something happen this morning, turns out it is pretty complicated xD

@shamanas
Collaborator
shamanas commented Sep 6, 2011

Started working on this, it looks pretty promising so far, but I have no idea how I will make rock generate the comma expression from FunctionCall.ooc. Here is what i get for now:

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? test__vargarg(____va4) : true;
}

My goal is:

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? (____va_args3 = {
                                lang_String__String_class(),
                                test__f()
                            } ,test__vargarg(____va4)) : true;
}
@shamanas
Collaborator
shamanas commented Sep 6, 2011

I managed to make rock generate

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? (____va_args3 = (struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ){ 
        lang_String__String_class(), 
        test__f()
    }, test__vargarg(____va4)) : true;
}

Seems valid to me but gcc nags me :(

@nddrylliog
Member

hum what is the whole second ____va_args3 = (struct { .... } ) part doing here?

@shamanas
Collaborator
shamanas commented Sep 6, 2011

Well this is the point of the code, not to initialize ____va_args3 outside the ternary. The struct {...} part is the structure type then the { ... } the elements initialisatio. This should work right ? (anonymous structure)

@nddrylliog
Member

Yes, but why are you repeating the type? Couldn't you just do:

false ? (____va_args3 = {
lang_String__String_class(),
test__f()
}, test__vargarg(____va4)) : true;

? And what's the GCC error?

@shamanas
Collaborator
shamanas commented Sep 6, 2011

I tried this too, it didnt work. The error (for current code) is error: incompatible types when assigning to type ‘struct ’ from type ‘struct ’

@nddrylliog
Member

I guess you'll have to typedef it somehow then?

@shamanas
Collaborator
shamanas commented Sep 6, 2011

With

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String\* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? (____va_args3 = { 
        lang_String__String_class(), 
        test__f()
    }, test__vargarg(____va4)) : true;
}
error is expected expression before ‘{’ token
@shamanas
Collaborator
shamanas commented Sep 6, 2011

@nddrylliog this might work, I will look it up a bit first ;-)

@shamanas
Collaborator
shamanas commented Sep 6, 2011

Instead of typedef'ing, I guess I will just assign each field of the annymous struct :D

@nddrylliog
Member

yeah that would work too

@shamanas
Collaborator
shamanas commented Sep 6, 2011

Ok this works perfectly for varargs now :) Here is code generated

void test__one() {
    struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? (____va_args3.__f1 = lang_String__String_class(), ____va_args3.__f2 = test__f(), test__vargarg(____va4)) : true;
}

Will commit now :D

@duckinator
Collaborator

Since you fixed vararg stuff, here's the C code related to closures:

Old:

void test__three() {
    lang_types__Closure test____comboRoot1 = test__f2();
    false ? ((lang_types__Bool (*)(void*)) test____comboRoot1.thunk)(test____comboRoot1.context) : true;
}

New:

void test__three() {
    lang_types__Closure test____comboRoot1;
    false ? (test____comboRoot1 = test__f2(), (lang_types__Bool (*)(void*))(test____comboRoot1).thunk)(test____comboRoot1.context) : true;
}
@shamanas
Collaborator
shamanas commented Sep 7, 2011

I have nearly fixed this, just an issue with the type of the VariableDecl in AstBuilder onFunctionCallCombo remaining :/

@shamanas
Collaborator
shamanas commented Sep 7, 2011

Ok basic example is working but I have to test more stuff (more than two function in the combo call etc.)

@shamanas
Collaborator
shamanas commented Sep 7, 2011

Ok this is still quite fragile. For example,

f: func(args: ...) -> Func -> Func -> Bool {
    "f was called." println()
    g
}

g: func -> Func -> Bool {
    "g was called." println()
    h
}

h: func -> Bool {
    "h was called." println()
    true
}

j: func -> Bool {
    "j was called." println()
    true
}

main: func {
    false ? f(j())()() : true
}

Shows: j was called. My guess is that the inBinOrTern "detection" is not quite working. I will fix that later.
Also,

f: func -> Func -> Func -> Bool {
    "f was called." println()
    g
}

g: func(args: ...) -> Func -> Bool {
    "g was called." println()
    h
}

h: func -> Bool {
    "h was called." println()
    true
}

j: func -> Bool {
    "j was called." println()
    true
}

main: func {
    false ? f()(j())() : true
}

Makes rock say: ERROR No such function __comboRoot1(Bool), so the type of the comboRoots is not determined correctly. Also something on my todo list.

@nddrylliog
Member

@shamanas so I cherry-picked your two commits fad727a and 755b4b1 but compilation is failing with

source/rock/backend/cnaughty/FunctionCallWriter.ooc:20:18 ERROR Undefined symbol 'fCall inBinOrTern'
        if(fCall inBinOrTern) {
                 ~~~~~~~~~~~   

Can you make a proper pull request for just this? Or something?

@shamanas
Collaborator

https://github.com/shamanas/rock/commits/master
Use all commits from fad727a I had stupidly forgot to add some variable declarations in the first commit and the code changes a lot after that anyway.

@duckinator
Collaborator

@shamanas what's the status on this?

If you can't figure out how to clean up your commits, can you just redo it or say here what you had to do to fix it so someone else can give it a shot?

@shamanas
Collaborator

Well many simple cases of this bug have been solved. I can not figure out how to fix the more complicated cases I posted. I also didn't have any time to work on rock this week, maybe I will do something tomorrow or the day after that.

@duckinator
Collaborator

@shamanas it's fine, was just curious :)

@shamanas
Collaborator

By the way, I think that actually putting the comma expr in the vararg function call would fix half the problems. This is also more general and will let me remove a bit of specific, ugly code. I'm thinking about something like

struct { 
        lang_types__Pointer __f1;
        lang_String__String* __f2;
    } ____va_args3;
    lang_VarArgs__VarArgs ____va4 = (lang_VarArgs__VarArgs) { 
        &(____va_args3), 
        NULL, 
        1
    };
    false ? test__vargarg((____va_args3.__f1 = lang_String__String_class(), ____va_args3.__f2 = test__f(), ____va4)) : true;

Note: This type of code would be generated on any vararg function call, not only in a ternary expression.

@nddrylliog nddrylliog closed this Dec 14, 2011
@nddrylliog nddrylliog reopened this Dec 14, 2011
@nddrylliog
Member

@shamanas That looks like a good idea. I think this is the kind of slight refactorings we should at the same time introduce in rock and add to oc.

Like I see it, oc is "a fat-free incarnation of ooc, but with all the characteristics that made us love it / believe in it in the first place". For example, C header parsing for oc is a must-have. Libcaching done right (with precompiling of separate libraries) should be a goal from the beginning. Using .use files as both pkgconfig-like and Makefiles is essential. We've learned an awful lot with rock, and I think the rock codebase is beyond repair although we'll keep making incremental fixes to it because it's what we use in production.

Here's how I see the transition: over time, we'll slowly flesh out oc with the good ideas we see in rock (because there are good ideas in rock, believe it or not!), and eventually settle on a subset of ooc that makes sense (possibly including some features that are not in rock yet such as C header parsing, ), and we'll (ofmlabs/whatever I end running in a few months) migrate our important codebases+libraries to that safe subset. So there'll be a time where most of the ooc code out there should be compilable by both rock and oc. And then we'll work on making rock definitively obsolete, ie polishing oc to the point where there's really no reason to bother with that legacy pile of code.

I wish I had more time to put into it personally: all along my on/off relationship with ooc, I have learned tons of stuff elsewhere, other languages, other platforms and other ways of thinking, so I have a slightly different perspective on things now, and yet I am still convinced ooc is not totally useless as a concept (even if its current implementation has some serious drawbacks, annoying bugs, and bloat due to the rapid initial growth).

It's true that it has been seemingly stalling for some time, but the fact some people keep patching it and trying to use it (cf. all of @shamanas's fixes, @nilium's war on the GC, @fredreichbier's new features, @duckinator bug management, etc. etc.), I think it has a purpose, it just needs some love.

So, @shamanas I see you've been working hard on rock and now have a good grasp of what's going on inside the beast, how would you like to work on oc a bit? Since I can't afford to spend much time on it (but can provide guidance) it's a chance for you to makes mistakes^W^W^Wmake a compiler your way, more or less :) There are some solid basics, it needs some love is all.

Well, let me know what you think.

@duckinator
Collaborator

@nddrylliog I like everything you said there, but I'd like to actively deprecate the parts of ooc we decide against in rock, and remove them a few months later. The fewer incompatible compilers we leave in our wake, the better _looks at j/ooc_. :)

EDIT: Of course oc would grow afterwards, but when we finally drop rock, oc should be basically rock - crap + features, not rock - crap - features + other_features (oc's features being a superset of rock's features instead of a huge mess, basically)

@shamanas
Collaborator

@nddrylliog I had thought of developping on oc before but you asking me to is a whole different thing. I would love to at least atempt to make something happen :D I always wanted to really get into compiler developement and I guess this is my chance of doing it. (Although basically the frontend is done and the general structure of the AST is laid out, right?)

@duckinator
Collaborator

Hey, digging up ancient bugs again. :D

What's the status on this?

@shamanas
Collaborator
shamanas commented Jun 2, 2012

@duckinator Still the same as months ago, however this should be fixed / worked around with libcee, at it is a code generation problem.

@shamanas shamanas was assigned Nov 21, 2012
@nddrylliog nddrylliog referenced this issue Nov 27, 2012
Merged

Closes #311 #485

@shamanas shamanas closed this in 3d8aa6a Nov 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment