Invalid code crashes the compiler

Looked a bit into it and I have a few questions.

1) Should the following compile

[].foo(1;2;3)

2) What is a ClosureListExpression? Is it of use outside for loops? In what kind of code? For def x = (1;2;3), do multiple closures get generated?

The AST for the JIRA example takes the form def expectedFieldNames = [].for(ClosureListExpression)

, and then ACG runs into this BUG because for variable i, no variable is declared because it is not really treated as a for loop here but separate scope ClosureListExpression and ClosureExpression.

first.. a BUG! message must not happen. Then about the ClosureListExpression. I added that quite some time before I added the for loop. It is not an official feature, because it was mostly rejected by the people. I use it sometimes though. (1;2;3) generates a list of closures, but behind it there is a single closure and the list contains only closures with jump points. The structure behind was thought to be used for some kind of continuations, but not really thought through.

The variable is treated right in the variable scope stuff, but asm does not know about this well enough. And so it fails in a way it should not. The code whould have been rejected way before reaching that point or simply work.

Now... if the example would produce a message, that list has no "for" method... would that be too strange?

>> Now... if the example would produce a message, that list has no "for" method... would that be too strange?

To do that, we need to get rid of the compilation error first, right? Such a message makes sense only at runtime due to possible MOP injections, right?

I thought too that it was a grammar/AST issue but then ClosureListExpression processing makes it correct grammar/AST-wise, in a sense. So I wasn't sure which way to get rid of the issue.

In the light of GROOVY-4252 I suggest to let the compiler recognize the ClosureExpression as error in APP

You mean ClosureListExpression? (recognizing ClosureExpression as an error would be quite suicidal )

And you mean we get rid of that unofficial ClosureListExpression feature (at least block the entry to it and keep rest of the code behind for now)?

ahem.... ClosureListExpression of course, sorry
I suggest we don't change the grammar and let the compiler fail in APP if something returns a ClosureListExpression, which should not take one. The removal of the ClosureListExpression itself may cause problems, since I think the for-loop uses it internally - even if it does, in the end, not create a list of closures.

This still means that for the issue at hand, the compiler would then through an error for the ClosureListExpression usage after the for and not for the wrongly used ".". In fact the "." is not wrong, a method called "for" is very well allowed in Groovy. We need a good message for this, otherwise the user will be majorly confused.

What would be a good message here?

Just to understand better, you are suggesting that we block the use of ClosureListExpression everywhere but in the for loop, right? That java-like for loop would remain the only legal use and other places need to reject the code?

yes, I don't want to remove it completely, since I think we may have it at a later point maybe. So for now let's block it

unles someone here says that we maybe should instead enable this fully

Attaching a patch for review.

A ClosureListExpression anywhere except the for loop is rejected by APP now. Below is how the output looks like in various scenarios:

A)

a)  [].bar(1;2;3)

b)  def x = (1;2;3)

c)  [].for(1;2;3){println "in loop";break}

produces

Invalid use of the list of expressions here. 
It is supported only for traditional java-like for loops. 
A possible cause could be that for loop is invoked as a method by mistake. at line: 1 column: 8.
a)   [].bar(1;2;3)
            ^
  
b)   def x = (1;2;3)
              ^
c)   [].for(1;2;3){println "in loop";break}
            ^

B)

class Crasher {
    public void m() {
        def fields = [1,2,3]
        def expectedFieldNames = ["patentnumber", "status"].
        for (int i=0; i<fields.size(); i++) {
            Object f = fields[i] 
            System.out.println(f); 
        }
    }
}

produces

Invalid use of the list of expressions here. 
It is supported only for traditional java-like for loops. 
A possible cause could be that for loop is invoked as a method by mistake. at line: 5 column: 14.
           for (int i=0; i<fields.size(); i++) {
                ^

C) The following compiles successfully as the only permitted use of ClosureListExpression (under for loop).

for(int i = 0; i < 5; i++){println i}

An alternative message could be:

Invalid use of the expression list. Either the for loop has been invoked 
as a method by mistake, or the list of expressions is not supported in this context.

Hi. anyone got a chance to review the patch?

I've not looked at the patch itself, but the rationale and behaviour you explained look fine to me.

The patch itself is quite small and simple too. It's more of taking a call on the behavior after the patch and the friendliness of the error message.

Indeed, the patch was pretty simple, sorry for not having had a look at it before commenting.
The patch does look good to me too.
I'm wondering if someday we could have some use for this syntax though, but that's another story (and we can reenable things later if needed anyway)
Otherwise, the error message is okay to me.
The sole thing I'm wondering about it is: "list of expressions". [a, b, c] is also a list of expressions. So perhaps we should just say a "closure list of the form (a; b; c)" is not supported in this context?

But on the face of it, "(a;b;c)" and even "for(int i=0; i<1;i++)" don't really to seem to have anything to do with the closures, I think.

How about "expression list of the form (a; b; c) is not supported in this context"?

"expression list of the form (a; b; c) is not supported in this context" sounds good to me.

Done.