`UNSAFE_CAST` warning is useless or has confusing description

[pineapplemachine]

Tested versions

I noticed this regression when opening my project (previously 4.2.2) in v4.3.rc1.mono.official [e343dbbcc]

System information

Windows 10 64-bit

Issue description

I have most of the GDScript-related settings turned to errors or warnings in my project, because when they behave reasonably I find these warnings and errors to be very helpful in catching my mistakes as soon as I write them instead of later on at runtime.

Recently I opened up my project in 4.3-rc1 and noticed some new UNSAFE_CAST errors that were not present with 4.2.2. In particular this is happening wherever I was using some_variant as [Type]. This happens with both classes (nullable) and with primitives/value types (not nullable).

Here's an example of an affected function. The line var point := intersection as Vector3 now produces the error Casting "Variant" to "Vector3" is unsafe. (Warning treated as error.).

func get_ray_intersection_xz(
    ray_origin: Vector3,
    ray_normal: Vector3,
) -> Variant:
    var plane := self.get_plane_xz()
    var intersection: Variant = plane.intersects_ray(ray_origin, ray_normal)
    if intersection == null:
        return null
    var point := intersection as Vector3
    point *= self.basis.inverse()
    point -= self.global_position
    return Vector2(point.x, point.z)

It is possible to work around this newly occurring error by changing the line to an implicit cast instead of an explicit cast, like so:

    var point: Vector3 = intersection

This regression was apparently introduced by https://github.com/godotengine/godot/pull/84043, which closed https://github.com/godotengine/godot/issues/84027.

Discussion on the related issue suggests that casts like variant_value as [Type] should always be considered unsafe, but the only alternative offered is to use type_convert, which also cannot be used except via an implicit cast (i.e. with a variable assignment or as the typed return value of a function, not using as) because type_convert itself returns a Variant. This means that result of type_convert (e.g. type_convert(intersection, TYPE_VECTOR3) per the above example) also cannot be used type-safely except via, again, an implicit cast.

I think there really needs to be a way to explicitly convert a Variant to another typed value that does not produce a type safety error. And at some point there should probably be an UNSAFE_IMPLICIT_CAST error so that these implicit casts can also be configured to generate errors.

Ideally I would like very much to see nullable types merged for 4.3 and for casts from Variant to any nullable type to be considered safe (producing null if the Variant did not contain a value of the type being explicitly cast to), e.g. variant_value as Vector3? and casting to non-nullable value types to be unsafe. But until nullable types are a thing, I would rather still be able to write variant_value as [Type] to explicitly mark intentional casts, rather than be required to use implicit casts to work around the error, or have to turn the UNSAFE_CAST error/warning off entirely and lose its benefits in other situations. https://github.com/godotengine/godot/pull/76843

In my opinion, adding built-in functions such as int_convert, float_convert, vector3_convert, etc. for each possible type to use instead of casting with as would also be an acceptable resolution, albeit not an ideal one, with these functions returning the corresponding types directly rather than returning a Variant as with type_convert.

