godot icon indicating copy to clipboard operation
godot copied to clipboard

GDScript 2.0: Variant type errors

Open MikeSchulze opened this issue 1 year ago • 20 comments

Godot version

v4.0.stable.official [92bee43ad]

System information

Windows 10, MacOs

Issue description

I encountered this problem by recording a use case for a bug report. And was very surprised that I did not see these errors before. Why are the addons peer default excluded from script validation? I think many don't know this, I was also very surprised when I found this option.

The compiler shows some errors, but this should not be an error or a warning The variable type is being inferred from a Variant value, so it will be typed as Variant. (Warning treated as error.)

Steps to reproduce

put this code in a fresh script under the root folder

extends Node


func callable_return_type(clazz :Callable):
	var value := clazz.call()


func meta_variant():
	var value := Engine.get_meta("value")


func array_variant():
	var store :Array[Variant] = []
	var value := store.pop_front()


func foo() -> Variant:
	return ""

func return_type_variant():
	var value := foo()
Row 5:The variable type is being inferred from a Variant value, so it will be typed as Variant. (Warning treated as error.)
Row 9:The variable type is being inferred from a Variant value, so it will be typed as Variant. (Warning treated as error.)
Row 14:The variable type is being inferred from a Variant value, so it will be typed as Variant. (Warning treated as error.)
Row 21:The variable type is being inferred from a Variant value, so it will be typed as Variant. (Warning treated as error.)

For the Callable example, I would say that the error/warning is correct. It's just that Callable has no type information, it should be improved as it was done for Array For all the other errors it should be corret, the return type is defined as Variant

Minimal reproduction project

N/A

MikeSchulze avatar Mar 07 '23 06:03 MikeSchulze

This is intentional, as far as I remember. See #72608.

dalexeev avatar Mar 07 '23 07:03 dalexeev

This is intentional, as far as I remember. See #72608.

But why? I cant see any reason why this should a warning or error. It is just an typed assignment of type Variant

MikeSchulze avatar Mar 07 '23 10:03 MikeSchulze

It's a typed assignment that has no effect, the result isn't typed

AThousandShips avatar Mar 07 '23 10:03 AThousandShips

It's a typed assignment that has no effect, the result isn't typed

nope it is typed! func return_variant() -> Variant has type Variant

Variant is also a type, it just like a generic type From Documentation

Klasse:   Variant

The most important data type in Godot.

The most important data type in Godot.

this is simply not consistent and should behave in the same way as other data types

MikeSchulze avatar Mar 07 '23 10:03 MikeSchulze

var is Variant, it has no effect to static typing it to Variant, the warning is likely there to indicate that you're making some mistake, by not realizing the function is not statically typed

AThousandShips avatar Mar 07 '23 10:03 AThousandShips

Why should this be an error? The assigned value is then a variant and should be clearly recognizable.

MikeSchulze avatar Mar 07 '23 10:03 MikeSchulze

Why would you ever static type something to Variant? It is pointless, so it likely implies something has been missed

AThousandShips avatar Mar 07 '23 10:03 AThousandShips

Most likely done by accident, usually inference is trying for a particular type.

From the source where it is set to be error

AThousandShips avatar Mar 07 '23 10:03 AThousandShips

Why would you ever static type something to Variant? It is pointless, so it likely implies something has been missed

As example if you write a generic value provider it can return different types depending to the implementation. func get_value() -> Variant:

I used this in my testing framework to receive a custom value as fuzzer result. The value is passed to the asserts to verify for a certain type/value.

I don't understand why this is treated as an error, it is just the sense of generics that I don't care about the type and only in the individual case it is dealt with according to the application. typically with instanceof or is_type

MikeSchulze avatar Mar 07 '23 10:03 MikeSchulze

As far as I understand it, in the case of Variant you should specify the type explicitly (var a: Variant = get_variant(), or you can simple var a = get_variant()) and not rely on type inference to avoid possible errors.

Variant is not a separate type, it is an indication that the value can be of any type. Variant has a few exceptions that distinguish it from other explicit types (for example, a variable of type Array (Array[Variant]) can refer to Array[int], although this is unsafe).

dalexeev avatar Mar 07 '23 10:03 dalexeev

It's not treated as an error, it's a warning, because it points at a potential issue. Basically, having Variant as a return type for explicitness does make sense, but using Variant as a type hint for a variable doesn't, because everything is a Variant, and that just means it's not actually typed.

YuriSizov avatar Mar 07 '23 10:03 YuriSizov

As far as I understand it, in the case of Variant you should specify the type explicitly

And that's the fact that I don't understand why I must do this only for the Variant type, that's inconsistent. It makes no difference in the end var value := get_variant() or var value :Variant = get_variant() to write. It is both the same, the variable value is typed as a Variant.

It's not treated as an error, it's a warning,

Nope it is reported as error see https://github.com/godotengine/godot/pull/72608/files#diff-06b39260e730d96cc70ee28502cbe05c19b7dc10fb9579e496dde2fd4cee051eR129

