Parser accepts invalid List literal

Although the following compiles, output is very incorrect:

class A {
  static x = [
           [1]
           [2]
  ]
}

assert A.x == [null]

Sorry, didn't understand my example earlier. The output is correct. It is doing [1][2], where first list is the list of items and 2nd list is the range of indexes for extracting a sublist and because [1] has no item with index 2, it is returning null.

Hmm, is this really invalid code?

Look at this:

class A {
 static x = [
          [1, 3, 4]
          [2]
 ]
}

assert A.x == [4]

Actually, x is a list of one element: it's a list consisting of one element being the third (index 2) element of the list [1, 3, 4], hence 4.
This is in reality: [1, 3, 4][2], but spanning two lines, which is a valid list / subscript access notation.

So should this really been fixed?

The code in my example here is not being fixed. I had posted my 2nd comment about my misunderstanding of the code I put in my first comment, which is the same thing that you have told now.

What is being fixed is the issue reported in original JIRA. That is not valid. Do you think that also is?

[1, 2, 3][X:Y] is not valid whether spanning 1 line or 2 lines, I think.

If the list applied on [1, 2, 3] to get a sublist has any map-entry expressions, it is not valid, right?

it could be valid... a getAt with a Map may make sense. But that would mean it has to be implemented since the BUG! message points out, that the compiler gets an unexpected exception. I mean we support for example foo["bar"] I think, so why not with a map?

And what will be the meaning of [1,2,3]["foo1": "bar1", "foo2": "bar2"]?

Roshan, yes, it's not yielding any meaninful thing... but it doesn't mean it's really invalid, syntax-wise / grammar-wise.
And Jochen is right, someone could give a meaning to that subscript taking a map as argument, as part of writing some DSL of some sort (sorry, I don't have a good example in mind).
So in the end, I'm not sure we should "fix" this.

In any case, the change that I am introducing is not something new. Prior to this JIRA fix, compiler was already behaving like this if the list in question was static, i.e., if the code was

class InvalidListLiteral {
  static x = [
    [foo: 1, bar: 2]
    [foo: 1, bar: 2]
  ]
}

So, if we forget about current fix, then compiler was inconsistent in handling "static x = [[foo: 1, bar: 2][foo: 1, bar: 2]]" and "def x = [[foo: 1, bar: 2][foo: 1, bar: 2]]". While the first instance got rejected as a compilation error, 2nd was rejected as GroovyBugError. So, if the handling of 1st case was already in as "accepted" behavior, then all I have done is made the compiler more consistent in doing that.

With 1.7-beta-1,

class A {
  static x = [[1][foo:2]]
}

resulted in

1 compilation error:

You tried to use a map entry for an index operation, this is not allowed. Maybe something should be set in parentheses or a comma is missing?
 at line: 2, column: 19

So, it was already being rejected as invalid syntax-wise. So, I used that as the point of reference and made the behavior same even for assuming that it was the behavior with which groovy has lived till now supporting all kinds of DSLs that it does

class A {
  def x = [[1][foo:2]]
}

My take is that we can also use this JIRA for bringing in the consistency in the compiler behavior and you can raise a new JIRA with the DSL example that you have in mind and then let it have its own cycle of analysis and implementation and then let's see.

Then it is upto you.

Hmm right, indeed, we can't write

def l = [1, 2, 3, 4]
l[1:2]

We have to write this instead:

def l = [1, 2, 3, 4]
l[[1:2]]

A possible example, in some DSL that would deal with text documents, would be to use that syntax for accessing a special line / column offset :

// getting a pointer to the 10th line, column 23
def place = document [ 10 : 23 ]

// getting a selection of text from the 10th line, column 23, to the 11th line, column 5
def selection = document [ 10 : 23, 11: 5 ]

The sole little drawback is when using variables for identifying lines, as lines are the keys, and because keys are strings by default in Groovy, we'd need the parens as follows:

def (line, column) = [10, 23]
def pointer = document[ (line) : column ]

Overall, I think we should allow maps to be allowed as keys.

Also, an additional note, the following would be equivalent:

foo [ a : b ]
foo [ [ a : b ] ]

This is consistent with how this works with method calls (both the following are equivalent):

foo ( a : b )
foo ( [ a : b ] )

So we should proceed with this improvement.

Well, none of the examples you have given are even compiling with 1.7-beta-1. I am not contending that the improvements you are suggesting should not be made, but fix of current JIRA is not breaking anything that was possible before. For me to rollback my fix, please give me one example that was working before and is not working now due to current JIRA's fix.

Else, all you say makes sense but the improvements need their new JIRA.

Agreed on the need for a new JIRA issue.

Now to come back to the initial example, yes, it doesn't currently parses correctly, but the error message should be better and made consistent.

Now, if we agree with the need of consistency with the map notation like this is working with method calls, then this example should compile again.
So it would mean that this issue should be marked as "won't fix", in favour of the yet-to-be-created issue.

I have lost a bit of thread here and I am not sure which example you are point to as the one that should compile again? Which is the example that was compiling earlier and is not compiling now? I have been looking for that in this discussion - an example that was working before and is not working now.

Another question is about the pre-my-fix, existing compiler behavior in case of

class A {
  static x = [[1][foo:2]]
}

Even if I rollback my code, compiler still rejects the above as before. So, what is the benefit in being inconsistent handling something (static/non-static cases) till the improvements you suggest make their way in?
Why not be consistent till the new improvements get implemented and roll back the compiler errors for both static and non-static cases together? That way the compiler is consistent throughout

I was referring to the first example posted in the description of this JIRA issue by Peter.
I had not looked at your fix. So I'm not saying that certain things which compiled before work now, or vice versa, don't worry!
So if you fixed this so that we get a proper compilation error rather than an AsmClassGenerator error, then that's great.

That said, if we open a new JIRA issue for the consistency with passing a map as a possibly index, just like we use maps in method calls, then the fix for that new JIRA issue will certainly make you make some changes to what you committed, as the script in the description of this JIRA issue will become "compilable" (although it will throw some error at runtime because there's no getAt() like that defined on map.

So as you fixed this issue, please close it, that's fine, and I'll open a new issue for the consistency suggestions.
Does this sound okay?

Yes, it does.

I am not worried either way - I can happily rollback, if convinced why.

My point was that marking this issue as "won't fix" would mean going back to the compiler inconsistency state and I just wanted to understand why that was preferred. I didn't see any plus in that approach.

IThe transition from

a) Your suggested improvement not in picture yet in implementation but compiler consistent in its errors/behavior.

to

b) The compilation errors removed consistently and your suggested improvement implemented.

made more sense to me, so the discussion here. Please don't get upset

I'm not upset at all! I think it's just me not being very clear, and you were perfectly right asking for more explanations!

So conclusion again: close the issue as fixed.

And I've created GROOVY-3795 to track the improvement we discussed here, with hopefully enough details and clarity.
But if it's not the case, don't hesitate to continue your comments, they are always very gladly welcome and appreciated!

Thanks again for your really great work Roshan!

Thanks a lot, Guillaume.

Fixed.