Improve GDScript Parser error messages

[voylin]

Fixed/improved following TODO's inside of gdscript_parser.cpp

• Line 1246: // TODO: Update message to only have the missing one if it's the case.
• Line 2131: // TODO: Properties setters and getters with unreachable code are not being warned
• Line 3421: // TODO: The analyzer can see if this is actually a Callable and give better error message.

Let me know if some need better wording or more information.

The set/get message:

Results in: `SCRIPT ERROR: Parse Error: Expected "get" after "set" in property declaration.` and `SCRIPT ERROR: Parse Error: Expected "set" after "get" in property declaration.` SCRIPT ERROR: Parse Error: Expected "get" after "set" in property declaration.

Unreachable code of property setters and getters

[dalexeev]

Thanks for the PR! Improvements like these should be accompanied by unit tests.

[dalexeev]

This TODO refers to improving error checking at the static analysis stage. At the parsing stage, data types are not yet resolved.

[voylin]

I'll look into this if I can achieve this or not.

[voylin]

I've added the changes to gdscript_analyzer.cpp which are hopefully what was requested from this TODO.

[voylin]

Thanks for the PR! Improvements like these should be accompanied by unit tests.

Thanks! I'll get working on that when I find the time in the next few days.

[voylin]

@dalexeev I added the necessary changes to the scripts and added some unit tests. (I hope that I did it correctly, added images to have more visual feedback.

extends Control


# Unit test for set/get errors.
var x:
	set(value): x = value
	invalid_keyword: pass
var y:
	get: return y
	invalid_keyword: pass
var z:
	invalid_keyword: pass
# Unreachable test
var test_var: int:
	get:
		return 0
		print("unreachable getter")
	set(val):
		test_var = val
		return
		print("unreachable setter")


func _ready() -> void:
	# Lambda unreachable test
	var callable: Callable = func() -> void:
		print("yes")
		return
		print("no")
# Function unreachable test
	print("yes")
	return
	print("no")

[AThousandShips]

Doesn't look like you added any unit tests?

[voylin]

Doesn't look like you added any unit tests?

My bad, understood it wrong and thought results of the different prints should be mentioned in the PR. ^^" Added the unit tests, but I had to give up on Line 3421: // TODO: The analyzer can see if this is actually a Callable and give better error message. It's too much beyond what I can do at this point. Been trying to get it to print a better error message from gdscript_analyzer, but the unit tests and my own testing kept returning the standard "can't find function ...". So giving up on that one, but hopefully the other 2 are okay now.

[voylin]

@AThousandShips Is there anything else I should do for this PR to get merged or doesn't it add enough value so it's better to close this PR? ^^"

[AThousandShips]

I can't speak to that, it still needs approval from the GDScript team