Godot-proposals: Different syntax & semantics for warning exemptions

[snoopdouglas]

Describe the project you are working on:

A space shooter.

Describe the problem or limitation you are having in your project:

I'm an old GCC blowhard, and so want to treat warnings as errors. I've figured out how to use #warning-ignore:, but it hasn't been easy.

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

• Given that the exemptions can directly affect the course of execution (in the case of 'treat warnings as errors'), I think they shouldn't be comments.
• A couple of other syntax changes, as described below; everything here is to help make the warning exemptions clearer.
• It is very important that the script editor provides autocompletion for these. We don't have that right now.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

@ is a common character to use for decorators, and I think it'd apply nicely here.

Additionally, as it stands, the first half of the expression is kebab-cased while the second is snake_cased. I understand how this might've come about, but still think it should change. There isn't anything else in GDScript, to my knowledge, that uses kebab-casing.

Here's an example:

# ignore all unused local variables:
@warning_ignore_all unused_variable

# ignore all unused class variables on the next line:
@warning_ignore unused_class_variable
var v: int

func f(x: int) -> int:
  # ignore all integer division on the next line:
  @warning_ignore integer_division
  return (x / 4) + (x / 3)

Note the change in semantics for the last example: as it stands, multiple #warning-ignore lines need to precede a line which breaks the rules multiple times.

And to replace #warnings-disable:

# disable all warnings for this entire file
@warnings_disable

However: the pluralisation warnings here annoys me a bit, as does the similarity in semantics between the words ignore and disable. @warning_ignore_all has no plural, but it is plural in nature. Maybe something for discussion.

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

No.

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

Addons can't change GDScript semantics (to my knowledge), and IMHO they shouldn't.

---

CC: godotengine/godot-docs#3447

[Xrayez]

• Given that the exemptions can directly affect the course of execution (in the case of 'treat warnings as errors'), I think they shouldn't be comments.

@ is a common character to use for decorators, and I think it'd apply nicely here.

Might be already on the roadmap, not sure: godotengine/godot#20318.

Additionally, as it stands, the first half of the expression is kebab-cased while the second is snake_cased. I understand how this might've come about, but still think it should change. There isn't _anything_ else in GDScript, to my knowledge, that uses kebab-casing.

Some marginally related proposal to help with string case conversions: #719.

[Calinou]

While we're at it, we should probably remove the "catch-all" #warning-ignore comment (except for entire files, like in Flake8). Such ignores are generally considered bad practice as they can hide important issues that could lead to bugs in the code.

Instead, users should always specify the warning they're ignoring, which makes it safer and more specific (you can read the code and know the warning that was ignored immediately).

[snoopdouglas]

@calinou Yeah, that sounds like a really good idea to me. In that case though, we'll also need to be able to specify multiple ignores for the following line, eg.

@warning_ignore unused_variable integer_division
var i: int = 3 / 4

This also throws up the question of whether to completely ditch #warnings-disable.

[Xrayez]

This also throws up the question of whether to completely ditch #warnings-disable.

Well if a developer starts to ignore warnings like that it might signify that the warning system may actually be not so useful (for now), and it's only natural because you can't really come up with a system which could satisfy most users.

For instance, if I were to treat all warnings as errors, some warnings like return_value_discarded would make the code quite difficult to read. For real-life examples, see wesnoth/haldric project with the snippet in

https://github.com/wesnoth/haldric/blob/2e70c303996f69a22f3a845f2168d33cc90638c7/source/global/Network.gd#L23-L34.

func _ready() -> void:
    set_pause_mode(PAUSE_MODE_PROCESS)
    # warning-ignore:return_value_discarded
    get_tree().connect("network_peer_connected",self,"_network_peer_connected")
    # warning-ignore:return_value_discarded
    get_tree().connect("network_peer_disconnected",self,"_network_peer_disconnected")
    # warning-ignore:return_value_discarded
    get_tree().connect("connected_to_server", self, "_connected_ok")
    # warning-ignore:return_value_discarded
    get_tree().connect("connection_failed", self, "_connection_failed")
    # warning-ignore:return_value_discarded
    get_tree().connect("server_disconnected",self,"_server_disconnected")

Quite difficult to parse (visually), and that's one of the most used functions in Godot IMO.

To compare, there's a special implicit discard syntax which I personally like better than the above (the underscore _ thingy):

func _ready():
    var _err = OK
    _err = get_tree().connect("network_peer_connected",self,"_network_peer_connected")
    _err = get_tree().connect("network_peer_disconnected",self,"_network_peer_disconnected")
    _err = get_tree().connect("connected_to_server", self, "_connected_ok")
    _err = get_tree().connect("connection_failed", self, "_connection_failed")
    _err = get_tree().connect("server_disconnected",self,"_server_disconnected")

That's just to convey an idea.

[Calinou]

@Xrayez Making Object.connect() return void should solve this particular issue, but it's a breaking change.

[snoopdouglas]

A more Haskell-esque _ = some_function(), or perhaps a discard keyword, would be a great addition if you ask me. I think I saw this suggested in some other GIPs. (I'm not at my desk right now, might be able to look for them later)

Edit: huh, can't seem to find them. Sorry if I dreamt that!

[Calinou]

@snoopdouglas This could be done once _ is considered as a valid identifier in GDScript (right now, only __ works). We can then have a special case in the GDScript compiler to avoid copying anything when assigning to the variable _.

This could also be done by adding a discard keyword, but this means we'd have to add yet another keyword, which comes with its downsides (like having to update third-party syntax highlighters and the like).

[snoopdouglas]

Hmm, I'd personally prefer _ so long as it didn't need to be declared wherever I needed it. Agreed that less keywords is a good thing.

[Jummit]

I don't get assigning variables just to prevent a warning. I always disable the "Discarded return value" warning and I think it should be disabled by default, as it's almost never actually a problem.

[snoopdouglas]

I don't get assigning variables just to prevent a warning.

Absolutely. But - in my opinion - that's a lesser problem than my program ending up in an undefined state because I forgot to act upon a return value somewhere, so it's good that a warning exists for this, regardless of its default enablement.

Keeping this on-topic though, those defaults aren't what I intended us to be discussing here.

[vnen]

I do intend to move those to annotations (#828). Only difference is that parentheses and commas are required, like a function call. It will also be possible to implement completion for it.

A more Haskell-esque _ = some_function(), or perhaps a discard keyword, would be a great addition if you ask me.

This could be done once _ is considered as a valid identifier in GDScript (right now, only __ works). We can then have a special case in the GDScript compiler to avoid copying anything when assigning to the variable _.

It's the opposite IMO. _ is not a valid identifier, which would be perfect for this case since you can't use this as a variable anyway, so it doesn't even need to be declared.