-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Python-style list comprehensions to GDScript #81788
Conversation
Just tested what Python does in case of an This is valid (obviously):
so is this (ternary on the list, not a filtering operation):
but this isn't:
So, it seems like just disallowing ternary at that place should work. if a ternary really is needed for the list, it should be placed in |
Apart from dictionary comprehensions (which I'm not sure if it should be a part of this PR or not), this PR should be ready for review. |
9910532
to
4df80b0
Compare
That was an accidental close |
Please use |
58bc811
to
ac3f41d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that we don't have a consensus on this feature yet. There are concerns about the readability of the syntax and the need for this feature in the context of game development. Regular for
loops do not look cumbersome, they are just 3-4 short lines instead of 1 long line. In my opinion, this is purely syntactic sugar, which does not add new functionality, and complicates the grammar of the language. Also arrays have map()
and filter()
methods.
I took a look at the implementation and added a few comments. Note that you don't have to do anything about this for now, since there is no consensus on the proposal yet.
Additional notes
1. We should not just imitate Python and always copy its features and syntax. It is worth comparing other languages, how they implement or do not implement the feature. Perhaps we will find some other option less confusing. See also the comment about the when
token instead of if
as a separator for filter expression.
2. The proposal and the PR forgot about typed arrays. In the case of a regular loop, we can specify the array type:
var a: Array[int]
for x in list_expr:
a.append(f(x))
But in the case of list comprehensions this is not provided as part of the syntax, you can't do it like this (see also #71336):
var a := Array[int]([f(x) for x in list_expr])
I haven't looked into this PR closely, but it seems to always generate an untyped array, and even the following won't work:
var a: Array[int] = [f(x) for x in list_expr]
See also godotengine/godot-proposals#7364. We haven't fully resolved all design issues with typed arrays.
ExpressionNode *list = nullptr; | ||
IdentifierNode *variable = nullptr; | ||
ExpressionNode *condition = nullptr; | ||
bool use_conversion_assign = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding use_conversion_assign
without datatype_specifier
doesn't make sense. See #80247. Perhaps list comprehensions should also support a static type specifier.
ExpressionNode *parse_expression(bool p_can_assign, bool p_stop_on_assign = false, bool p_disallow_ternary = false); | ||
ExpressionNode *parse_precedence(Precedence p_precedence, bool p_can_assign, bool p_stop_on_assign = false, bool p_disallow_ternary = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a similar question in #80085 (see discussion in godotengine/godot-proposals#4775). Perhaps instead of disabling the ternary operator in some cases, we should introduce a new token (when
) that will act as a separator. No decision has been made yet.
@@ -3745,6 +3791,20 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident | |||
} | |||
} | |||
|
|||
// Check if we are inside a comprehension. This allows the comprehension to access the variable it is iterating over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that identifier is resolved correctly. We recently fixed a bug with scopes (see #79880). It is worth checking that the resolution here is consistent: a local identifier should not shadow class or global identifier in the list expression. Also, the implementation is probably a little over-complicated (but I'm not sure).
@@ -1501,6 +1520,7 @@ class GDScriptParser { | |||
ExpressionNode *parse_ternary_operator(ExpressionNode *p_previous_operand, bool p_can_assign); | |||
ExpressionNode *parse_assignment(ExpressionNode *p_previous_operand, bool p_can_assign); | |||
ExpressionNode *parse_array(ExpressionNode *p_previous_operand, bool p_can_assign); | |||
ComprehensionNode *parse_comprehension(ExpressionNode *expression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ComprehensionNode *parse_comprehension(ExpressionNode *expression); | |
ComprehensionNode *parse_comprehension(ExpressionNode *p_expression); |
Prefix arguments with p_
@@ -2734,8 +2745,40 @@ GDScriptParser::ExpressionNode *GDScriptParser::parse_await(ExpressionNode *p_pr | |||
return await; | |||
} | |||
|
|||
GDScriptParser::ComprehensionNode *GDScriptParser::parse_comprehension(ExpressionNode *expression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GDScriptParser::ComprehensionNode *GDScriptParser::parse_comprehension(ExpressionNode *expression) { | |
GDScriptParser::ComprehensionNode *GDScriptParser::parse_comprehension(ExpressionNode *p_expression) { |
@@ -108,10 +108,10 @@ class GDScriptCompiler { | |||
generator->start_block(); | |||
} | |||
|
|||
void end_block() { | |||
void end_block(bool expect_no_locals = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void end_block(bool expect_no_locals = true) { | |
void end_block(bool p_expect_no_locals = true) { |
@@ -79,7 +79,7 @@ class GDScriptCodeGenerator { | |||
virtual void end_parameters() = 0; | |||
|
|||
virtual void start_block() = 0; | |||
virtual void end_block() = 0; | |||
virtual void end_block(bool expect_no_locals) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual void end_block(bool expect_no_locals) = 0; | |
virtual void end_block(bool p_expect_no_locals) = 0; |
@@ -468,7 +470,7 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator { | |||
virtual void end_parameters() override; | |||
|
|||
virtual void start_block() override; | |||
virtual void end_block() override; | |||
virtual void end_block(bool expect_no_locals) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual void end_block(bool expect_no_locals) override; | |
virtual void end_block(bool p_expect_no_locals) override; |
do { | ||
ComprehensionNode::ForIf for_if; | ||
|
||
if (consume(GDScriptTokenizer::Token::IDENTIFIER, R"(Expected loop variable name in list comprehension)")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (consume(GDScriptTokenizer::Token::IDENTIFIER, R"(Expected loop variable name in list comprehension)")) { | |
if (consume(GDScriptTokenizer::Token::IDENTIFIER, R"(Expected loop variable name in list comprehension.)")) { |
@@ -2249,6 +2249,7 @@ GDScriptParser::ExpressionNode *GDScriptParser::parse_precedence(Precedence p_pr | |||
// Allow keywords that can be treated as identifiers. | |||
token_type = GDScriptTokenizer::Token::IDENTIFIER; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated whitespace change
List<int> used_temporaries; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -86,7 +86,9 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator { | |||
|
|||
Vector<StackSlot> locals; | |||
Vector<StackSlot> temporaries; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void GDScriptAnalyzer::resolve_iteration(GDScriptParser::ExpressionNode *list, | ||
GDScriptParser::IdentifierNode *variable, | ||
bool *use_conversion_assign, | ||
GDScriptParser::TypeNode *datatype_specifier) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void GDScriptAnalyzer::resolve_iteration(GDScriptParser::ExpressionNode *list, | |
GDScriptParser::IdentifierNode *variable, | |
bool *use_conversion_assign, | |
GDScriptParser::TypeNode *datatype_specifier) { | |
void GDScriptAnalyzer::resolve_iteration(GDScriptParser::ExpressionNode *p_list, GDScriptParser::IdentifierNode *p_variable, | |
bool *r_use_conversion_assign, GDScriptParser::TypeNode *r_datatype_specifier) { |
This comment was marked as abuse.
This comment was marked as abuse.
@nukeop i think in yesterday's gdscript meeting, the discussion for the inclusion of this feature was postponed to 4.3 |
Not every proposal ends up with a consensus. If there are concerns which cannot be addressed then something may not get implemented/approved. While there are clearly people who want this feature, it doesn't mean it's going to be added if language maintainers don't find it fitting the language design. |
This comment was marked as abuse.
This comment was marked as abuse.
A consensus, at least in the sense that we use it here, does mean a positive decision to approve the proposed change. It's different from the final decision that maintainers can make to approve or reject the feature. A consensus comes from everyone agreeing on the next step, and while there are still people who request this feature, the only consensus that we can have is to accept it. Of course, if the request is withdrawn after concerns are raised, that would also be a consensus, but that seems unrealistic to me in this case. So a consensus is not always possible, and this can die due to the lack of a consensus, because we prefer to preserve the status quo when we can't agree on the next step. So the final decision by maintainers can come from the fact that there is no consensus, and that can be the end of the story. |
How come it was postponed? |
I assume it's because it's a major feature and we're almost at feature freeze so little time to do proper testing |
This comment was marked as abuse.
This comment was marked as abuse.
This PR was opened last week so no that isn't the same, there was no PR for it then Also what do you mean by "excuse"? This PR was opened very close to feature freeze, but since the support for this particular feature is very mixed and many strong arguments against it has been raised it is not a simple question of "just add it". So even if this wasn't still a non-agreed upon feature with mixed support there's no way this could have been rushed through before feature freeze, a week simply isn't enough time to review and test |
This comment was marked as abuse.
This comment was marked as abuse.
I'm also wondering what's the exact reasons for postponing the discussion. Is there something like a meeting summary? I mean, even when the discussion is postponed due to a lack of consensus, there should be a summary of the viewpoints from both those in favor and those opposed. So people could improve what they have now accordingly. p.s., There are two different reasons given for the postpone: because a lack of consensus, and because we don't have time to do proper testing. It does sound like a perfunctory response. |
I am not aware of the specific reasons, I gave my guess, but the fact that we are now in feature freeze week or thereabouts such a major feature being added isn't really realistic
This is just exaggeration, this PR posted a week ago, it hasn't been lingering at all, a feature such as this isn't something you merge in a week 🙃 |
This comment was marked as abuse.
This comment was marked as abuse.
Feature freeze starts from next thursday. I don't think it's reasonable to start rejecting something because we're one or two weeks from feature freeze. Isn't that the point of setting a specific feature freeze date? |
Well this hasn't been reviewed and the specifics are still being discussed, so there's the question of "should we spend time on debating this on short timeframe or leave it for next cycle" The argument of "rejecting something because we're one or two weeks from feature freeze" only really applies if something is ready to go right? Again I don't know the reasons here, but there has been questions raised about correctness of some code here, and about the specific syntax, and the need for this feature in the first place, so packing that entire thing, deciding syntax, reviewing code, and testing it into at most 10 days is a bit of a stretch no? Getting a major feature like this merged in 18 days is fast, very fast |
But again, just my guesses, but the reaction here is very agitated, and presents it as if this feature is being killed when it's just postponed, and presenting it as if this PR is forced to linger when it is very very new Have a nice day, but those are my theories, let's not get up in arms about our favourite feature not getting here fast enough (I'm specifically talking about the exchange I was having right here, not about anyone else) |
Hey everyone! I think there is a little bit of misinterpretation of what @kahveciderin has said. The discussion itself has been postponed, as in there has been no discussion and more pressing matters have been prioritized. There is nothing to summarize from the meeting, no consensus or lack thereof from it to discuss here. To put it plainly: this feature is not at the top of the priority list for the GDScript team. It's controversial enough to not be an easy decision to make, and the team is not ready to discuss the idea right now, preferring their time to be used on things which can be resolved in the meantime.
@nukeop You are very much aware that @vnen has made several clear statements, including to yourself specifically, that warn about this feature being controversial and questioning its usefulness. We cannot stop anyone from implementing a PR despite those statements, but you can't claim that there has been lack of clear communication. Now, it starts to look like you are purposefully fanning the flames under this discussion. If it's genuine frustration, I'm sorry, but you have to learn to deal with the fact that not every suggestion, no matter how popular it may appear, is going to end up being accepted. You have been warned about the entitled tone of your messages already. I would like to remind you about our code of conduct and advise you to keep your comments to constructive feedback. What you're doing is not helping with the development, the review process, or a positive attitude towards the feature itself. I would strongly suggest everyone to take a break until there is anything worth discussing, i.e. until after the GDScript team has a chance to deliberate on this at some later date. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
what strong arguments have been raised against it? I haven't seen any personally
sorry I am not angry or "up in arms", I'm just confused why this feature has been delayed again and again with no real communication |
@JohnnyUrosevic You can refer to the linked proposal, godotengine/godot-proposals#2972, for arguments for and against the feature, as well as official communication on the matter. Do note that even if you or somebody else don't consider arguments against as valid, doesn't mean we can ignore them. It would be very improper for maintainers to only take seriously those who want the feature. I would also like to repeat myself: everyone interested in this feature should take a break from discussing it and wait for more news. There is absolutely no point in asking the same question over and over again to get the same answers. I appreciate that you may not care that some community members and maintainers don't like the idea of this feature and you can say that they are free not to use it. That still puts the proposal into the controversial territory. Language design is hard. There needs to be a consensus why we are making the syntax, the parser, the grammar more complicated. |
@YuriSizov I have been following that thread for years. As far as I can tell, no one in charge of designing the language has said anything in that thread about what concerns are preventing them from implementing this. Last I heard was "we will discuss it after Godot 4" and then silence. I just think some transparency would be nice for such a heavily suggested feature. I have been taking a break and waiting for news for years at this point, I just want to see what the big issue is |
@JohnnyUrosevic The summary from the people behind the language has been given 5 years ago:
Again, you might disagree, and that's fine. Nobody is trying to convince you that your preferred syntax is wrong. We just didn't find it fits the goals of the language, and the argument in favor of it being readable is mostly supported by people who are already used to the syntax from years of Python usage. GDScript doesn't require you to learn Python. It doesn't require you to build Python-specific experience and familiarity. And if we take that out of question, it becomes less of a strictly positive change. We also have to consider, as I've mentioned, making the parser and the grammar more complicated, covering all corner cases, making sure that the new language feature can be used cohesively, doesn't conflict with other features. Just because it's a solved problem in Python (I assume), doesn't mean it would be any easier to solve it here. GDScript is a different language with only vague similarities with Python. It owns Python nothing in terms of adapting its syntax. Finally, for such a passionately discussed language feature, it is little else than syntactic sugar. It doesn't really unblock new ways to code logic and implement algorithms. It just lets you write code that is usually represented in blocks as a one-liner. So we are discussing adding a new complex syntax that offers little functionality and may lead to people writing GDScript that other community members will find hard to comprehend. If you can't see why this is a controversial matter, I don't think any answer that maintainers can give you would satisfy you. Whether or not this puts the end to this discussion, I'm leaving it at this. Any further discussion unrelated to the actual code and feature review on this PR lowers its chances of being reviewed, because these long discussions on PRs get in the way of reviewers. |
@YuriSizov so if they have made their decision and aren't gonna change it why is the proposal not closed? |
I normally wouldn't be a part of this discussion here, as I felt like I (with the help of other people in the discussion as well) expressed why this feature is needed, but after seeing this discussion here as well I felt the need to chime in. As the owner of the proposal & this PR, and also as someone who has worked with nearly every language under the sun in the past ~10 years, I'd like to offer my perspective. I understand that GDScript is a domain-specific language, tailored to gamedev, but at least to me, it feels incomplete most of the time. When I first started my Godot journey, GDScript seemed like the least polished part of Godot, and I honestly still think the same. The weird limitations it imposes are a hurdle in my development process, and thus GDScript actually reduces my productivity when working on Godot. GDScript lacks basic functionality that most other programming languages do have, and therefore isn't "done" (like v0.x.y of a product). I don't remember feeling this confined in a programming language ever, and that's not in a good way. Seeing how the proposal evolved, I began to wonder how many more awesome GDScript features are waiting for approval, just like this one, with (in my opinion) weird reasons (like how complexity is quoted in this proposal), and how complete it has the potential to feel when the language becomes just a bit more mature. (I don't want to be misconstrued, it's not my intention to disrespect anyone who has worked with the development of GDScript at all, I'm just stating my experiences, and all my assesments are subjective, maybe GDScript is someone's favorite language, I don't know) Moving on to the topic in hand, the purpose of list comprehensions is more than just line reduction; it syntactically expresses that we're creating a list out of an iterator (sometimes conditionally). In my humble opinion, I don't find list comprehensions hard to read. Following a for loop is harder than reading a list comprehension. You need to make sure the for loop doesn't have any other side effects while reading, but list comprehensions do one thing and one thing only, which is to create a list from an iterator. Even if we consider that list comprehension syntax might be a bit confusing for an absolute beginner programmer, I honestly don't think GDScript should be someone's first programming language. Just like SQL, it's a specialized tool, and beginners typically learn programming concepts in more general-purpose languages before diving into domain-specific ones. List comprehensions and similar constructs are already part of languages such as Python, C#, Fortran, Elixir, Erlang, F#, Haskell, and more. In the proposal, someone has also mentioned that the representation is also there in math ( I understand that the core maintainers are trying to see this feature through a game developer's glasses and assign meaning to it in that context. However, saying that this feature isn't strictly about game development is similar to saying, let's say, the XOR operator isn't strictly about game development. Truth is, it has it's place. I (and I assume some of the supporters of this proposal) feel rightfully limited when working with iterators & lists, and this limitation honestly decreases my productivity a lot because now I either need to write a function (or a lambda) that does what I want, which is one more thing to keep track of. Also I would like to add that there are some misunderstandings in the proposal. Some people, for example, talked about the list comprehensions not being able to use Regarding the delays in the discussion phase, and also the old (I think now 6-year-old?) PR getting closed after a lack of discussion, I understand that this isn't the #1 priority right now, and I wouldn't expect this to be included in the release anytime soon. But I invite everyone, especially those who are strictly for this change, along with the core GDScript maintainers, to use this version of GDScript with list comprehensions for a while to see how it improves things. Those who have a use case that could be worth mentioning can simply mention it in the proposal discussion, so we would have more strong reasons to (hopefully) get this accepted. Anyways, just my two cents. |
Also supersedes #51997 |
This comment was marked as abuse.
This comment was marked as abuse.
|
I was researching a missing feature in C# and came across this Stack Overflow answer that seemed relevant to this topic. Let me explain. Basically, C# left out a useful feature because of reasons similar to ones given here like language purity, not increasing complexity, fitting the language design, and a notion of how you are "supposed" to code. Yes, it's not a 1:1 comparison since the C# feature isn't syntactic sugar, and they did have another reason for not implementing it (which was debatable too, see 2nd answer). However, it is similar in the sense that there is a demand for it, it's been done in other languages before and proven useful, and the only reason for not implementing it is to keep the language simple. List comprehensions are an elegant way to write code, they are more readable (subjective, sure) than multiple lines of a for loop, and everything else others have probably already said. Yes, they are technically syntactic sugar but it improves the developer experience, which is, in my opinion, something we should care more about than the approachability of the language by beginners. A useful feature that's in demand shouldn't be left out because of reasons like keeping the language more beginner-friendly. Also, languages borrow features from each other all the time, and it's not like there's going to be a new groundbreaking syntax for some incredible novel functionality. Comprehensions aren't exclusive to python, and GDScript doesn't have to shy away from implementing it with such shallow reasoning. That being said, I am aware that features require maintaining, checking for edge cases, etc. I've never worked on a compiler before so I'm not aware of all the possible roadblocks but my intention with this comment is not to say "what are we waiting for, just merge it already", but to just voice my support for the feature and share my thought process on this matter since I think it will apply to other feature requests down the road (or current ones). If we want Godot to improve and be used by more and more developers, it's inevitable that people will ask for features like this that some people might find unfit for the language design. However, I think it's also important to realize that programming languages are not prescriptive, they are descriptive. They evolve over time because they are just tools at the end of the day and developers' needs change and more people use the language so it needs to adapt. That's not to say that it needs to implement every feature and become bloated, of course. I can't even write a statically typed This felt a bit dramatic to type, but I felt the arguments against this feature were just shallow. Sorry if my language sounded rude or anything. I recognize that I haven't contributed anything to Godot and I respect the previous contributors for their work. |
Hi, is dictionary comprehension going to be a part of this pr as well? |
@lemonJumps i can work on it if that's desired, feel free to share your use case though at it allows us to get more recognition from the people who are responsible for deciding if this gets merged or not. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This feature won't be a part of 4.2, and until there is a decision by the GDScript team, there is not much that can be discussed. So let's give it a bit of time, shall we? |
For the reasons mentioned here, I'm closing the PR. Thanks for your work, nonetheless, @kahveciderin. Don't hesitate to join us more often in our weekly meetings. |
This adds python-style list comprehensions to GDScript.
Examples:
To Do:
Issues:
Invalid operands 'Nil' and 'int' in operator '>'.
Closes godotengine/godot-proposals#2972