Skip to content

Add GDScript warning pages #10635

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions tutorials/scripting/gdscript/gdscript_basics.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,9 @@ Member variables are initialized in the following order:
To fix this, move the ``_data`` variable definition above the ``a`` definition
or remove the empty dictionary assignment (``= {}``).


.. _doc_gdscript_basics_static_variables:

Static variables
~~~~~~~~~~~~~~~~

Expand Down Expand Up @@ -1557,6 +1560,9 @@ Lambda functions capture the local environment:
lambda.call()
print(a) # Prints `[1]`.


.. _doc_gdscript_basics_static_functions:

Static functions
~~~~~~~~~~~~~~~~

Expand Down
1 change: 1 addition & 0 deletions tutorials/scripting/gdscript/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ GDScript
gdscript_styleguide
static_typing
warning_system
warnings/index
gdscript_format_string

.. seealso::
Expand Down
47 changes: 47 additions & 0 deletions tutorials/scripting/gdscript/warnings/assert_always_false.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
ASSERT_ALWAYS_FALSE
=======================

The warning message is:

.. code-block:: none

Assert statement will raise an error because the expression is always false.

The default warning level for this warning is **Warn**.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be something in Sphinx's restructuredtext that may avoid having to copy and paste this every time. Putting a pin on that.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#substitutions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I can define a series of substitutions like

.. |default_is_ignore| replace:: The default warning level for this warning is **Ignore**.
.. |default_is_warn| replace:: The default warning level for this warning is **Warn**.
.. |default_is_error| replace:: The default warning level for this warning is **Error**.