(Edit: Oh, when writing the above, I assumed that invalid casts like 0 as String also produced an UNSAFE_CAST error/warning, but it seems that this case produces a different "Invalid cast" error and UNSAFE_CAST doesn't actually apply to anything except Variants. I guess it's unclear what benefit this error/warning has in any case, then, if it forbids implicit casts but not explicit casts, and if it's unsafe now even to cast a Variant to a reference (nullable) type?)

Steps to reproduce

Minimum reproduction:

Turn on UNSAFE_CAST warnings/errors in project settings:

An UNSAFE_CAST warning or error will occur for line 3 of this GDScript code:

func _ready() -> void:
    var value: Variant = Vector2.ONE
    var vector: Vector2 = value as Vector2

Minimal reproduction project (MRP)

See trivial reproduction steps

[AThousandShips]

CC @dalexeev

[dalexeev]

This is not a regression. The warning has been adjusted to reflect the actual behavior of the as operator. It is unsafe because it can cause runtime errors. This is the problem that needs to be solved (by changing the behavior of the as operator or by introducing a new type casting method) if we want to make every cast type-safe. Then we can deprecate the warning, but until then the warning is valid. See #84027 for details.

Currently you can check the type using the is operator:

var a: Variant
if a is int:
    var b: int = a

Or alternatively you can disable the warning in the Project Settings if you don't care about this behavior.

It is possible to work around this newly occurring error by changing the line to an implicit cast instead of an explicit cast, like so:

    var point: Vector3 = intersection

This is not an implicit type casting, but a type narrowing. There is a proposal for an explicit type narrowing syntax, but we tend to think that the static analyzer should do a better job of tracking control flow in general and local assignments and type checks in particular.

    var point: Vector3 = intersection

This is correct if you have already checked the type before. In this case, type casting (as) should not be used, as it generates unnecessary runtime code.

[pineapplemachine]

@dalexeev Ah, just as you were commenting I had updated the issue with a newer realization:

(Edit: Oh, when writing the above, I assumed that invalid casts like 0 as String also produced an UNSAFE_CAST error/warning, but it seems that this case produces a different "Invalid cast" error and UNSAFE_CAST doesn't actually apply to anything except Variants. I guess it's unclear what benefit this error/warning has in any case, then, if it forbids implicit casts but not explicit casts, and if it's unsafe now even to cast a Variant to a reference (nullable) type?)

My understanding is that my referring to var point: Vector3 = intersection as an implicit cast, not type narrowing, is correct. If I had written if intersection is Vector3: var point: Vector3 = intersection, then that would be narrowing. But the narrowing if condition is not required here. In your example of var b: int = a, this line will not produce any error even if you omit the type narrowing if a is int:.

In any case, now that I realize that ignoring UNSAFE_CAST does not affect anything except for Variants and it was not providing any benefit before or after https://github.com/godotengine/godot/pull/84043, I can just turn off that error/warning. I had assumed that the "Invalid cast" errors for problematic casts with other non-Variant types were affected by that setting as well, but it turns out not.

This still seems like a problem though.

[dalexeev]

@pineapplemachine Please take a look at the following example to better understand the warning purpose:

var a: CanvasItem = Node2D.new()
# Compile time: type safe.
# Runtime: OK.
var b := a as Control
print(b) # Prints "<null>"

var c: Variant = 123
# Compile time: UNSAFE_CAST warning.
# Runtime: Error "Invalid cast: could not convert value to 'String'.".
var d := c as String
print(d)

There is the same type relationship between (CanvasItem, Node2D, Control) and (Variant, int, String), but the as operator behaves differently for objects and built-in types.

Objects are nullable, null is the default and valid value for any object class. While built-in types have their own default values. The as operator and the deprecated convert() function prohibit casting types that are not implicitly convertible (like int and float). While the new type_convert() silently returns a default value in this case. We could make as behave like type_convert(), but that requires discussion and consideration of compatibility concerns.

[pineapplemachine]

This code produces a runtime error, because it contains an invalid implicit cast. But it does not trigger any warning or error currently:

var variant_value: Variant = ""
var int_value: int = variant_value # Trying to assign value of type 'String' to a variable of type 'int'

This code produces an UNSAFE_CAST error, though it does not cause a runtime error:

var variant_value: Variant = Node.new()
var control: Control = variant_value as Control # Casting "Variant" to "Control" is unsafe.
print(control) # Prints <null> (No runtime error!)

Here is what should happen eventually:

• variant_value as ReferenceType should be considered safe. At runtime, this expression very reasonably produces null when the Variant value is not an instance of ReferenceType. Otherwise it produces the non-null value of the Variant.

• Nullable value types should be added, per https://github.com/godotengine/godot/pull/76843. variant_value as ValueType? should be considered safe. At runtime, this expression should produce null when the type of the value boxed by the Variant could not normally be cast via value as ValueType. Otherwise the produced nullable value should contain the non-null value of the Variant, cast to the nullable type.

• variant_value as ValueType should trigger an UNSAFE_CAST error, because null is not representable with ValueType, and falling back to a default value instead of producing an error for strange casts hides mistakes and would differ from the casting behavior standardized by other languages, and from the current (desirable) behavior of e.g. string_value as int producing an "Invalid cast" error. Syntax like variant_value as int? ?? 0 can be used instead, if a fallback is desired, using the null-coalesce operator as already proposed in the nullable value types PR.

• var typed_value: ValueType = variant_value (without a preceding type guard e.g. if variant_value is ValueType:) should trigger an UNSAFE_CAST error, because this is an unsafe implicit cast. (Or arguably a new UNSAFE_IMPLICIT_CAST error, to ease the migration of existing codebases that currently make use of the implicit cast loophole, so that the explicit and implicit casting errors can be separately toggled.)

But right now, neither nullable value types nor type narrowing via type guards are supported by GDScript. Because of that, the UNSAFE_CAST error is useless. This is because there is no way to cast a Variant to a value type that does not violate GDScript's type safety, except through the loophole of using an implicit cast (via either assignment or returning from a function with a return type annotation), since implicit casts are not yet checked by the type safety system. The new 4.3 behavior doesn't even permit explicit casting in cases that can be statically verified not to produce a runtime error, i.e. the case of casting to a nullable reference type.

[dalexeev]

This code produces an UNSAFE_CAST error, though it does not cause a runtime error:

var variant_value: Variant = Node.new()
var control: Control = variant_value as Control # Casting "Variant" to "Control" is unsafe.
print(control) # Prints <null> (No runtime error!)

var variant_value: Variant = 123
# Compile time: UNSAFE_CAST warning.
# Runtime: Error "Invalid cast: can't convert a non-object value to an object type.".
var control: Control = variant_value as Control
print(control)

Because of that, the UNSAFE_CAST error is useless.

Please read my comments carefully. The fact that the as operator can generate type errors at runtime means that the operator is unsafe. When we are talking about static types, just one instance of such an error is enough for it to be considered unsafe in general.

It does not matter that you assume that Variant in your case is Vector2 | Nil, a Variant value can be of any type, built-in or object. And the as operator is unsafe at runtime when you try to cast:

1. non-convertible built-in types to each other;
2. a built-in type to an object;
3. or an object to a built-in type.

Please look at the test cases in #84043 and study the issue carefully before suggesting solutions.

[pineapplemachine]

Please read my comments carefully.

I believe I do understand what you are saying. I think you have missed my point. The error is useless because:

1. UNSAFE_CAST does not apply to implicit casts, only explicit casts.
2. If UNSAFE_CAST did apply to implicit casts, then it would be impossible to unbox Variants without triggering a type error. It would become impossible to interact with vast parts of the Godot API without producing a type safety error or warning, because of the lack of tools to interact with Variant values in a way that satisfies the type system.
3. As of 4.3, UNSAFE_CAST newly triggers for cases like variant_value as ReferenceType, even though these can be statically verified not to cause a runtime error. (This cast, of course, safely produces null at runtime.)

The result is that there is no reason for anyone to turn on UNSAFE_CAST as a warning or error in their project. The error/warning conveys no useful information, not in 4.2 or in 4.3, because the only way to comply and silence the warning/error is via the implicit cast loophole in the type system, where explicit casts are checked for type safety but implicit casts are not.

[pineapplemachine]

Further attempting to explain myself...

For the warning/error to not be useless, there would need to be an explicit way to unbox a value type from a Variant that will not cause either a runtime error or a type safety error. Such as via support for nullable value types and/or support for type narrowing, which I explained before. Typed convert functions that do not themselves return another Variant, like I previously suggested with int_convert, etc. could also be a solution.

Also, the error should not occur when casting to a reference type, e.g. variant_value as ReferenceType, since this can be statically verified not to cause a runtime error.

If these two issues were resolved: 1. Lack of an explicit way to cast or convert value types without triggering the error/warning and 2. Lack of an explicit way to cast reference types without triggering the error/warning, then UNSAFE_CAST would no longer be useless, and there would be a constructive reason for users to set UNSAFE_CAST to produce an error or warning instead of being ignored.

Additionally, implicit casts (assignments and returning from a function with a return type annotation) should really be checked with at least the same strictness as explicit casts using as. It is not normal or sensible for a type system to be more permissive with implicit casts than explicit casts. And these implicit casts will cause runtime errors just the same as explicit casts using as.

I am a hobbyist creator of small programming languages and a contributor in passing to tools like linters and standard libraries. I promise you I understand the problem. Please engage with my argument and do not dismiss my comments by suggesting I have not read carefully enough.

[dalexeev]

This code produces a runtime error, because it contains an invalid implicit cast. But it does not trigger any warning or error currently:

var variant_value: Variant = ""
var int_value: int = variant_value # Trying to assign value of type 'String' to a variable of type 'int

This is not an implicit cast, but an implicit and unsafe type narrowing. The line is marked as unsafe, but there is no named warning, unlike other situations:

Currently, it is your responsibility to ensure that this type narrowing is safe at runtime. Also, if you are trying to always have a green line instead of a gray one, please see the docs:

Safe lines do not always mean better or more reliable code.

I was thinking about adding UNSAFE_ASSIGNMENT and UNSAFE_OPERATION warnings, and I have an unfinished local branch (also there is a PR #83697). But I ran into some problems with GDScriptParser::DataType regarding weak types, plus there are ongoing discussions about explicit type narrowing syntax, improvements to control flow analysis, and a unified type system in general. So I put it on hold indefinitely.

3. As of 4.3, UNSAFE_CAST newly triggers for cases like variant_value as ReferenceType, even though these can be statically verified not to cause a runtime error.

No, I demonstrated this above. Casting a Variant to an object is unsafe because the value may be a built-in type, which will cause a runtime error.

The fact that there is no explicit type narrowing syntax or that the static analyzer does not properly track the control flow is not directly related to this warning. The fact that in the future we may change the behavior of the as operator and deprecate the warning is also irrelevant now. The warning reflects the current behavior of the as operator, we fixed the warning without knowing the future.

The point of this warning is to inform you that casting Variant to any type other than Variant is unsafe, since the cast operation itself may cause a runtime error. You are free to do whatever you want with this: disable the warning (it is disabled by default) or use type checking plus implicit type narrowing.

[pineapplemachine]

This is not an implicit cast, but an implicit and unsafe type narrowing.

This is not what the term "type narrowing" normally refers to. See, for example:

https://mypy.readthedocs.io/en/stable/type_narrowing.html https://www.typescriptlang.org/docs/handbook/2/narrowing.html

It is accurate to refer to this as implicit casting. This is because a value of one type is being converted (cast) to another, without the use of explicit conversion or casting semantics.

https://medium.com/@tiwaridiwakar9/type-casting-implicit-vs-explicit-7e6fea5ba823

Here is another way to hopefully demonstrate this difference between type narrowing and implicit casting: The line is still marked as unsafe even if it is preceded by a type guard. If the GDScript compiler recognized type narrowing, then the assignment line would not be marked as unsafe.

I think an UNSAFE_ASSIGNMENT warning would be a good idea.

No, I demonstrated this above. Casting a Variant to an object is unsafe because the value may be a built-in type, which will cause a runtime error.

Ok, now I see what you mean with this. Then this means that simply making variant_value as ReferenceType would not be adequate to fix the problem and make UNSAFE_CAST a useful warning, in combination with the fix for value type unboxing. Either this would be changed to produce null when the Variant contains a value type (I think this is reasonable behavior, but perhaps too major of a change to make now), or there would need to be some other way to explicitly convert a Variant value to a reference type value that does not produce a runtime error for Variants containing a value type, and also does not produce a type safety warning/error.

The fact that there is no explicit type narrowing syntax or that the static analyzer does not properly track the control flow is not directly related to this warning.

I agree. Since UNSAFE_CAST can be safely ignored, it probably is not appropriate to call this a regression as in the title of the issue. I was initially under the mistaken impression that the UNSAFE_CAST project setting also encompassed "Invalid cast" errors where Variants are not involved, due to the vague documentation of the "Unsafe Cast" option in project GDScript settings in 4.2.

But it does make UNSAFE_CAST useless, in that it does not provide constructive information to the user. This equivalent of "you must not use as to explicitly unbox Variants, you must use implicit casting instead" is not a useful error/warning.

And I really doubt I'm the only one who turned on the "Unsafe Cast" errors expecting them to be helpful in catching mistakes given the 4.2 description of the setting, and who is going to be caught off-guard when 4.3 causes correct code that previously did not produce type warnings or errors to suddenly have warnings/errors, without a clear way to deal with them. Users will likely think they need to change their code somehow to let the compiler recognize their code as safely making an explicit conversion, rather than thinking they need to use an implicit cast instead, or to set UNSAFE_CAST to ignored. This is presumably going to disrupt a lot of users' work with Godot, like it did mine, once 4.3 releases.

The point of this warning is to inform you that casting Variant to any type other than Variant is unsafe, since the cast operation itself may cause a runtime error. You are free to do whatever you want with this: disable the warning (it is disabled by default) or use type checking plus implicit type narrowing.

What you are questionably describing as "implicit type narrowing" still results in the assignment line being marked as unsafe. And the assignment is marked the same whether it is preceded by what could be statically recognized as a type guard (but isn't) or not.

Thank you for pointing that out about subtly differently colored line numbers, by the way. I had not yet realized that the color of the line number could communicate secret unnamed and unexplained warnings.

[dalexeev]

This is not what the term "type narrowing" normally refers to.

Terminology may vary, so let me clarify what I mean. The terms "type casting" and "type conversion" are often used interchangeably. Type conversions occur in various situations, both explicitly and implicitly. In this issue, by "type casting" I meant only the as operator. Not implicit type conversions (assignment, parameter passing, etc.) and not calls to built-in type constructors (Vector2(Vector2i)), global functions (type_convert()), or methods (String.to_int()).

Type narrowing or downcasting is a special case of type conversion where the source type is converted to its subtype. As opposed to the general case where the source and target types may not be a supertype and subtype (like String and StringName are implicitly convertible to each other, but are not a supertype and subtype). Type narrowing can also be understood as an explicit special syntax or implicit analyzer inference that serves to achieve type-safe downcasting (neither is currently available in GDScript).

The as operator is primarily an analogue of dynamic_cast, while some people use as as an analogue of static_cast. In my opinion, the as operator is not needed in the following options 2 and 3. Potentially, GDScript will be able to infer that this code is safe, remove false-positive unsafe lines and warnings.

# Option 1. Unsafe in fact and marked as unsafe.
var a: Variant = <Variant value>
var b := a as int # Correct UNSAFE_CAST warning.

# Option 2. Safe in fact, but marked as unsafe.
var a: Variant = <Variant value>
if a is int:
    var b1: int = a # Unsafe line (unsafe assignment).
    var b2 := a as int # Unsafe line (UNSAFE_CAST).

# Option 3. Safe in fact, but marked as unsafe.
var a: Variant = <Variant value>
if a is not int:
    return
var b1: int = a # Unsafe line (unsafe assignment).
var b2 := a as int # Unsafe line (UNSAFE_CAST).

Of course, this is not true at the moment and there is no actual difference between downcasting assignment and the type casting operator, in both cases GDScript double checks types at runtime (using different opcodes though). But there is a conceptual difference, in my opinion the as operator is not needed here and should not be used unnecessarily elsewhere.

Ok, now I see what you mean with this. Then this means that simply making variant_value as ReferenceType would not be adequate to fix the problem

It's incorrect to compare ReferenceType and ValueType here, it's correct to compare built-in types and objects. Array, Packed*Array, and Dictionary are passed by reference but are not nullable and are subject to the same problem with the as operator.

But it does make UNSAFE_CAST useless, in that it does not provide constructive information to the user. This equivalent of "you must not use as to explicitly unbox Variants, you must use implicit casting instead" is not a useful error/warning.

Yes, that's pretty much what the warning implies. If you strive for type-safe code, you shouldn't use the as operator on Variant expressions, you should do type checking and use unsafe-marked downcasting assignment instead, since there is currently no safe-marked alternative. In fact, the as operator is only safe for casting objects to objects (returning null if the cast is not possible). For built-in types, the as behavior is quite confusing and buggy.

If you have a suggestion for improving the warning message, I would appreciate it. In my opinion, the warning is not useless. It tries to be as helpful as possible in this situation. Note that we originally had the as operator with questionable behavior and a broken warning. If the warning didn't exist, I might agree with you on whether to introduce it or try to fix the as design first. But the warning already exists, so in my opinion it made sense to fix it and leave it up to the users, given that the warning is disabled by default.

[pineapplemachine]

But there is a conceptual difference, in my opinion the as operator is not needed here and should not be used unnecessarily elsewhere.

I don't think that a superfluous as should trigger an error, since of course it can be statically verified not to produce a runtime error, though it would be useful if it could trigger a separate warning. That is the sort of thing I'd expect a linter to point out. But I do agree that the as should be unnecessary in those type narrowing cases you gave.

Array, Packed*Array, and Dictionary are passed by reference but are not nullable and suffer from the same problem with the as operator.

Gotcha. It may have been more appropriate to write NullableType than ReferenceType. In my opinion, the least astonishing behavior of as would be to produce null without a runtime error for a mismatched type when the target type is nullable, and trigger a runtime error (and an UNSAFE_CAST error) only if the target type is not nullable. In combination with the addition of nullable types from that aforementioned PR, I feel that this would be a quite clean solution.

If you have a suggestion for improving the warning message, I would appreciate it.

I think my primary suggestion would be to elaborate in the description of the project setting. So perhaps changing from:

When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a [Variant] value is cast to a non-Variant.

To something like:

When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a [Variant] value is cast to a non-Variant using the [code]as[/code] operator. However, in this version of Godot there is no way to convert a [Variant] value to any other value that is recognized as safe.

I think that would help to prevent confusion for users initially discovering the setting. But for users who already had the setting turned on without fully understanding its behavior, perhaps it could help to change the message from:

Casting "Variant" to "%s" is unsafe.

To something like:

Casting "Variant" to "%s" is unsafe. There is no safe way to convert or cast a Variant to another type.

But: If there is any room for small feature additions yet in 4.3, I think it would go a long way to add typed conversion functions that are like type_convert, but the functions are per-type, with an appropriately typed return value. If functions like this were available, then the message could instead suggest the use of these other functions as a safe means of Variant conversion instead of as.

In any case, I made a proposal to more formally suggest this addition: https://github.com/godotengine/godot-proposals/issues/10320

[theDoktorJ]

Ok, so I'm running into this issue as well. I must admit, the discussion above is way above my head. I am simply trying to cast from Variant to a known type. After checking with the is keyword that my variable is the correct type. How is that unsafe? I can absolutely understand that doing so without checking first is unsafe, but in prior versions, checking with the is keyword seemed to nullify this error, as it should be.

[pineapplemachine]

@theDoktorJ I recommend turning off the "Unsafe Cast" warning by setting this option to "Ignore" in your project settings.

To try to TL;DR it a bit: Right now the Unsafe Cast warning is of questionable helpfulness. The main reason for this is that it's not yet smart enough to avoid very significant false positives, reporting problems where a human programmer can see that there clearly aren't, like the example in your report #95267 where you were already checking for the correct type using is.

If you really want to keep the warning on in project settings instead of setting it to Ignore, you can fix the warning message by removing the explicit cast in your example, i.e. by changing var test_2: TestResource = test as TestResource to var test_2: TestResource = test. But I recommend turning off the warning. It currently doesn't do anything except warn you for casting explicitly (with as) instead of implicitly (without it). Warnings and errors for other kinds of invalid casts are not controlled by this setting, just this one thing with Variants and as.

[theDoktorJ]

Thank you, that was extremely helpful. I will just disable it for the time being.