comprehensions beginning with if clause #869

gavinking · 2013-12-04T14:04:44Z

Currently, a comprehension must begin with for. For #807 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.

The text was updated successfully, but these errors were encountered:

lucaswerkmeister · 2014-01-15T17:48:05Z

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.

lucaswerkmeister · 2014-01-15T19:26:07Z

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.

lucaswerkmeister · 2014-01-15T21:08:18Z

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

lucaswerkmeister · 2014-01-15T23:24:47Z

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.

gavinking · 2014-01-16T01:25:08Z

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

lucaswerkmeister · 2014-01-17T01:30:11Z

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)

FroMage · 2014-01-17T08:41:56Z

Look at the generated .src, in theory you see it displayed before it crashes.

lucaswerkmeister · 2014-01-17T09:17:46Z

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 (#869)

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

FroMage · 2014-01-17T09:19:15Z

Show me the .src and stack trace?

lucaswerkmeister · 2014-01-17T09:27:44Z

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 (#869)

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

lucaswerkmeister · 2014-01-17T12:58:48Z

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

FroMage · 2014-01-17T13:06:35Z

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

FroMage · 2014-01-17T13:34:56Z

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

lucaswerkmeister · 2014-01-17T14:07:12Z

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.

lucaswerkmeister · 2014-01-17T14:09:13Z

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.

lucaswerkmeister · 2014-01-17T14:11:58Z

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

Introduces the new abstract class InitialComprehensionClause as superclass of IfComprehensionClause and ForComprehensionClause to the tree.

…uses

lucaswerkmeister · 2014-01-17T15:21:56Z

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

lucaswerkmeister · 2014-01-17T15:42:55Z

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.

lucaswerkmeister · 2014-01-18T21:28:21Z

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

gavinking · 2014-01-18T22:03:27Z

Sounds good!

This also significantly simplifies the $next$0() method – see the test case. CC ceylon/ceylon-spec#869

…uses

lucaswerkmeister · 2014-01-19T23:54:29Z

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

* `(if)+ expression` did not have a "tail", i. e., did not return if the condition was false * `(if)+ (for) (if|for)* expression` wrote the "tail" before closing the "if"’s braces: a) double return inside the if, b) missing return outside the if part of ceylon/ceylon-spec#869

(that is, comprehensions without "for" clauses.) (Part of ceylon/ceylon-spec#869.)

lucaswerkmeister · 2014-01-20T19:05:58Z

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!

(that is, comprehensions without a "for" clause) The comprehension would return the result infinitely instead of only once. Still part of ceylon/ceylon-spec#869.

lucaswerkmeister · 2014-01-20T22:14:43Z

Phew! Pull requests opened. (Example code over at #908.)

gavinking · 2014-01-20T23:42:40Z

Awesome! Thanks!

@chochos

Suggested by @chochos [1]. I added another test to verify that conditions after a false condition are not evaluated: if (false) if (functionWithSideEffects()) value should not have side effects. [1] f8ef15c#commitcomment-5126747

FroMage · 2014-01-27T13:12:19Z

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.

lucaswerkmeister · 2014-01-27T13:29:07Z

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.

gavinking · 2014-01-27T23:05:50Z

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

Introduces the new abstract class InitialComprehensionClause as superclass of IfComprehensionClause and ForComprehensionClause to the tree.

gavinking · 2014-01-28T01:46:12Z

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

…uses

This also significantly simplifies the $next$0() method – see the test case. CC ceylon/ceylon-spec#869

…uses

* `(if)+ expression` did not have a "tail", i. e., did not return if the condition was false * `(if)+ (for) (if|for)* expression` wrote the "tail" before closing the "if"’s braces: a) double return inside the if, b) missing return outside the if part of ceylon/ceylon-spec#869

