Re-add local variable type hints

[TheLimeGlass]

Description

Re-adds local variable type hints. This allows Skript to understand local variable types in some circumstances, and provide warnings so the user knows nothing will happen. Like setting a variable to a string and then attempting to teleport the player to the string should produce a warning.

Also made calling functions with local variables of the wrong type error #1229

Considering this breaking change I guess, since some users may not know their scripts aren't working.

Examples of proper warning now:

test "type hints":
	set {_string} to "empty"
	set helmet of last spawned zombie to 1 of {_string}
	# {_string} still a string
	add stone to {_string} # Fails and does nothing
	# {_string} still a string
	set helmet of last spawned zombie to 1 of {_string}
	# {_string} still a string

	set {_timespan} to a second
	set helmet of last spawned zombie to 1 of {_timespan}

[Skript] Line 3: (..\..\..\..\..\src\test\skript\tests\misc\type-hints.sk)
     Variable '{_string}' is of type 'text', not a 'item stack, item type or entity type'
     Line: set helmet of last spawned zombie to 1 of {_string}

 [Skript] Line 7: (..\..\..\..\..\src\test\skript\tests\misc\type-hints.sk)
     Variable '{_string}' is of type 'text', not a 'item stack, item type or entity type'
     Line: set helmet of last spawned zombie to 1 of {_string}

 [Skript] Line 11: (..\..\..\..\..\src\test\skript\tests\misc\type-hints.sk)
     Variable '{_timespan}' is of type 'time span', not a 'item stack, item type or entity type'
     Line: set helmet of last spawned zombie to 1 of {_timespan}

Notes:

• I think the issues bensku had in the past was the clearing of the type hints, which would cause either a race condition or just be the incorrect type.
• The test currently produces warnings as it should. There currently isn't a way to test against proper warnings, so i'll also need to do that in a new pull request soon.
• No, I do not think this feature should be configurable via the config.sk, if a type is not the correct type it will not work in general, so why hide the warning. What are you doing? (I added this to EffSuppressWarnings so it can technically be accessed for users, it's more added for the test runner though)
• Addons can use the following if they wish to disable this warning because they're doing witch craft:

getParser().getCurrentScript().suppressWarning(ScriptWarning.LOCAL_VARIABLE_TYPE);

---

Target Minecraft Versions: any
Requirements: none
Related Issues: #1229, #3864 and #5060

[TheLimeGlass]

Also fixed #1229

[APickledWalrus]

Looking excellent 👍

[kiip1]

Looks good

[kiip1]

Time for this to become an enum I guess

[APickledWalrus]

Yeah we could just use a switch statement to assign the ScriptWarning value based on the parse mark

[kiip1]

Or some ordinal like:

int mark = parseResult.mark;
if (mark < 0 || mark >= Enum.values().length)
	return false;
warning = Enum.values()[mark];

[APickledWalrus]

Oh yeah this is already done in init. Could probably store the ScriptWarning we are suppressing and use that for the toString switch instead of the int constants

[TheLimeGlass]

I was thinking of making it it's own type. This is user accessable, so making it a ClassInfo is just simplier for us to add new enums in the future.

I wasn't going to address this here, but I guess I can.

[APickledWalrus]

We could do that too. You don't have to do it for this PR though

[TPGamesNL]

I remember one of the issues of local variable type hints being syntax like do X and store result in {_var}, often used for async computations. Skript doesn't know at parsetime that this variable will be changed, so it will know have the right type hints.

How does this PR deal with that issue?

[TheLimeGlass]

I remember one of the issues of local variable type hints being syntax like do X and store result in {_var}, often used for async computations. Skript doesn't know at parsetime that this variable will be changed, so it will know have the right type hints.

How does this PR deal with that issue?

It doesn't, and it's not the end of the world either, it'll act without type hints still.

Maybe a better implementation rather than listening to the set/add would be the listening in on the variable change in Variables.class.

Would be able to add a JUnit async effect test once the JUnit pull request is merged.

[TPGamesNL]

Listening for acceptsChanges might also not work, I've seen addons use if (expr instanceof Variable) {} before instead of properly checking if the expr can be changed, so they never indicate to Variable that the var will be changed at runtime.

Indeed it's not the end of the world, but it's annoying for a user to get a false positive warning, it may confuse them.

Make sure to also test for code that changes Event (and thus local variables) within a structure, such as sections (newer and older impls of them). Also those that carry over local variables from another event (e.g. spawn section of Skript)

[TheLimeGlass]

Listening for acceptsChanges might also not work, I've seen addons use if (expr instanceof Variable) {} before instead of properly checking if the expr can be changed, so they never indicate to Variable that the var will be changed at runtime.

Indeed it's not the end of the world, but it's annoying for a user to get a false positive warning, it may confuse them.

Make sure to also test for code that changes Event (and thus local variables) within a structure, such as sections (newer and older impls of them). Also those that carry over local variables from another event (e.g. spawn section of Skript)

Ok, well I applied type hints when the change method of the variable expression is used. If an addon does not properly use the change method we cannot provide type hints for it. I don't want to be re-parsing a VariableString to check if it's simple in the Variables.setVariable method.

[TheLimeGlass]

NOTE: It should be failing. I have to update those tests to correct them now.