comprehensions beginning with if clause

[CeylonMigrationBot]

[@gavinking] Currently, a comprehension must begin with for. For #3913 we should relax this restriction, and allow a comprehension to begin with if. Then, for example:

{ if (exists x) x }

Evaluates to {x} if x exists, or to {} otherwise.

Likewise:

 [ if (!string.empty) string ]

Evaluates to [string] if string is not empty, or to [] otherwise.

[Migrated from ceylon/ceylon-spec#869]
[Closed at 2014-01-28 11:57:01]

[CeylonMigrationBot]

[@lucaswerkmeister] Should we also allow expressionComprehensionClauses at the start? That would mean that

{String+} asSequence = { "element" };

would be a comprehension instead of an enumeration with a one-long sequencedArgument.

[CeylonMigrationBot]

[@lucaswerkmeister]

Should we also allow expressionComprehensionClauses at the start?

God no. { element } is something completely different than { } and { element1, element2 }? Hell naw.

I’ll try to implement this; let’s see how far I get. I think I’ve got the grammar part, now I have to fix the Visitors.

[CeylonMigrationBot]

[@lucaswerkmeister] Okay, I have something. I get 6 failures and 3 errors in the ceylon-compiler tests, but I also get these in a clean working tree – is my setup messed up or is this correct?

Also, I’m suspicious. That was way too easy. Let’s try if it works...

[CeylonMigrationBot]

[@lucaswerkmeister] Haha, yeah. It was too easy. For some reason I thought that if it compiles, then the rest happens automagically...

I see two ways of making comprehensions with leading if work (EDIT: for the Java platform. haven’t looked at JS yet). First of all, here’s a brief overview on how comprehensions currently work as far as I understand it, in case you want a refresher.

• the regular way would be to generate $next$0() for initial if clauses (and then $iterator$1() for the following for clause, which is currently just set in the "constructor"), and teach $next$X() and $iterator$Y() that there’s not always a previous iterator.
• the hacky way would be to put the initial if clause(s) outside the anonymous AbstractIterable, and just return empty if the conditions don’t apply.

The main difference between these is that in the hacky way, the conditions are evaluated when the Iterable is created, while in the regular way they are evaluated when the Iterable is first evaluated. That’s a big difference if the condition has side effects, and I think that the semantics of the regular way make more sense. WDYT?

Also, in case someone else wants to work for this, here are my grammar changes, and I’m pretty confident that these are sufficient.

[CeylonMigrationBot]

[@gavinking] However you implemented, the condition needs to be evaluated lazily, each time the comprehension is evaluated. (Comprehensions can be reevaluated.)

[CeylonMigrationBot]

[@lucaswerkmeister] Progress report: I think I have implemented all necessary changes in the ExpressionTransformer (I went with the regular way), and I’m not sure where the ExpressionVisitor needs more work. Unfortunately, I currently get an NPE about 75 stackframes down a horrifying com.sun.javac... rabbit hole when compiling { if (exists x) x }, and I have no idea how to debug that. I need some sleep first.

If anyone’s interested or wants to try and fix the error, here’s my current changes: https://gist.github.com/lucaswerkmeister/8466797 (copy, then xclip -o | git apply)

[CeylonMigrationBot]

[@FroMage] Look at the generated .src, in theory you see it displayed before it crashes.

[CeylonMigrationBot]

[@lucaswerkmeister] I see it, looks as intended, compiles fine when I put it in its own file (and fix the .type.names).

----- Ursprüngliche Nachricht -----
Von: "Stéphane Épardaud" notifications@github.com
Gesendet: ‎17.‎01.‎2014 09:42
An: "ceylon/ceylon-spec" ceylon-spec@noreply.github.com
Cc: "Lucas Werkmeister" mail@lucaswerkmeister.de
Betreff: Re: [ceylon-spec] comprehensions beginning with if clause (#3975)

Look at the generated .src, in theory you see it displayed before it crashes.
—
Reply to this email directly or view it on GitHub.

[CeylonMigrationBot]

[@FroMage] Show me the .src and stack trace?

[CeylonMigrationBot]

[@lucaswerkmeister] When I'm home, ca. 1.5h

----- Ursprüngliche Nachricht -----
Von: "Stéphane Épardaud" notifications@github.com
Gesendet: ‎17.‎01.‎2014 10:19
An: "ceylon/ceylon-spec" ceylon-spec@noreply.github.com
Cc: "Lucas Werkmeister" mail@lucaswerkmeister.de
Betreff: Re: [ceylon-spec] comprehensions beginning with if clause (#3975)

Show me the .src and stack trace?
—
Reply to this email directly or view it on GitHub.

[CeylonMigrationBot]

[@lucaswerkmeister] Okay. Took a little longer that expected, because I fixed some other bugs first. This isn’t the same exception as yesterday night, but it’s close enough, I think.

• for if
• if for
• the changes to ceylon-spec, ceylon-compiler and ceylon-js, in case you want to test it yourself

[CeylonMigrationBot]

[@FroMage] IMO it's the anonymous constructor block that is the issue. Looks like it was not declared properly in the AST. Try removing it.

[CeylonMigrationBot]

[@FroMage] No that's crazy, this is already used now. Looks like the class symbol has a package kind. This is just crazy.

[CeylonMigrationBot]

[@lucaswerkmeister] I agree it’s crazy, because I get a different exception when I’m debugging from Eclipse than when I’m running it from the command line...

But you’re actually right; at least when I’m debugging from Eclipse, javac gets an NPE because the stats (statements) of the anonymous contructor are empty (null element).

I’ll fix that (initialize boolean $next$0$exhausted$ to false), and then see what happens.

[CeylonMigrationBot]

[@lucaswerkmeister] correction: not empty, the ExpressionTransformer has an initIterator statement, and if transformForClause doesn’t set that, it actually adds a null statement to the constructor.

[CeylonMigrationBot]

[@lucaswerkmeister] And it compiles!! AssertionError in the test case of course. Just a few more seconds...

[CeylonMigrationBot]

[@lucaswerkmeister] Dammit! It works from the IDE and crashes with still the same error (kind 31 etc.) on the command line.

[CeylonMigrationBot]

[@lucaswerkmeister] Kind 31 is the ERR kind, by the way. The only place where it’s set that I could find is in Type.java, line 1309 – the ErrorType. So javac recognizes some error somewhere, but decides to not tell me about it, crashing later instead, when any trace of the error is long wiped from the stacktrace. Wonderful.

I’m visiting my parents over the weekend, so unfortunately I won’t be able to work on this until sunday evening. If you want to try things out, you can see my two commits referenced above; that’s all I have.

[CeylonMigrationBot]

[@lucaswerkmeister] Actually... I was just able to compile and run the following code on my netbook:

void run() {
    {String*} strings = { if ("hi".contains("h")) "Hello, World!" };
    print(strings.first);
}

I’ll have to check why. Weird architecture bug (32/64-bit) or not caused by me and fixed in the meantime?

(Interim report: typechecker and java backend apparently done, javascript backend todo.)

[CeylonMigrationBot]

[@gavinking] Sounds good!

[CeylonMigrationBot]

[@lucaswerkmeister] Alright, I think I have some implementation for the Javascript backend. However, I currently get:

TypeError: Property 'next' of object ceylon.language::ComprehensionIterator@1 is not a function

Now that I’ve committed my changes, I can go back to master and check if that’s really my error. (Probably, but you never know!)

Also, I got the error (kind 31) on the Java backend again, but then it suddenly vanished. Maybe ceylon/ceylon-compiler@024c4b8 fixed it?

(Interim report: typechecker and java backend apparently done, javascript backend needs more testing and bugfixing.)

[CeylonMigrationBot]

[@lucaswerkmeister] Alright, I found and fixed the remaining known bug in the JS backend (spoiler: it was a really stupid one). I’ll write some more tests now, and then I think it’s time for a pull request!

[CeylonMigrationBot]

[@lucaswerkmeister] Phew! Pull requests opened. (Example code over at #4014.)

[CeylonMigrationBot]

[@gavinking] Awesome! Thanks!

[CeylonMigrationBot]

[@FroMage] Thanks, we'll take a look ASAP, and merge it. FTR we're going to try to move to a single repo real soon so you don't have to go through that pain again for pull requests.

[CeylonMigrationBot]

[@lucaswerkmeister] Cool, sounds good!

FTR, I already started rebasing the branches locally to resolve conflicts ASAP (since this used to be scheduled for 1.2). No conflicts so far though.

[CeylonMigrationBot]

[@gavinking]
@FroMage

Thanks, we'll take a look ASAP, and merge it

ETA? I have reviewed the typechecker changes, which look good, and am ready to push them.

[CeylonMigrationBot]

[@gavinking] I have pushed the typechecker changes. Changes to ceylon-compiler and ceylon-js still need to be pushed.

[CeylonMigrationBot]

[@gavinking] All done. Thanks, @lucaswerkmeister, great work!

[CeylonMigrationBot]

[@FroMage] I have reviewed the jvm compiler changes and they seem very good, congrats! I have added some tests though, as I felt there were some cases missing.

Do you mind adding runtime tests for this in the language module? We've had lots of bugs in comprehensions at runtime in the past and we absolutely need to test this, we can't rely just on how the compilation looks ;)

Thanks!

[CeylonMigrationBot]

[@lucaswerkmeister] @chochos already bugged me about runtime tests :) he suggested a few, I added these and a few more in ceylon/ceylon.language@f8ef15c, ceylon/ceylon.language@4864b49.

[CeylonMigrationBot]

[@FroMage] OK if you added them already then sorry, it's great :)