Remove REDUNDANT_FOR_VARIABLE_TYPE warning

[ryanabx]

Remove REDUNDANT_FOR_VARIABLE_TYPE
Change REDUNDANT_FOR_VARIABLE_TYPE to FOR_LOOP_VARIABLE_SUPERTYPE_SPECIFIED

These changes are still open to discussion!

Currently, REDUNDANT_FOR_VARIABLE_TYPE warns the user when they explicitly type a for loop variable as the same type as what's inferred by the analyzer. This discourages the user from explicitly typing their for loop variables:

for i: int in range(4): # Emits REDUNDANT_FOR_VARIABLE_TYPE warning
for i: Variant in range(4): #Emits REDUNDANT_FOR_VARIABLE_TYPE warning

It is a common convention in statically typed languages to annotate for loop variables, even if the iterator variable can be inferred by the analyzer:
https://www.w3schools.com/cs/cs_foreach_loop.php
https://www.geeksforgeeks.org/g-fact-40-foreach-in-c-and-java/#

Since it is a convention in modern statically typed languages, statically typed GDScript would be the exception to the general concensus. I have no problem with letting the analyzer infer the type of the iterator variable, but I do not think that a warning should be issued when a type annotation is used for a loop and the explicit and inferred types match.

EDIT 9/12/23: The discussion having progressed, seems to be leaning towards removing the REDUNDANT_FOR_VARIABLE_TYPE warning in its entirely. For those people behind in the discussion, it was previously discussed whether or not we add a warning for the strict supertype of an inferred variable instead, for example:

for i: A in my_b_list: # Type of my_b_list is Array[B] where B extends A

Most people seem to be of the view that no warning should be necessary in any case of explicitly typing a for loop variable.

As always, more discussion is welcomed.

Please let me know your thoughts! 😄

[dalexeev]

Depends on #81355

I don't think it's right, this PR should contain 1 commit with REDUNDANT_FOR_VARIABLE_TYPE changes, #81355 is unrelated. Also, #81355 has 4.x milestone (this doesn't mean it won't be included in 4.2), but this PR should be included in 4.2 so that renaming the warning doesn't break compatibility.

My motivation for adding the warning was that specifying the for loop variable type is often not really necessary.

1. for loop variable always infers type. You can do var b = a or var b := a, but you can't control that for for loop variable.
2. The for loop variable type specifier is intended primarily for type narrowing (and this is always unsafe code), not for readability.

Specifying the type in each for loop when it is inferred anyway is unnecessary and tedious. And I don't think it really increases readability. For example, it's obvious that i is int in for i in range(<...>) and for i in <...>.size(). Users may think that specifying the type in each for loop is required for the variable to have a hard static type. But this is not true, the type is automatically inferred. You should only specify the type when it doesn't happen or if you want to narrow the inferred type.

Perhaps it should be two warnings, one for the exclusive supertype, the second for the exact type. And both warnings should be enabled by default.

[ryanabx]

I don't think it's right, this PR should contain 1 commit with REDUNDANT_FOR_VARIABLE_TYPE changes, #81355 is unrelated. Also, #81355 has 4.x milestone (this doesn't mean it won't be included in 4.2), but this PR should be included in 4.2 so that renaming the warning doesn't break compatibility.

You're right. I can remove the changes that modify aspects of UNTYPED_DECLARATION and remove the dependency on #81355 . I'll go ahead and do that so we can keep discussion related to REDUNDANT_FOR_VARIABLE_TYPE

My motivation for adding the warning was that specifying the for loop variable type is often not really necessary.

1. for loop variable always infers type. You can do var b = a or var b := a, but you can't control that for for loop variable.
2. The for loop variable type specifier is intended primarily for type narrowing (and this is always unsafe code), not for readability.

In my opinion, the intention of the loop variable type specifier is not clearly indicated, and as a result, might leave users confused about its proper use.

Lots of languages use type indicators on their for loops (and I know that since GDScript is dynamically typed, and the type is inferred by the for loop analyzer regardless of annotation, it's a different story, but convention is important here in my opinion) Even statically typed languages which could reasonably infer the type of an iterator typically require an annotation anyways:

// C# code snippet
List<string> greetings = new List<string>(){"a", "b", "c", "d"};
foreach(string greet in greetings) {
    // code here
}

For users coming from other languages, they might become confused as to why explicitly naming the type of their iterator is not recommended. At the very least it should probably be documented somewhere, perhaps in one of our sections about static typing. If it's already documented there let me know.

Specifying the type in each for loop when it is inferred anyway is unnecessary and tedious. And I don't think it really increases readability. For example, it's obvious that i is int in for i in range(<...>) and for i in <...>.size(). Users may think that specifying the type in each for loop is required for the variable to have a hard static type. But this is not true, the type is automatically inferred. You should only specify the type when it doesn't happen or if you want to narrow the inferred type.

I think it increases readability, because not every list has a type that is immediately obvious to a reader given the name of the list. It gives the user an immediate heads up on what things it can do with the iterator variable, given the type annotation.

As for helping users realize they don't need an annotation for a hard static type, I don't know the best way to get that information across, but I don't think we should be discouraging attempts at ensuring a hard static type (even if they're potentially misguided)

