System for GDScript warnings

[vnen]

Depends on #19264, so commits are duplicated from there, only the last commit is relevant here. Was merged before this.

Closes #18271.

This PR adds a system to show warnings in the editor for things that may cause issues when running the script.

The status bar now show how many warnings there are in the current script:

If you click on the Warnings label, it shows a panel listing the individual warnings:

The Line number links to the line, so you can click and go straight to it. If you click the ignore link, it adds a comment to ignore the particular warning.

Warnings are also shown in the console log when the script is loaded:

There are new project settings under Debug -> Gdscript -> Warnings to disable warnings globally in the project. You can disable them per warning type or everything (unsafe line warnings are disabled by default):

You can also treat warnings as errors:

Three special comments can be added to the script to change the detected warnings:

• #warnings-disable: disable all warnings for the file.
• #warning-ignore-all:unused_variable: ignore all warnings of type unused_variable in the file.
• #warning-ignore:unused_variable: Ignore the next warning if it is of type unused_variable.

Currently defined warnings:

• Variable used but never assigned
• Variable never assigned but used in an assignment operation (+=, *=, etc)
• Variable is declared but never used
• Function argument is never used
• Code after a return statement (unreacheable code)
• Expression not assigned to a variable (standalone expression). Solves GDScript: writing only "array[i]" doesn't trigger an error #7302
• Function returns void but it's assigned to a variable
• Float value into an integer slot, precision is lost
• Typed assign of function call that yields (it may return a function state)
• Variable has the same name of a function (deprecation)
• Function has the same name of a variable (deprecation)
• Function has the same name of a constant (deprecation)
• Possible values of a ternary if are not mutually compatible
• Signal is defined but never emitted
• Function call returns something but the value isn't used
• Function not found, but there's a property with the same name (hint)
• Function not found, but there's a constant with the same name (hint)
• Property not found, but there's a function with the same name (hint)
• Integer division, decimals are discared
• Property not found in the detected type (but can be in subtypes) (unsafe line)
• Fucntion not found in the detected type (but can be in subtypes) (unsafe line)
• Cast used in an unknown type (unsafe line)
• Function call argument is of a supertype of the require argument (unsafe line)

[LikeLakers2]

Since you're putting these strings directly in code, *unreachable. Unless maybe this is a spelling that I'm unaware of?

[vnen]

Oh, I got myself fooled. I could swear this was the proper spelling. Thanks for the correction.

[Calinou]

This looks really good. 👍

I wonder if strings should be used for error codes instead of integers, so that #warning-ignore:3 would become something like #warning-ignore:unused_variable. This way, it's more readable and is less likely to break after engine updates.

[myhalibobo]

nice

[vnen]

@Calinou

I wonder if strings should be used for error codes instead of integers, so that #warning-ignore:3 would become something like #warning-ignore:unused_variable

Yeah, I think that would be better.

[mhilbrunner]

Nice work! I wonder if the "special" comments should have some sort of prefix. If we add further similar comments in the future, we'd only need to scan for that prefix, instead of checking for each possible meta comment type.

E.g. if we use #meta-warning-ignore:3 instead of #warning-ignore:3.
Then, in the future, we add a comment to... I don't know, disable some optimizations for a specific script: #meta-optlevel:0 (random example).

Now in the code to detect these, we only need to check for the meta prefix, instead of a giant if-else/switch for each possible comment we may add in the future.

(meta is a suggestion; others are gdscript or gd or pragma, whatever)

[mhilbrunner]

Also, is there a way to highlight lines with warnings? Either by coloring them like with the static typing, or by adding the typical "warning" icon at the beginning of that line, or underlining, or...

Because in large projects/files, I can see it becoming very overwhelming to use that bottom console log :)

[LikeLakers2]

If you don't mind me suggesting another special comment on the side, perhaps add one to enable Treat Warnings As Errors per-file? Maybe it could be #warnings-treat-as-errors (going by the naming convention you've used for the others) or simply #strict-mode.

[vnen]

@mhilbrunner

I wonder if the "special" comments should have some sort of prefix. If we add further similar comments in the future, we'd only need to scan for that prefix, instead of checking for each possible meta comment type.

Maybe. I'm not sure if much more stuff will be added to the system, but I guess it doesn't hurt to be future proof.

Also, is there a way to highlight lines with warnings? Either by coloring them like with the static typing, or by adding the typical "warning" icon at the beginning of that line, or underlining, or...

Many warnings are only "hints" and not necessarily that it is broken. It might be too much for beginners to have to deal with highlighted lines. I also feel that warnings are something you deal with later, not when writing the code (when writing you have many unused variables, since you didn't write the code to use them yet).

Maybe using a yellow tint in the line numbers, like the green tint for safe lines, which might be subtle enough to not scare the people who don't know about it, but I'm not really convinced yet.

Because in large projects/files, I can see it becoming very overwhelming to use that bottom console log :)

The bottom panel is per file, so it doesn't matter the size of the project. Might be long on large files but only if you already don't care enough about the warnings to remove them.

@LikeLakers2

If you don't mind me suggesting another special comment on the side, perhaps add one to enable Treat Warnings As Errors per-file?