but using Variant as a type hint for a variable doesn't, because everything is a Variant, and that just means it's not actually typed.

means var value :Variant = get_variant() would also be untyped according to your statement, but does not generate an error, so somehow does not quite fit together

MikeSchulze avatar Mar 07 '23 12:03 MikeSchulze

And that's the fact that I don't understand why I must do this only for the Variant type, that's inconsistent.

Because only for Variant var a = f() and var a := f() are the same thing.

dalexeev avatar Mar 07 '23 12:03 dalexeev

If you don't want it to be treated as an error change the setting? The reasoning for the default is given

AThousandShips avatar Mar 07 '23 12:03 AThousandShips

If you don't want it to be treated as an error change the setting? The reasoning for the default is given

this is no help for plugins, because this is a global property and would affect the whole project

I will fix my issues by just do: var value :Variant = get_variant() but it just code noise

MikeSchulze avatar Mar 07 '23 12:03 MikeSchulze

this is no help for plugins, because this is a global property and would affect the whole project

All warnings in plugins are ignored by default.

(But you should still avoid warnings.)

dalexeev avatar Mar 07 '23 12:03 dalexeev

All warnings in plugins are ignored by default.

I now this ;) I was surprised that this property exists at all. This has led to the fact that I only now see the errors in my plugin.

MikeSchulze avatar Mar 07 '23 12:03 MikeSchulze

The exclude addons setting is meant to end users to not have to see a bunch of warnings that come from the addons they use, since often those come from a third-party.

Regarding inference on Variant, it is done on purpose as said before. The fact that you are relying on a function that returns Variant for your static typing can be very problematic, since it can easily not be your intention.

Usually you use a static type to restrict what can be stored in the variable, and type inference is just meant to omit redundancy when the type is obvious from the default value. Variant is most often not an obvious return type. So if you are using static type to not restrict the type (by using Variant) you have to tell so explicitly, because type inference is probably a mistake for this case. Or not use a static type at all, which behaves the same. The Variant is not really a type, it's just a explicit way to tell when there's no type restriction, it is not supposed to be used implicitly.

Your Callable.call() example is a good showcase on how this can be done accidentally. One may think the Callable is retaining information about its bound function and restricting to its return type, but it does not and only use the generic Callable interface, which returns Variant.

If you don't want this behavior you can either use the @warning_ignore annotation or disable it for the project.

vnen avatar Mar 07 '23 22:03 vnen

Ok, so in the end this error is displayed only to avoid errors? That's funny, at least the developer should decide if this is an error or not. This should not be set peer default by the engine, no other language does that as far as I know. It is in the end nothing else like object in C#, it even says so in the official documentation. So the caller is responsible what should happen with the function result. There is for example Engine.get_meta() -> Variant. So if I write: var data :Variant= Engine.get_meta() then I don't see any errors and I have to test at the end anyway what type it is. In the end the developer always has to look at the type to know what he is working with.

My point here is that you can't just assume a priori that it's a mistake. Just the use of Variant as return type or argument makes the whole thing flexible.

But what am I talking about here, this seems to be a done deal. So i just fixed the error, where is not an error in my case. ;) You can close this discussion, if you want

MikeSchulze avatar Mar 08 '23 10:03 MikeSchulze

The main problem is that GDScript is not a strongly statically typed language. It can be used in dynamic style, static style, or even a combination of the two (although we recommend sticking to one style within a project).

= and := are too similar to each other, so we can't always tell if it's a mistake or the preffered style. But we want to help users avoid mistakes when we see that they are probably doing something weird.

You can disable the warning in your project and use whatever style you want. Of course, if you are developing a plugin, you should also consider that user preferences may differ from yours. And it may be less convenient for you if you want your plugin to be compatible with as many projects as possible. The alternatives have already been mentioned above: 1. use = instead of := (for Variant it's the same in practice), or 2. specify the Variant type explicitly.

Note that even if we change the default level from "Error" to "Warn", little will change for you as the plugin developer: you can still expect the user to switch the level to "Error" (since you expect the user to disable the "Exclude Addons" checkbox).

dalexeev avatar Mar 08 '23 11:03 dalexeev

But what am I talking about here, this seems to be a done deal.

Everything can be changed. This is a new thing in Godot 4.0 which was just released, so it will take time until we understand the full implication of this to all users.

I still prefer to make it an error by default and annoy some users than the opposite and have users introduce subtle issues without realizing. But if most users do expect this as a valid behavior we can change it to warn or ignore by default.

I personally do find odd to infer Variant because it's not a type (i.e. there is no value for x that would make x is Variant to return true, if that was a valid check, unless we hardcoded it to always be true). But everything is up to discussion and I'm open to listen to arguments.

I proposed adding a note to this in the documentation (#6934). I believe this report is now resolved. Anyone is free to create a proposal or discussion to ask for a change in the defaults.

vnen avatar Mar 09 '23 19:03 vnen

Thanks to all who took the time to discuss this. Let's see what time brings ;)

MikeSchulze avatar Mar 09 '23 19:03 MikeSchulze