Perhaps it should be two warnings, one for the exclusive supertype, the second for the exact type. And both warnings should be enabled by default.

I could be convinced that having two warnings would be a good idea. I'll have to think on it more!

[ryanabx]

I have thought about whether I think having two warnings is necessary, and I still don't see the point in a redundant for variable warning. However, if one were to exist, I don't think we should set it to WARN by default. I think this setting should be set to IGNORE by default. My primary citation for that reasoning is convention from other languages (no language to my knowledge warns users for annotating their variables), and also simply the personal testimony that I type my for loop variables, even if they are implicitly the same. I believe that there are others that would also be annoyed about a warning such as that being on by default.

[dalexeev]

Well, you can only do 4 things with inferred type:

1. Don't specify an explicit type unless you are happy with the inferred type. Unlike other declarations, the for loop variable automatically infers the hard type if possible.
2. Specify an explicit type equal to the inferred one if you want to make the code more readable/reliable. This is optional due to point 1 and depends on the style you prefer.
3. Specify a less specific type (supertype). This is definitely weird, since you would only need it if you wanted to assign some other value to the for loop variable, which is not generally considered good style.
4. Specify a more specific type (subtype), although this is unsafe. This feature was added precisely for this purpose.

I agree that point 2 may be a valid need if the user prefers a more verbose style and that it is reasonable to distinguish between points 2 and 3.

But I'm still not sure if this is a style we should encourage. In my opinion, inferring the for loop variable type in 4.x is very convenient; the main reason why this feature is relevant is untyped dictionaries and the absence of nested types (like Array[Array[int]]). If/when this is added, you will be able to almost never specify the type. Except for cases like for control: Control in get_children(), but I don't think it's needed often.

Whether or not a warning is necessary in case 2, I suggest discussing it at the GDScript team meeting (and in this thread before). I don't have a clear opinion on this issue.

[Lielay9]

But I'm still not sure if this is a style we should encourage.

The current style guide already encourages adding a type when it might not be "clear" what it is:

Bad:

...
# What type is this? It's not immediately clear to the reader, so it's bad.
var value := complex_function()

As there's no warning for not using :=, there shouldn't be one for for-loops either. Besides, I find discouraging verbosity quite ironic considering the c++ guidelines.

Please don't use the auto keyword for type inference. While it can avoid repetition, it can also lead to confusing code

[Mickeon]

I'm going to chime in on this and say that I do find the original warning to be quite useless, and removing it in favour of this PR's warning makes complete sense. If REDUNDANT_FOR_VARIABLE_TYPE is on by default when I get my hands onto 4.2, I would turn it off in a heartbeat for each new project of mine, or alternatively be so annoyed by it that I'm going to forget about the feature entirely (bit of exaggeration, of course).

This is not from an "internal workings of Godot" perspective, mind you, Rather, it's from the average user's perspective, that just would like to write slightly more verbose code on the following few occasions:

• The loop variable's type may be unclear at first glance:
  • Could be the ambiguous variable names. Not everyone has mastered the craft of naming things.
  • Could be for using mixed types around the same code (e.g. looping through indexes VS. looping through keys),
• The container's type isn't inferred properly.
  • Could be an issue of GDScript, the editor, the user simply not wanting to make the array typed, anything of the sort.
  • As such, autocompletion options don't appear, which is quite painful.

[dalexeev]

I agree with this PR and will approve it after some fixes. The combined warning was my mistake. The warning for strict supertype definitely makes sense. The warning for equal types is controversial, so we shouldn't add it without consensus.

Also should add tests for this case (modules/gdscript/tests/scripts/runtime/features/for_loop_iterator_specified_types.gd).

Thanks for the PR @ryanabx and thanks for the comments to all participants!

[adamscott]

I know that REDUNDANT_FOR_VARIABLE_TYPE doesn't have it, but I suggest to add the word "loop" to the constant, such as REDUNDANT_FOR_LOOP_VARIABLE_TYPE, makes it easier to read.

[dalexeev]