I thought about this, but it is really useful to have this per file? In some files you care enough about warnings to treat them as errors but ignore the warnings in the rest of the project?

[LikeLakers2]

I thought about this, but it is really useful to have this per file? In some files you care enough about warnings to treat them as errors but ignore the warnings in the rest of the project?

I mean, maybe someone's making an addon and wants those addon scripts to have this strict mode, but not enforce it on the rest of the project. I don't know, I can't think of many uses, but chances are it'll be useful to someone to be able to enable it per-file.

[vnen]

I mean, maybe someone's making an addon and wants those addon scripts to have this strict mode, but not enforce it on the rest of the project. I don't know, I can't think of many uses, but chances are it'll be useful to someone to be able to enable it per-file.

Well, usually you have a dev project with this enabled when developing the addon, the user of the addon should not really care about the warnings the addon may cause.

It's not too hard to add it, but I'll refrain to do so until a real need arises.

[sdfgeoff]

I'd be keen to see this as an external linting tool as well, or perhaps some way to run it automatically on a file from command line flags. This would be useful for automated quality control (eg automatically running it when committing to VCS).

[Calinou]

I'd be keen to see this as an external linting tool as well, or perhaps some way to run it automatically on a file from command line flags.

The editor has a --check-only CLI flag right now:

  --check-only                     Only parse for errors and quit (use with --script).

However, based on the description, it does not operate on entire projects but on individual files instead. Therefore, a new flag such as --check would probably have to be added.

[vnen]

I'd be keen to see this as an external linting tool as well, or perhaps some way to run it automatically on a file from command line flags.

My idea is to eventually make a static analyzer that evaluates the whole project (#19811). This could be made to work from the command line. Currently there's no automatic way to check the whole project for errors.

[LikeLakers2]

I'm not sure if you've already done so, but why not expose this warning system to GDScript? It's already possible to set a script's source code, so perhaps this could be exposed as well to help devs who want to enable scripting within their games.

[vnen]

@LikeLakers2 what could be done is adding a function to return the list of warnings.

[MarioLiebisch]

Nice, but while at it, I'd like to see that panel expanded to also include some potential non-warning tags like #todo: implement this or #hack: ignore amount of mooney etc.

Also that first option Enable feels a bit odd. It's also somewhat confusing when you tick Enable and then disable some of the other warnings. How about renaming it to Show warnings? Or maybe inverse it as Don't show any warnings?

Also I'd move the individual opt-out options to their own group.

[aaronfranke]

These warnings are excessive. Some warnings are very unnecessary, such as unused delta in _process functions - not all logic loops need to use the delta since last loop! Please make an exception for _process(delta) etc to never display a warning about delta.

[chanon]

Elixir uses an underscore in front of parameters that are intended to not be used, so warnings aren't generated for them. Ex. in this case defining the method as

def _process(_delta):
    ....

Would tell the compiler/warning checker that _delta will not be used so it won't complain.

[neikeq]

@aaronfranke I think that warning should be disabled by default

[akien-mga]

Some warnings are very unnecessary, such as unused delta in _process functions - not all logic loops need to use the delta since last loop! Please make an exception for _process(delta) etc to never display a warning about delta.

Such warning makes sense for user-created functions, where an unused parameter probably means that it's either not needed in the first place, or that the function does not do what is expected. But indeed, there are quite a few cases where it's not relevant:

• Engine callbacks such as _process, _physics_process.
• Signal callbacks which come with a binding-defined parameter (e.g. animation_finished which has the name of the previous animation).
• Self-defined "virtual" methods which might have an argument used by some inherited classes but not all.

So all in all it's probably difficult to make this warning efficient, unless we add logic to identify the above cases.

[Calinou]

So all in all it's probably difficult to make this warning efficient, unless we add logic to identify the above cases.

What about disabling the unused parameter warning if the function name starts with an underscore? This seems to be the case for engine callbacks and most signal callbacks (the default names start with an underscore, but it can be changed by the user).

[vnen]

Warnings can always be disabled. We could do some defaults to disable some particular warnings, but then we need a way to add them back, because it might happen that you want the warning so you don't forget about it. For example, some signals that have parameters do expect you to use the parameter most of the time (e.g. body_entered), so disabling them might not be wanted in these cases.

It's quite hard to determine what the coder want or not.

What about disabling the unused parameter warning if the function name starts with an underscore?

It's a possibility but again: it needs a way to enable them back.

Elixir uses an underscore in front of parameters that are intended to not be used, so warnings aren't generated for them.

That might be the most sane and flexible solution, though it has to be very well documented to be useful.

[rcorre]

Elixir uses an underscore in front of parameters that are intended to not be used, so warnings aren't generated for them.

A leading _ is also respected by pylint, it was something I instinctively tried given the similarity of python to gdscript.

[OvermindDL1]

The underscore prefix being ignored for unused variable/binding warnings is not just a python/elixir thing but goes back to a lot of languages such as ML languages (SML/OCaml) to Haskell to Erlang to Prolog to even some C/C++ compilers have a flag to let those be ignored. It is exceedingly common.