Add Python-style list comprehensions to GDScript

[Lucrecious]

Code Explanation

Hi, this is my first attempt at contributing to Godot!

I'm a fan of languages even though I've never done anything formal, but I decided to give this a shot with the little knowledge I remember from my favourite language classes in university.

Aside from a little clean-up, refactoring and figuring out a small bug, I think this is close to code-review quality.

To implement this I created a new ForIfClauseNode that gets recognized in the parser.

Here's the additional section to the language grammer I added logically to GDScript:

arrayDecl = "[" [ expression { "," expression } "," ] "]"
    | "[" [ expression forIfClauses ] "]";
dictDecl = "{" [ keyValue { "," keyValue } "," ] "}"
    | "{" [ keyValue forIfClauses] "}"
 ;
keyValue
    = expression ":" expression
    | IDENTIFIER "=" expression
    ;
forIfClauses = forIfClause { forIfiClauses } ;
forIfClause
    = "for" IDENTIFIER "in" logicOr { "if" expression } ;

The reason for the logicOr instead of an expression is so that the ternary operator doesn't get considered. This is sort of how the python one works as well, and I assume it's partly because of their ternary operator.

Here's python's for-if-clause:

for_if_clauses:
    | for_if_clause+
for_if_clause:
    | ASYNC 'for' star_targets 'in' ~ disjunction ('if' disjunction )* 
    | 'for' star_targets 'in' ~ disjunction ('if' disjunction )* 

Our syntax allows either for a for-if-clause or a comma separated list of expressions but not both.

This works for both Dictionary and Array.

The compiler essentially converts this

var array := [ expression_template for identifier in expression_range if condition ]

into this

var array := []
for identifier in expression_range:
    if condition:
        array.push_back(expression_template)

And it does something similar for dictionaries.

I have a test file written as well that tests a number of different scenarios. I plan for it to be more comprehensive in the actual pull request.

Things I Need Help With

Bug!

There's a bug that stems from my ignorance with the compiler, I'm hoping I'm missing something small.

prints('3x3 grid only diagonals:',[Vector2(x, y) for x in range(3) for y in range(3) if x == y])

This should print an array that looks something like [(0, 0), (1, 1), (2, 2)] but it only prints [(0, 0)]. I'm assuming this is a scope issue? But this also works in other scenarios. The code is very, very simple so I'm hoping the language gods can see what I'm missing.

Building the Array or Dictionary

In the PR you'll see two ways I'm appending the template element to the array or dictionary.

With the array, I simply use the write_call_builtin_type which feels messy. I'm sure there's a cleaner way to do it.

With the dictionary, I created a new OPCODE that allows me to insert value into the dictionary. This seems like the way to go, but again, not sure and would like guidance here.

Bugsquad edit: closes godotengine/godot-proposals#2972

[AaronRecord]

FYI the commits should be squashed. It doesn't really matter until the PR is ready for review though :)

[Calinou]

This should print an array that looks something like [(0, 0), (1, 1), (2, 2)] but it only prints [(0, 0)]. I'm assuming this is a scope issue? But this also works in other scenarios. The code is very, very simple so I'm hoping the language gods can see what I'm missing.

Try with Vector2i instead of Vector2 to sidestep possible issues with floating-point precision.

[AaronRecord]

var array := [ expression_template for identifier in expression_range if condition ]

This is the same as this, right?

var array := expression_range.filter(func(indentifier): return condition))

If you need to modify it you could do:

var array := expression_range.filter(func(indentifier): return condition)).map(func(expression_template): return 2)

Edit: I'm not strictly for or against this change, I'm just pointing out existing alternative ways to do the same thing (if you just need to filter the array, the .filter(...) way is shorter).

[Lucrecious]

var array := [ expression_template for identifier in expression_range if condition ]

This is the same as this, right?

var array := expression_range.filter(func(indentifier): return condition))

If you need to modify it you could do:

var array := expression_range.filter(func(indentifier): return condition)).map(func(expression_template): return 2)

Yes! And honestly, with the addition of lambdas and these methods, list comprehension isn't really necessary. Every benefit I like about this is already what we get with the new GDScript.

I still like these and luckily, I only spent a couple of hours on this, so I'm cool abandoning this. I'm also cool working on it and including it if people actually want it.

[sairam4123]

Well, I feel like List comprehensions are readable more than filter and map functions, also please don't abandon this cool work, this is certainly desired by a lot of people, who come from Python.

[JestemStefan]

@Lucrecious Do you have any benchmarks like normal loop over array vs array.map() vs list comprehensions?

I'm currently struggling with iterating over huge array so I'm interested :P

IMO list comprehensions are less readable then something like map() method, but if they are faster then I give it 👍

[Lucrecious]

I do not have any benchmarks, I'll try to do some this weekend!

@sairam4123 I'm more than happy to continue the work if people want it - I'd love to make a contribution to Godot (my favourite engine 🥺). I'm just not too motivated since the main contributors seem very against list comprehensions.

But like I said, I'm more than happy to keep tabs on this.

[Lucrecious]

That was an accident close.

[AaronRecord]

I'm more than happy to continue the work if people want it - I'd love to make a contribution to Godot (my favourite engine 🥺). I'm just not too motivated since the main contributors seem very against list comprehensions.

But like I said, I'm more than happy to keep tabs on this.

I wouldn't stress about it, this is your first PR, and whether or not this gets merged there'll be other opportunities to contribute 😄, and hopefully you've learned some things while making this.

[adamscott]

For the reasons mentioned godotengine/godot-proposals#2972 (comment), I'm closing the PR.