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

GDScript: Add INFERRED_DECLARATION warning #82139

Merged

Conversation

dalexeev
Copy link
Member

Note A. This warning is in addition to UNTYPED_DECLARATION, rather than including to. In my opinion, this makes the most sense because if the user enables all warnings, then they won't end up with a situation where one declaration generates both warnings.

var a = 1 # Warning: UNTYPED_DECLARATION.
var b := 2 # Warning: INFERRED_DECLARATION.
var c: int = 3 # No warning.

So the user has a choice:

  1. Don't enable warnings.
  2. Enable only UNTYPED_DECLARATION, use static types (explicitly or implicitly), gaining the benefits of static checks.
  3. Enable both warnings. More readable, but more verbose. Like C++ without the auto keyword.

It's technically possible to only enable INFERRED_DECLARATION, but I don't see much point in doing so.

Note B. This is a bit against the style we recommend, since a warning is generated on any type inference. It doesn't matter whether the initializer contains a type name or not. The warning requires that you specify the type explicitly:

var vector := Vector2() # Warning.
var node := Node.new() # Warning.
var control: Control = Control.new() # No warning.

While this is more verbose and we can implement this check, I don't think we should do it. This verbosity gives additional reliability.

For example, if you have var x := Type.new(), you could forget about the type and accidentally change it when you change the value to SubType.new(). Or copy-paste Vector2i(128, 96) in place of Vector2(64, 48) and change the type from Vector2 to Vector2i without noticing it due to implicit conversions.

Note С. I'm not sure about the name, maybe INFERRED_TYPE_DECLARATION is better (instead of INFERRED_DECLARATION)?

CC @ryanabx

Copy link
Contributor

@ryanabx ryanabx left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @dalexeev !

My only thoughts are that it would be odd if a user enabled INFERRED_DECLARATION but not UNTYPED_DECLARATION. You would get a warning if you infer type, but not if the type is undeclared and not inferred. Maybe we could put some kind of example somewhere in the documentation about these two warnings, so users know what to expect?

Great changes though! I approve 👍

modules/gdscript/gdscript_warning.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member Author

Maybe we could put some kind of example somewhere in the documentation about these two warnings, so users know what to expect?

Here:

[b]Note:[/b] This warning is recommended [b]in addition[/b] to [member debug/gdscript/warnings/untyped_declaration] if you want to always specify the type explicitly.

@ryanabx
Copy link
Contributor

ryanabx commented Sep 22, 2023

Maybe we could put some kind of example somewhere in the documentation about these two warnings, so users know what to expect?

Here:

[b]Note:[/b] This warning is recommended [b]in addition[/b] to [member debug/gdscript/warnings/untyped_declaration] if you want to always specify the type explicitly.

It wasn't a strong enough warning in my opinion. I think it might be better to strongly recommend that the warning only be turned on if debug/gdscript/warnings/untyped_declaration is enabled.

@dalexeev dalexeev force-pushed the gds-add-inferred-declaration-warning branch from cc33fd1 to 4ce2730 Compare September 22, 2023 20:14
@ryanabx
Copy link
Contributor

ryanabx commented Sep 23, 2023

Changes look good! Thanks again!

@adamscott adamscott modified the milestones: 4.x, 4.2 Sep 26, 2023
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Approved for 4.2 by the GDScript team during our meeting.

@akien-mga akien-mga merged commit 9b0b441 into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Add a opt-in GDScript warning when type inference is not used where it could be (and vice versa)
4 participants