(that is, comprehensions without "for" clauses.) (Part of ceylon/ceylon-spec#869.)

(that is, comprehensions without a "for" clause) The comprehension would return the result infinitely instead of only once. Still part of ceylon/ceylon-spec#869.

Add tests for ceylon/ceylon-spec#869

gavinking · 2014-01-28T11:57:01Z

All done. Thanks, @lucaswerkmeister, great work!

FroMage · 2014-01-29T16:38:05Z

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!

lucaswerkmeister · 2014-01-29T16:44:30Z

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

FroMage · 2014-01-29T16:51:59Z

OK if you added them already then sorry, it's great :)

This was referenced Dec 4, 2013

iteration and branching in named argument lists #807

Closed

return more than one value from a for-comprehension #860

Open

lucaswerkmeister mentioned this issue Jan 16, 2014

anon functions in comprehension ceylon/ceylon-compiler#986

Closed

lucaswerkmeister added a commit to lucaswerkmeister/ceylon-compiler that referenced this issue Jan 17, 2014

Implement ceylon/ceylon-spec#869: comprehensions starting with if cla…

daca186

…uses

lucaswerkmeister added a commit to lucaswerkmeister/ceylon-compiler that referenced this issue Jan 19, 2014

Fix comprehensions starting with multiple "if" clauses

024c4b8

This also significantly simplifies the $next$0() method – see the test case. CC ceylon/ceylon-spec#869

lucaswerkmeister added a commit to lucaswerkmeister/ceylon-js that referenced this issue Jan 19, 2014

Implement ceylon/ceylon-spec#869: comprehensions starting with if cla…

8b8fd58

…uses

lucaswerkmeister added a commit to lucaswerkmeister/ceylon-js that referenced this issue Jan 20, 2014

Fix stupid bug in "if+ expr" comprehensions

bb6c802

(that is, comprehensions without "for" clauses.) (Part of ceylon/ceylon-spec#869.)

lucaswerkmeister added a commit to lucaswerkmeister/ceylon.language that referenced this issue Jan 20, 2014

Add tests for ceylon/ceylon-spec#869

f8ef15c

lucaswerkmeister mentioned this issue Jan 20, 2014

#869: allow comprehensions to start with an if clause #908

Closed

ghost assigned gavinking Jan 27, 2014

gavinking pushed a commit that referenced this issue Jan 28, 2014

#869: allow comprehensions to start with an if clause

4bc4491

Introduces the new abstract class InitialComprehensionClause as superclass of IfComprehensionClause and ForComprehensionClause to the tree.

gavinking pushed a commit to ceylon/ceylon-compiler that referenced this issue Jan 28, 2014

Implement ceylon/ceylon-spec#869: comprehensions starting with if cla…

e030577

…uses

gavinking pushed a commit to ceylon/ceylon-compiler that referenced this issue Jan 28, 2014

Fix comprehensions starting with multiple "if" clauses

1dd7eab

This also significantly simplifies the $next$0() method – see the test case. CC ceylon/ceylon-spec#869

gavinking pushed a commit to ceylon/ceylon-js that referenced this issue Jan 28, 2014

Implement ceylon/ceylon-spec#869: comprehensions starting with if cla…

c2934fa

…uses

gavinking pushed a commit to ceylon/ceylon-js that referenced this issue Jan 28, 2014

Fix stupid bug in "if+ expr" comprehensions

3d386d2

(that is, comprehensions without "for" clauses.) (Part of ceylon/ceylon-spec#869.)

gavinking added a commit to ceylon/ceylon.language that referenced this issue Jan 28, 2014

Merge pull request #397 from lucaswerkmeister/test-869

b8516f2

Add tests for ceylon/ceylon-spec#869

gavinking closed this as completed Jan 28, 2014

lucaswerkmeister referenced this issue in ceylon/ceylon.language Jun 3, 2014

improve lazy & eager comprehension tests

3e6cbb7

CeylonMigrationBot mentioned this issue Nov 14, 2015

comprehensions beginning with if clause eclipse-archived/ceylon#3975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

comprehensions beginning with if clause #869

comprehensions beginning with if clause #869

gavinking commented Dec 4, 2013

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

gavinking commented Jan 16, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 18, 2014

gavinking commented Jan 18, 2014

lucaswerkmeister commented Jan 19, 2014

lucaswerkmeister commented Jan 20, 2014

lucaswerkmeister commented Jan 20, 2014

gavinking commented Jan 20, 2014

FroMage commented Jan 27, 2014

lucaswerkmeister commented Jan 27, 2014

gavinking commented Jan 27, 2014

gavinking commented Jan 28, 2014

gavinking commented Jan 28, 2014

FroMage commented Jan 29, 2014

lucaswerkmeister commented Jan 29, 2014

FroMage commented Jan 29, 2014

comprehensions beginning with if clause #869

comprehensions beginning with if clause #869

Comments

gavinking commented Dec 4, 2013

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

lucaswerkmeister commented Jan 15, 2014

gavinking commented Jan 16, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

FroMage commented Jan 17, 2014

FroMage commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 17, 2014

lucaswerkmeister commented Jan 18, 2014

gavinking commented Jan 18, 2014

lucaswerkmeister commented Jan 19, 2014

lucaswerkmeister commented Jan 20, 2014

lucaswerkmeister commented Jan 20, 2014

gavinking commented Jan 20, 2014

FroMage commented Jan 27, 2014

lucaswerkmeister commented Jan 27, 2014

gavinking commented Jan 27, 2014

gavinking commented Jan 28, 2014

gavinking commented Jan 28, 2014

FroMage commented Jan 29, 2014

lucaswerkmeister commented Jan 29, 2014

FroMage commented Jan 29, 2014