godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.9k stars 20.16k forks source link

Accessing nonexistent properties on inner classes doesn't raise UNSAFE_PROPERTY_ACCESS #92499

Open toolness opened 3 months ago

toolness commented 3 months ago

Tested versions

System information

Godot v4.2.2.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 31.0.15.3699) - AMD Ryzen 9 5900X 12-Core Processor (24 Threads)

Issue description

The last two lines of this should raise UNSAFE_PROPERTY_ACCESS when the associated preference is set, but they don't:

class Foo:
    pass

func do_thing_with_new_foo() -> void:
    var foo = Foo.new()
    foo.nonexistent_property = 1
    print(foo.other_nonexistent_property)

Steps to reproduce

I'm using VSCode. UNSAFE_PROPERTY_ACCESS is properly raised as a squiggly red underline on named/global classes, but not on inner classes.

Minimal reproduction project (MRP)

N/A

toolness commented 3 months ago

Actually, I just noticed that if the instantiation of Foo in my example is the following:

var foo: Foo = Foo.new()

Then the later UNSAFE_PROPERTY_ACCESS are raised properly.

However, if I don't explicitly annotate foo and write foo.nonexistent_method(), that does properly raise an UNSAFE_METHOD_ACCESS warning, so... something very strange is going on. For now I guess I will just try to remember to explicitly type all of my variables, but this behavior (I'm uncertain if it's actually a bug or not) doesn't give me a lot of confidence in GDScript's strict typing.

huwpascoe commented 3 months ago

Assigning untyped means the variable is a Variant type, which the editor isn't able to track apparently.

There's a very useful shorthand for creating typed variables by deduction: var foo := Foo.new()

toolness commented 3 months ago

Ah, thanks for the clarification! I was confused about the use of := vs. = and it was unclear that := infers the type and = doesn't.

Upon searching, I noticed that this is explained in the GDScript style guide but not in the Static typing in GDScript documentation--I had been reading the latter, and not the former!

matheusmdx commented 3 months ago

Upon searching, I noticed that this is explained in the GDScript style guide but not in the Static typing in GDScript documentation--I had been reading the latter, and not the former!

Just to clarify, this is also explained in the Static typing in GDScript docs:

image

toolness commented 3 months ago

Ah sorry, my bad! Thanks for clarifying.

For what it's worth, it might be helpful to really drive this home in some sort of callout--many existing languages that support type inference (TypeScript, Python, Rust, etc) don't require a separate operator to infer types on assignment, so this could be a potential stumbling block for folks coming from those. I could submit a PR to add such a callout if you think it'd help.

Additionally, possibly providing a toggle-able warning (similar to unsafe property/method access) might be helpful here too, to make sure that folks who always want to write type-safe code always use := instead of =? Or are there exceptions where = is really what you want to use even in typed code?

dalexeev commented 3 months ago

Additionally, possibly providing a toggle-able warning (similar to unsafe property/method access) might be helpful here too, to make sure that folks who always want to write type-safe code always use := instead of =?

There is UNTYPED_DECLARATION warning.

huwpascoe commented 3 months ago

Yeah, although it's mentioned I didn't notice the shorthand until some time after learning GDScript.

The only place you'd use = in declaring a variable is if it's to be a variant.

# these are same type, Variant
var a = "one"
var b: Variant = 1

And which one to use, technically should use b. Just like functions that don't return a value should be technically declared func foo() -> void: but, the internal editor's formatter is kinda basic and I'm kinda lazy.