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

Show a linter warning when accessing a getter/setter _without_ self #981

Closed
sszigeti opened this issue May 31, 2020 · 3 comments
Closed

Comments

@sszigeti
Copy link

sszigeti commented May 31, 2020

Describe the project you are working on: The scenario is generic GDScript coding using setters and getters.

Describe the problem or limitation you are having in your project: One of my frequent "duh!" mistakes is forgetting to use self., which is especially annoying when I'm using the setter/getter as a proxy (a.k.a. computed property), and the regular variable just thinks it's null, while self.variable would of course give me what I really meant to access.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Although I'd prefer an "opt-out" approach when using variables with setters/getters in their home classes (i.e. without self. I'd use the setter/getter, and by explicity using self. I'd use the local variable, circumventing its setter/getter), I realize it's too late for this change, so the next best thing would be to show a linter warning when a variable with a setter/getter is used without self. in its home class. This warning could be suppressed with a usual #warning-ignore:strict-setget comment.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: This proposal only affects the GDScript editor, while working on the code of a class that has variables with setters/getters, and in that class any of these variables are accessed without self. outside of their accessor functions. Any warning, generated by not using self. in such a case would highlight the actual behavior, not what the programmer might have assumed to happen.

A sample, problematic code:

extends Node
var some_proxied_data setget ,get_some_proxied_data 
func get_some_proxied_data():
    return SingletonDataStore.some_data
func print_my_proxied_data():
    print("some proxied data: %s" % some_proxied_data)

The above code would most likely print null, instead of the actual value of the proxied variable in the getter. Accessing the some_proxied_data variable from outside of this class always triggers the getter, so the programmer could think it's working the same way inside its own home class.

With this proposal the Godot editor would mark the print(... % some_proxied_data) statement with a warning, suggesting the usage of self. there for actually accessing the getter, leading to a more consistent behavior.

If this enhancement will not be used often, can it be worked around with a few lines of script?: No, this is an enhancement to the editor's linter.

Is there a reason why this should be core and not an add-on in the asset library?: This isn't a breaking change and it can make developers be more aware of how getters and setters work.

@nobuyukinyuu
Copy link

Does using self actually call the setter/getter as opposed to directly accessing a script's instance variable? That's interesting; I always figured you had to call the set/get funcs directly to update the property the way you would from an external source. If that's not an explicitly documented feature, I think it probably should be!

Similarly, a warning to clarify usage of a "shadowed" variable could be helpful, although I feel like it would be better to be an opt-in feature rather than opt-out because of how annoying it could end up being to people who know what they're doing.

@sszigeti
Copy link
Author

sszigeti commented May 31, 2020

@nobuyukinyuu Actually, self behavior is (now) documented please scroll down a bit to "As said, local access will not trigger the setter and getter. Here is an illustration of this", and see the sample code.

Regarding the warning for shadowed variables, JetBrains Rider has some truly excellent helper messages, some of them even have their own web page with code samples. The trick is that someone has to write these messages, and it can be quite challenging to explain enough without being too verbose. :)

@Calinou
Copy link
Member

Calinou commented Sep 13, 2020

This proposal has been made obsolete by how setters and getters work in the new GDScript, closing.

The previous setget syntax was removed in favor of properties. It is meant to be more tied to the variable declaration and avoid you having to create dedicated functions (though you still can if you prefer). Another change is that, unlike setget properties always call their setter/getter even inside the same class. This gives users a consistent behavior which won't cause confusion or lead to potential mistakes during a refactor.

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

3 participants