and then use them (like |default_is_warn|) to paste that message in. Automating the ProjectSettings links might take a bit more work, since it involves changing text within the reference link. I would really love to figure out a way to automate this well, since as you pointed out so much of the text is the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm mistaken I don't think this would work across multiple pages so there may still be a need to merge everything into one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I wouldn't let this narrow issue of substitutions determine if we make one page or multiple pages for these warnings. (FWIW a page for each warning seems natural to me, and there is prededent with the C# diagnostics, but this is a larger decision and it seems like other reviewers are leaning towards a single page)

  • It's likely not worth the effort to automate the class reference links, since that would involve dynamic replacement and IIRC that isn't straightforward with RST substitutions.

  • You can make a substitution accessible from all pages rst_epilog, we already use it at least once:

    godot-docs/conf.py

    Lines 289 to 298 in 6ae63dd

    rst_epilog = """
    .. |weblate_widget| image:: https://hosted.weblate.org/widgets/godot-engine/{image_locale}/godot-docs/287x66-white.png
    :alt: Translation status
    :target: https://hosted.weblate.org/engage/godot-engine{target_locale}/?utm_source=widget
    :width: 287
    :height: 66
    """.format(
    image_locale="-" if language == "en" else language,
    target_locale="" if language == "en" else "/" + language,
    )

However, we don't use that for cases like the upgrading pages or the class reference, where each page redefines the same substitions:

.. || replace:: :abbr:` (This API breaks compatibility.)`
.. |✔️| replace:: :abbr:`✔️ (This API does not break compatibility.)`
.. |✔️ with compat| replace:: :abbr:`✔️ (This API does not break compatibility. A compatibility method was added.)`

.. |virtual| replace:: :abbr:`virtual (This method should typically be overridden by the user to have any effect.)`
.. |const| replace:: :abbr:`const (This method has no side effects. It doesn't modify any of the instance's member variables.)`
.. |vararg| replace:: :abbr:`vararg (This method accepts any number of arguments after the ones described here.)`
.. |constructor| replace:: :abbr:`constructor (This method is used to construct a type.)`
.. |static| replace:: :abbr:`static (This method doesn't need an instance to be called, so it can be called directly using the class name.)`
.. |operator| replace:: :abbr:`operator (This method describes a valid operator to use with this type as left-hand operand.)`
.. |bitfield| replace:: :abbr:`BitField (This value is an integer composed as a bitmask of the following flags.)`
.. |void| replace:: :abbr:`void (No return value.)`

There might be a compile time increase when using rst_epilog, which is why it's not used more. I haven't looked closely at this, though.

There's also .. include (docs) which could be used to only include the substitutions on the pages that use it (and therefore reduce compile times if using rst_epilog is too heavy). We don't currently use .. include, but it's usage looks relatively straightforward.

(I still have to do a deeper review of the rest of this later)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an RST file and .. include is how I implemented the test on my end.

warning_page_substitutions.rst:

.. |default_is_ignore| replace:: The default warning level for this warning is **Ignore**.
.. |default_is_warn| replace:: The default warning level for this warning is **Warn**.
.. |default_is_error| replace:: The default warning level for this warning is **Error**.

assert_always_false.rst:

ASSERT_ALWAYS_FALSE
=======================

The warning message is:

.. code-block:: none

    Assert statement will raise an error because the expression is always false.

|default_is_warn|
To modify it, see :ref:`ProjectSettings.debug/gdscript/warnings/assert_always_false<class_ProjectSettings_property_debug/gdscript/warnings/assert_always_false>`.

.. include:: warning_page_substitutions.rst

With this, we'd only have to edit or translate the sentences once in warning_page_substitutions.rst and they'd apply everywhere, which is nice. Dynamic substitution would be very helpful for this, but as you said it sounds like it would be quite involved.

To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Assert Always False<class_ProjectSettings_property_debug/gdscript/warnings/assert_always_false>`.

When this warning occurs
------------------------

The :ref:`assert() <class_@GDScript_method_assert>` keyword can be used to ensure that a given condition is met before allowing code execution to continue. If the first argument passed to it is true in a boolean context, the rest of the function will run as expected; if it is false in a boolean context, then the project will stop.

If ``assert()`` is passed something guaranteed to be false in a boolean context, then the ``assert()`` call will always stop the project.

.. code-block::

# Zero always evaluates to false.
assert(0, "Zero is false in a boolean context")

# Likewise, an empty string always evaluates to false.
assert("", "An empty string is false in a boolean context")

.. note::

Godot will *not* raise this warning if a literal false boolean is passed:

.. code-block::

# Despite false being passed, this won't raise ASSERT_ALWAYS_FALSE.
assert(false, "False is false")

This is because ``assert(false)`` calls are often used in development to forcibly halt program execution and avoid strange errors later on.

See `GH-58087 <https://github.com/godotengine/godot/issues/58087>`_ for more information.

How to fix this warning
-----------------------

Assuming you want code following the ``assert()`` to run, remove it from your code. If you do want code execution to stop at that point, replace the condition with ``false``, or :ref:`consider using breakpoints instead <doc_debugger_tools_and_options>`.



36 changes: 36 additions & 0 deletions tutorials/scripting/gdscript/warnings/assert_always_true.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
ASSERT_ALWAYS_TRUE
======================

The warning message is:

.. code-block:: none

Assert statement is redundant because the expression is always true.

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Assert Always True<class_ProjectSettings_property_debug/gdscript/warnings/assert_always_true>`.

When this warning occurs
------------------------

The :ref:`assert() <class_@GDScript_method_assert>` keyword can be used to ensure that a given condition is met before allowing code execution to continue. If the first argument passed to it evaluates to ``true``, the rest of the function will run as expected; if it is ``false``, then the project will stop.

If ``assert()`` is passed an expression that is guaranteed to be ``true``, then the ``assert()`` call will never stop the project, thus making it redundant.

.. code-block::

# The boolean true will always be true, so this assert will never stop
# the program.
assert(true, "True is false, somehow?")

# Likewise, 3 will never be equal to 4, so this assert will never stop
# the program.
assert(3 != 4, "3 is equal to 4")

How to fix this warning
-----------------------

Remove the ``assert()`` statement from your code.



Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
CONFUSABLE_CAPTURE_REASSIGNMENT
===================================

The warning message is:

.. code-block:: none

Reassigning lambda capture does not modify the outer local variable "my_var".

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Confusable Capture Reassignment<class_ProjectSettings_property_debug/gdscript/warnings/confusable_capture_reassignment>`.

When this warning occurs
------------------------

TODO


How to fix this warning
-----------------------

TODO



31 changes: 31 additions & 0 deletions tutorials/scripting/gdscript/warnings/confusable_identifier.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
CONFUSABLE_IDENTIFIER
=========================

The warning message is:

.. code-block:: none

The identifier "my_vаr" has misleading characters and might be confused with something else.

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Confusable Identifier<class_ProjectSettings_property_debug/gdscript/warnings/confusable_identifier>`.

When this warning occurs
------------------------

Some scripts such as Cyrillic have characters that look like Latin (such as English, Spanish, etc.) characters, but are actually different.

.. code-block::

var engine_nаme = "Godot"
print(engine_name)

In this code snippet, the ``print`` statement would fail, because ``engine_name`` is actually not defined. The identifier in the ``print`` statement uses the Latin character "a" (U+0061), while the identifier in the variable declaration above uses the Cyrillic character "а" (U+0430).
Comment on lines +20 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooo, this is nasty, is there any other letter that may be slightly more distinguishable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can double-check later, but I recall going into the code to try and find the "look-alike" table that Godot may have been using to detect these, and was unable to find it (perhaps it was being processed by a third-party library?). The letters being indistinguishable might be helpful here, though. They look identical to each other, yet aren't the same, which is the point of the warning.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this page that visualizes the Unicode confusable characters: https://util.unicode.org/UnicodeJsps/confusables.jsp

For this example, I could replace the Cyrillic character with ɑ (lowercase alpha), like so:

var engine_nɑme = "Godot"
print(engine_name)

There are a few other characters (α, ⍺, 𝐚, 𝑎, etc) that could also be used instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this alternative. It's subtle but at least noticeable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should make the intentionally confusable identifier less confusing, even for demonstration in documentation. The point is that some Unicode characters are visually indistinguishable from each other in most fonts. If someone doesn't know this, I think it's better for them to learn it here, rather than expecting characters to be similar but still slightly different.

Or we can clarify it separately, something like Moreover, some characters may have no visual differences at all in most fonts, like Latin "a" and Cyrillic "а"..


How to fix this warning
-----------------------

Avoid using Cyrillic or other scripts' characters that are visually similar to Latin ones. A good rule of thumb is to prefer the Latin alphabet for program identifiers.



Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
CONFUSABLE_LOCAL_DECLARATION
================================

The warning message is:

.. code-block:: none

The variable "my_param" is declared below in the parent block.

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Confusable Local Declaration<class_ProjectSettings_property_debug/gdscript/warnings/confusable_local_declaration>`.

When this warning occurs
------------------------

TODO


How to fix this warning
-----------------------

TODO



25 changes: 25 additions & 0 deletions tutorials/scripting/gdscript/warnings/confusable_local_usage.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
CONFUSABLE_LOCAL_USAGE
==========================

The warning message is:

.. code-block:: none

The identifier "my_var" will be shadowed below in the block.

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Confusable Local Usage<class_ProjectSettings_property_debug/gdscript/warnings/confusable_local_usage>`.

When this warning occurs
------------------------

TODO


How to fix this warning
-----------------------

TODO



25 changes: 25 additions & 0 deletions tutorials/scripting/gdscript/warnings/deprecated_keyword.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
DEPRECATED_KEYWORD
======================

The warning message is:

.. code-block:: none

The "..." keyword is deprecated and will be removed in a future release. Please replace it with "...".

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Deprecated Keyword<class_ProjectSettings_property_debug/gdscript/warnings/deprecated_keyword>`.

When this warning occurs
------------------------

There are currently no deprecated keywords in GDScript; as such, this warning should never appear.


How to fix this warning
-----------------------

Follow instructions on the Godot Docs for how to use the alternative keyword.



24 changes: 24 additions & 0 deletions tutorials/scripting/gdscript/warnings/empty_file.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
EMPTY_FILE
==============

The warning message is:

.. code-block:: none

Empty script file.

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Empty File<class_ProjectSettings_property_debug/gdscript/warnings/empty_file>`.

When this warning occurs
------------------------

This warning may appear when you create a ``.gd`` file with no code to run. A completely blank file will raise this warning, as will a file that only contains comments.

How to fix this warning
-----------------------

Add code to the ``.gd`` file or delete it.



Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
ENUM_VARIABLE_WITHOUT_DEFAULT
=================================

The warning message is:

.. code-block:: none

The variable "my_var" has an enum type and does not set an explicit default value. The default will be set to "0".

The default warning level for this warning is **Warn**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Enum Variable Without Default<class_ProjectSettings_property_debug/gdscript/warnings/enum_variable_without_default>`.

When this warning occurs
------------------------

This warning may appear when declaring a variable whose type is an enum with values assigned to its members, but the variable is not assigned a value.

.. code-block::

enum MyEnum {
A = 1,
B = 2,
C = 3
}

func _ready():
var my_var: MyEnum # Will give warning ENUM_VARIABLE_WITHOUT_DEFAULT.

Godot will usually default an enum-typed variable to the integer ``0``. However, if the enum does not have a member that corresponds to ``0``, Godot will be confused on how to assign it.

How to fix this warning
-----------------------

Provide the variable with a default value, like so:

.. code-block::

var my_var: MyEnum = MyEnum.A

Alternatively, if the enum has a member with a value of 0, Godot will use that as the default value.

.. code-block::

enum MyEnum {
Z = 0, # Will be used as the default value.
A = 1,
B = 2,
C = 3
}

func _ready():
var my_var: MyEnum # Will default to MyEnum.Z.



Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
GET_NODE_DEFAULT_WITHOUT_ONREADY
====================================

This warning appears when the default value of a node's property is set to a location in the scene tree without using the ``@onready`` annotation.

Depending on how you attempt to access the scene tree, the warning message will be one of the following:

.. code-block:: none

The default value uses "$" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.

The default value uses "%" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.

The default value uses "get_node()" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.

The default warning level for this warning is **Error**.
To modify it, set :ref:`Project Settings > Debug > GDScript > Warnings > Get Node Default Without Onready<class_ProjectSettings_property_debug/gdscript/warnings/get_node_default_without_onready>`.

When this warning occurs
------------------------

Instance properties can be set by declaring them outside of a method. Additionally, they can be given a default value using ``=``::

.. code-block::

extends Area2D

var my_num = 3

The property ``my_num`` will always start out with a value of ``3`` in each instance of this class.
GDScript also has methods to retrieve specific nodes from the scene tree: namely the :ref:`get_node() <class_Node_method_get_node>` method, and its shorthand versions ``$`` (and ``%`` for unique nodes). Thus, if you want to have a property's default value be a particular child node, it may be tempting to write something like the following::

.. code-block::

extends Area2D

var my_collision_shape = $CollisionShape2D

However, properties' default values are evaluated and assigned before the scene tree is set up. This means that at the time of assigning the default value, the node may not be in the scene tree, and the variable will be set to ``null`` instead.

How to fix this warning
-----------------------

The most straightforward solution is to add the ``@onready`` annotation before your property declaration::

.. code-block::

extends Area2D

@onready var my_collision_shape = $CollisionShape2D

Now, the default value of the property will not be assigned until the scene tree has been initialized, and the node will be present.

Alternatively, you can set the value of the property at the beginning of your ``_ready()`` method::

.. code-block::

extends Area2D

var my_collision_shape

func _ready():
my_collision_shape = $CollisionShape2D
Loading
Loading