godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Make the script editor not emit "shadowing" warnings when reusing non-static names in static functions #6898

Open L4Vo5 opened 1 year ago

L4Vo5 commented 1 year ago

Describe the project you are working on

A level editor.

Describe the problem or limitation you are having in your project

When a static function has a parameter/variable with the same name as an instance variable, the SHADOWED_VARIABLE and SHADOWED_VARIABLE_BASE_CLASS warnings are emitted, but nothing is actually being shadowed.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

In static functions, the GDScript editor should fully ignore non-static instance variables and methods, including avoiding the warnings.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

(same as above ?)

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around by ignoring the warnings.

Is there a reason why this should be core and not an add-on in the asset library?

Warnings are core.

dalexeev commented 1 year ago

Probably the warning message needs to be improved, but personally I find name collisions to be error-prone (even between static and non-static members). Static members are not separated from non-static members by barbed wire, and you are not required to use the self and Self prefixes (#391 keyword for accessing the current class, not yet implemented).

L4Vo5 commented 1 year ago

Turns out the part related to errors was a bug, so I edited the proposal to focus on the warnings.

leifb commented 4 months ago

While this discussion seems to be dead, I still want to add my thoughts:

I use shadowing all the time, to me it's an important core feature of any language. I try to give my variables descriptive, but short and concise names and avoid using any kind of prefixes. This results in getting the warning a lot. The most common case is constructor methods:

class_name WebSocketPacket

var type := ""
var data: Variant = null

@warning_ignore("shadowed_variable")
static func create(type: String, data: Variant):
    var instance = WebSocketPacket.new()
    instance.type = type
    instance.data = data
    return instance

In this case type and data represent exactly the same thing, so I give them the same name.

But there are also non-static cases where shadowing is exactly what I want:

signal status_changed()

enum Status { WAITING, RUNNING, STOPPED }

var status := Status.WAITING

@warning_ignore("shadowed_variable")
func update_status(status: Status):
    if self.status == status:
        return

    self.status = status
    self.status_changed.emit()

In my opinion, this is very readable code, with the exception of that pesky warning_ignore annotation. Using something like new_status would be a worse option to me, but I tend to do this either way now because it's easier than adding the annotation.

In the end, I think it's personal preference. I understand that the warning is helpful to beginners or those who simply don't like shadowing. But I feel like the warning system should not be used to dictate a certain code style. Shadowing is a feature of the language and users should be allowed to use it. But to me, I found myself writing worse code because of the warnings, as tagging functions with @warning_ignore("shadowed_variable") adds clutter that hurts readability a lot.

Now, the feature that tries to give a solution is obviously that I can configure my editor to just not show that warning. But unfortunately that does not help at all, since I want to publish my project to the asset library. The guidelines say that I should suppress all script warnings and I definitely agree. This basically forces a certain code style on me that I'm not happy with.

Maybe the editor should just not show warnings from assets, but I don't know if that is possible right now. Another possible workaround might be a more powerful annotation that suppresses warnings for whole files.

dalexeev commented 4 months ago

Now, the feature that tries to give a solution is obviously that I can configure my editor to just not show that warning. But unfortunately that does not help at all, since I want to publish my project to the asset library. The guidelines say that I should suppress all script warnings and I definitely agree.

Another possible workaround might be a more powerful annotation that suppresses warnings for whole files.