Skip to content
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

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

Open
L4Vo5 opened this issue May 17, 2023 · 4 comments

Comments

@L4Vo5
Copy link

L4Vo5 commented May 17, 2023

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.

@L4Vo5 L4Vo5 changed the title Make the script editor aware of static functions Make the script editor more aware of static functions' limited scope May 17, 2023
@dalexeev
Copy link
Member

dalexeev commented May 17, 2023

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 L4Vo5 changed the title Make the script editor more aware of static functions' limited scope Make the script editor not emit "shadowing" warnings when reusing non-static names in static functions May 17, 2023
@L4Vo5
Copy link
Author

L4Vo5 commented May 17, 2023

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

@leifb
Copy link

leifb commented Jul 7, 2024

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
Copy link
Member

dalexeev commented Jul 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants