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

Add an optional untyped_declaration warning #81355

Merged

Conversation

ryanabx
Copy link
Contributor

@ryanabx ryanabx commented Sep 5, 2023

Addresses (and may close) godotengine/godot-proposals#173

Adds a new property to ProjectSettings: debug/gdscript/warnings/untyped_declaration, which will print a warning or error if there are variables, parameters, or constants that aren't explicitly typed using : <type> or during initialization, :=.

var foo: int = 0 # Good
var bar := 0 # Also good
var baz = 0 # Warning will be thrown

A warning will also be thrown if a function does not have an explicit return type. This does not affect _init() as it shouldn't have a return type other than void anyways, and is always implicitly void.

func foo() -> int: # Good
    return 0

func bar(): # Warning will be thrown
    return 1

Feel free to test!

Bugsquad edit: Outdated information removed.

@ryanabx ryanabx force-pushed the features/enforce-static-typing branch from 2af43ab to 074831e Compare September 5, 2023 19:27
@ryanabx ryanabx marked this pull request as draft September 5, 2023 19:27
@ryanabx ryanabx force-pushed the features/enforce-static-typing branch from 074831e to bdcc2b9 Compare September 5, 2023 19:29
@ryanabx ryanabx marked this pull request as ready for review September 5, 2023 19:30
@AThousandShips
Copy link
Member

(also the failure of the "Test Godot Project" isn't related to this PR but a random error)

@ryanabx ryanabx force-pushed the features/enforce-static-typing branch from bdcc2b9 to 0277011 Compare September 5, 2023 19:44
modules/gdscript/gdscript_warning.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_warning.h Outdated Show resolved Hide resolved
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

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

My requests were done, but someone from the GDScript team should do the proper review.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Comment and code style and phrasings look good to me, haven't tested the code specifically

@ryanabx ryanabx force-pushed the features/enforce-static-typing branch 2 times, most recently from bf09cab to ec188d7 Compare September 5, 2023 21:43
@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 5, 2023

Went ahead and tested this with my own project. Everything appears to work as expected! 🥳

EDIT: I need to investigate an issue with lambdas

@ryanabx ryanabx force-pushed the features/enforce-static-typing branch from ec188d7 to 9f2bb09 Compare September 6, 2023 01:09
@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 6, 2023

Issue with lambdas is fixed! I think it would now be ready for review!

@anvilfolk
Copy link
Contributor

Don't currently have time for a more thorough review, but since I didn't notice any explicit mention of class members, I assume this already works with them, because their declaration might work as an assignable? :)

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 6, 2023

Don't currently have time for a more thorough review, but since I didn't notice any explicit mention of class members, I assume this already works with them, because their declaration might work as an assignable? :)

image

Class member variables and functions work!

@AThousandShips
Copy link
Member

I'm not sure return values fall under "static typing" and warrants a separate error setting

@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 6, 2023

image

Our static typing page claims they are. (Also our "automatic static typing" setting adds return types indiscriminately)

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Resolved

The bug with setter should be fixed this way:

if (member.variable->getter != nullptr) {

 if (member.variable->getter != nullptr) {
-    member.variable->getter->set_datatype(member.variable->datatype);
+    member.variable->getter->return_type = member.variable->datatype_specifier;
+    member.variable->getter->set_datatype(member.get_datatype());
 
     resolve_function_body(member.variable->getter);
 }
 if (member.variable->setter != nullptr) {
-    resolve_function_signature(member.variable->setter);
-
-    if (member.variable->setter->parameters.size() > 0) {
-        member.variable->setter->parameters[0]->datatype_specifier = member.variable->datatype_specifier;
-        member.variable->setter->parameters[0]->set_datatype(member.get_datatype());
-    }
+    ERR_CONTINUE_MSG(member.variable->setter->parameters.is_empty(), "Parser bug: Setter must have an argument.");
+    member.variable->setter->parameters[0]->datatype_specifier = member.variable->datatype_specifier;
+    member.variable->setter->parameters[0]->set_datatype(member.get_datatype());
 
     resolve_function_body(member.variable->setter);
 }

Also note that you can optionally specify a type for the for loop variable (see #80247). This is a bit different from the rest of the cases, so please don't do anything about it just yet. I will think about it and write my opinion later.

modules/gdscript/gdscript_warning.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_warning.h Outdated Show resolved Hide resolved
modules/gdscript/tests/gdscript_test_runner.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_warning.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 8, 2023

@AThousandShips Since this PR is ready and approved, would it be possible to update the milestone to 4.2?

@AThousandShips AThousandShips modified the milestones: 4.x, 4.2 Sep 9, 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.

Discussed during the GDScript Team meeting. Looks good! Thanks for the PR, @ryanabx!

@akien-mga
Copy link
Member

Needs a rebase.

@ryanabx ryanabx force-pushed the features/enforce-static-typing branch from e69ff06 to 14edf04 Compare September 11, 2023 14:39
@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 11, 2023

Needs a rebase.

Done!

@akien-mga akien-mga changed the title Add an optional "untyped_declaration" warning Add an optional untyped_declaration warning Sep 11, 2023
@akien-mga akien-mga merged commit 0545ed5 into godotengine:master Sep 12, 2023
14 of 15 checks passed
@akien-mga
Copy link
Member

Thanks!

@sandygk
Copy link
Contributor

sandygk commented Oct 31, 2023

I am using 4.2 beta 3 and for some reason I get warnings when I don't specify the -> void return type, but in the PR description it says it's not required.

This does not affect _init() as it shouldn't have a return type other than void anyways, and is always implicitly void.

Did this changed at some point?

@ryanabx
Copy link
Contributor Author

ryanabx commented Oct 31, 2023

I am using 4.2 beta 3 and for some reason I get warnings when I don't specify the -> void return type, but in the PR description it says it's not required.

This does not affect _init() as it shouldn't have a return type other than void anyways, and is always implicitly void.

Did this changed at some point?

Init doesn't have a return type, and I think it actually warns you if you do use one, not sure.

For any other method, a warning is issued if there's no return type specified

@dalexeev
Copy link
Member

dalexeev commented Nov 1, 2023

This does not affect _init() as it shouldn't have a return type other than void anyways, and is always implicitly void.

Did this changed at some point?

If I remember correctly, this was originally implemented incorrectly. I've updated the first post.

I don't think we should make an exception for _init() and _static_init(), since explicit void is allowed for them (unlike some other languages) and is generated by autocompletion (if you enabled the text_editor/completion/add_type_hints editor setting). For _ready() you can also specify only void or no type at all, but the warning is generated.

@sandygk
Copy link
Contributor

sandygk commented Nov 2, 2023

What I'd suggest is to make -> void optional. So if it's not added, and nothing is returned, the function is void. It keeps the code shorter, cleaner, and requires less typing. The same way we don't do f(void) if there are no arguments, (like we used to do in C). If there are no arguments, we know it by the fact there aren't any, we don't have to explicitly say it. Well, the same applies to the return type. If we don't return anything in the function, then it's void. If we return something, then it isn't. And the compiler can check that.

And autocompletion helps, but only when implementing already defined functions that it can suggest. For new functions it won't know that you want it to be void, so it's unnecessary extra typing if you want your code to be strongly typed.

For instance TypeScript works the way I suggest. In the following code it will complain if I don't type the argument a, but it doesn't complain if the function is not declared as void:

function f(a: string) {
    console.log(a)
}

if we still see value in forcing the developer to explicitly type -> void, then we could at least make it an option to allow implicit void return type

@sandygk
Copy link
Contributor

sandygk commented Nov 8, 2023

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.

9 participants