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 static typing #650

Open
Cykyrios opened this issue Jul 29, 2024 · 6 comments
Open

Add static typing #650

Cykyrios opened this issue Jul 29, 2024 · 6 comments

Comments

@Cykyrios
Copy link

Versions

4.x (currently 4.3 rc1 and GUT 9.3.0)

The Feature

I would like to see static typing added throughout GUT files, as this would drastically decrease the number of warnings generated. I know the documentation recommends disabling warnings for addons, but I can't do this as I'm working on my own addon as well, and I strive to leave no warning at all; however I use stricter GDScript warnings than the default settings (basically enforcing static typing and checking safe method/parameter access, etc.).
As it stands, any time I run my tests, I see about 400 warnings (and that's likely truncated output), but I can't be sure they all come from GUT, I have to go through my files to check I have no warnings. Luckily those 400 warnings do not appear when I run the actual project, only the tests.

Now I'm aware this may require quite a bit of work, but this should also help make the project more robust overall.

@bitwes
Copy link
Owner

bitwes commented Jul 30, 2024

Short, Impulsive, Knee Jerk Reaction

No. Absolutely not.

The Actual Answer

400 is a lot. Probably too many. 9.3.0 introduced code that was supposed to avoid generating warnings. It seems to be working with the default warning levels, but I have verified that if you set everything to "Warn" and run from the command line (warnings are not suppressed there), it prints 841 warnings.

841 is "a little" more than "a lot" more than "a lot" (if "a lot" = 400).
Is it too many? For sure.

Something should be done to suppress these warnings.

But...strict typing...

We can discuss all the benefits and perils of dynamic vs strict. At the end of the day, when I'm using a dynamically typed language, I'm going to do dynamically typed language stuff. I'm happier that way (the inverse is also true, to some extent).

There are some cases where strict typing makes GUT easier to use, and I'm open to those kinds of changes. GUT should also do its best to stay out of the way of the end user, regardless of how they like to work.

Numbers

From the addons/gut directory:

  • grep -r 'func ' | wc -l prints 1206
  • grep -r 'var ' | wc -l prints 1492
  • find . -name '*.gd' | xargs wc -l prints 14395

That's 1,206 functions, 1,492 variable declarations in (maybe) 10,000 lines of code. And that's just 3 numbers.

If we only had add ->void to all the functions, that's 6 * 1206 = 7236 characters.
If we only had to add :Variant to all the variables, that's 5 * 1492 = 11936 characters.
Let's say functions have an average of .5 arguments. Most should get :Variant as well. So that's another 8 * 603 = 4824 characters.

That's 3,301 changes to make, consisting of 23,996 characters.

Q: What's the point of these numbers?
A: I got curious and calculated them.

@bitwes
Copy link
Owner

bitwes commented Jul 31, 2024

@Cykyrios
I've pushed up some fixes to the i650_more_warnings branch. It would be a lot of help (roughly 400 units of help) if you could try out the changes. I don't have any projects of my own to test it out with (that don't generate at least 7000 warnings on the highest settings).

You can get the addons/gut folder from here:
https://github.com/bitwes/Gut/archive/refs/heads/i650_more_warnings.zip

@Cykyrios
Copy link
Author

Cykyrios commented Aug 1, 2024

I just realized the 400 warnings were the project limit, the actual number was 1379. I'm happy to say that I got no warnings at all with the downloaded folder, even with all warnings enabled (at that point, I get warnings when running my project but nothing from GUT tests).
I had a very quick look at the scripts to see what the difference was, it looks to me like addons warnings were not properly overridden before?

I think this won't change my initial request for static typing, but not having to manually disable addons warnings is already an improvement, I'll just have to go through my tests to make sure there are no warnings there.

@bitwes
Copy link
Owner

bitwes commented Aug 1, 2024

Thanks for testing it out. I should be merging this into main soon.

it looks to me like addons warnings were not properly overridden before?

Warnings were not being disabled early enough when tests are being run. This didn't matter with default settings because GUT didn't generate any of the default warnings. Extra/earlier steps had to be taken to avoid output from more strict warnings.

I think this won't change my initial request for static typing

If you can share scenarios where static typing would make using GUT easier I would consider them. I consider 400 warnings (much less 1379) a big usability issue and GUT should be as easy to use as possible.

If there isn't an end-user benefit though, I don't see any justification for adding static typing throughout the project.

@Cykyrios
Copy link
Author

Cykyrios commented Aug 1, 2024

I have 2 reasons for using static typing and stricter warnings:

  • better auto-complete as types are known
  • reduced risk of runtime errors caused by typos or functions/members not present on some objects

But extending GutTest, autocomplete works fine, so I don't think there would be an obvious benefit for most users (except checking my tests don't generate additional warnings).

Anyway, this fix makes my life much easier already, and typing everything (ideally not just Variants everywhere) would definitely require a large chunk of time.

@bitwes
Copy link
Owner

bitwes commented Aug 2, 2024

Great! Thanks for you help with this. I'll close this out when the next point release comes out (soon). If anything else comes up let me know.

better auto-complete as types are known

The auto-complete is great. In the next release, the InputSender gets a class_name of GutInputSender for this reason (and other reasons).

ideally not just Variants everywhere

I may have exaggerated a bit in my first reply, but almost all the asserts (and supporting methods) must accept Variant since you cannot overload methods (assert_eq is a good example, you can call it with any two things). This is true of a lot of the user facing methods. Accepting a variant allows them to be flexible. End-users may not be using strict types, so methods need to take anything and validate the datatypes. It is better to have a test fail and the run keep going than to get an error that causes tests to not run.

For example, if takes_an_int wasn't strictly typed the following test would fail. When run in debug mode it does break on this line, but without debug you just get a script warning and everything continues with a false positive.

func takes_an_int(i : int):
	print(i)

func test_does_dataype_mismatch_cause_a_break():
	pass_test('this should fail later')
	var p = 'asdf'
	takes_an_int(p) # causes script error at runtime
	fail_test('If we get here it fails like we want it to')

Again, thanks for your help. I really enjoy diving into the details on this kind of stuff.

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

No branches or pull requests

2 participants