My thoughts after the meeting (about removing the warning entirely, and not just for equal types). The for loop variable has an inconsistency compared to other variables in that it automatically infers the hard type (if possible), not the weak type (since 4.0 release, #80247 did not change this). Although this can be confusing for users, it is quite convenient and makes the code more optimized.

for i in range(5):
    i = "p_" + str(i) # Error, you cannot assign a string because `i` is **hard** int.

Now you can remove an inferred type by specifying Variant (however, the feature was not originally intended for this).

for i: Variant in range(5):
    i = "p_" + str(i) # OK.

You can simply declare a new variable instead of reusing the same variable. Languages generally recommend declaring new variables rather than reusing old ones.

for i in range(5):
    var s: String = "p_" + str(i) # OK.

I still think the warning improves code style specifically for the for loop variable, even though we don't have a similar warning for the rest of the variables.

@ryanabx @adamscott @vonagam But I don't mind removing the warning entirely if a lot of people support it. We may add a warning later when we reach consensus.

[vonagam]

I am for removing the warning.

[ryanabx]

I still think the warning improves code style specifically for the for loop variable, even though we don't have a similar warning for the rest of the variables.

@ryanabx @adamscott @vonagam But I don't mind removing the warning entirely if a lot of people support it. We may add a warning later when we reach consensus.

The way I see it, there are two use cases for specifying a super type instead of the inferred subtype:

1. Information hiding. This was mentioned in the meeting, but some programmers prefer to type a for loop variable as a super type simply because they only need functions defined in that super type.
2. Explicit type annotations. If we wanted to add a warning for lack of explicit type annotations, I believe the for loop variable wouldn't be an exception. If a user doesn't know the type of the loop, they may use a super type instead. This use case promotes non exact type binding; however, the user will only be able to use whatever functions are available in the super type they specify, which goes hand in hand with use case 1. A warning for not having an explicit type annotation goes counter to a warning for having one that's a super type. It would make the scope of the explicit type definition limited to the exact inferred type or subclasses of it.

I'm sure about my position on removing the warning for when the annotation is the same as inferred, but I'm still gathering opinions on the super type warning.

[produno]

After testing i came here looking for this, or a reason why there was a warning. I agree with others that there shouldn't be a warning. Specifically because i like to ensure everything is typed and explicit anyway, which to me makes the warning obsolete. I think this should also be encouraged.

As i went back through my code to add these i noticed places where i wasn't sure of the type, but once added i had the warning saying it was already inferred. If i the programmer did not know it was inferred and did not know what type it was at a glance, then i need to ensure i add the type.

If i can turn it off then its not a massive problem but i was still confused by it, so i am sure plenty of others will be too.

[ryanabx]

I have updated the commit, rebasing to match current master, and making a few modifications:

The warning for strict supertype is now FOR_LOOP_VARIABLE_SUPERTYPE_SPECIFIED.

I also opted to make the warning IGNORE by default (changed from WARN by default). After getting a lot of feedback on the need for a warning, this might be a good compromise to work with for now. Let me know your thoughts, though.

EDIT: For those of you just coming into the discussion, most of the discussion has moved to whether or not an explicit supertype warning is needed: i.e.

for i: Variant in range(4): # Warning? Or not?

Most, if not all of us have agreed now that a redundant type warning is not needed, and should be removed: i.e.

for i: int in range(4): # No more warning

[vonagam]

Making it IGNORE is acceptable but it is proper to remove it entirely. @dalexeev more or less ok with removal after the last meeting and I don't see anybody else who was for keeping it. As shown there are valid use cases and keeping the unnecessary warning will just pollute the code (only a little but still).

In problematic examples of such sort:

for i in range(5):
    i = ...

I see "the code smell" in the assignment to iteration variable here and typing is irrelevant. Yes, with i: Variant it will be possible to do i = "strange" but even with hard inferred int type one can do i = 5 which is not cool the same.

And in general I do think that it is more productive to talk about for control: Control in my_controls then about unrealistic for i: Variant in range(4).

[ryanabx]

Making it IGNORE is acceptable but it is proper to remove it entirely. @dalexeev more or less ok with removal after the last meeting and I don't see anybody else who was for keeping it. As shown there are valid use cases and keeping the unnecessary warning will just pollute the code (only a little but still).

You're right. I think my tendency is to want to compromise even if one person is a holdout, which is not always the best thing to do. I haven't seen anyone else state that they are for such a warning, nor do I personally think a warning for any explicit typing of for loop variables is necessary. Pending review from @adamscott , @dalexeev , or other members of the team, I will just remove all warnings of the sort for now. If there are any more opinions to be shared, this discussion can remain open :)

In problematic examples of such sort:

for i in range(5):
    i = ...

I see "the code smell" in the assignment to iteration variable here and typing is irrelevant. Yes, with i: Variant it will be possible to do i = "strange" but even with hard inferred int type one can do i = 5 which is not cool the same.

Yes, I don't think changing i at all is good practice. For other reasons, static typing with a supertype might be a good idea (the reasons I mentioned above)

And in general I do think that it is more productive to talk about for control: Control in my_controls then about unrealistic for i: Variant in range(4).

True, for my personal opinion which aligns with yours (no warning should be necessary for either option) I felt like any example explains my point well enough, but I understand that a better example would help convey the point much easier.

[YuriSizov]

Thanks!