GDScript - Static typed String/StringName with type checking will result an error

[ydipeepo]

Godot version

v4.0.beta1.official [20d6672]

System information

It's not system specific error.

Issue description

If it is cast implicitly, it would work in all cases, but it seems that in some cases the conversion between String and StringName does not match the actual type.
I think the type check is not working properly. By the way, casting by as worked well.

Steps to reproduce

1. Assign StringName value to statically typed String var

or

1. Assign String value to statically typed StringName var
2. Checking the type with is
3. An error is displayed: Expression is of type blabla so it can't be of type blabla

Minimal reproduction project

func _ready():
	var istr1 = StringName("bbb")
	var mstr1 = String(istr1)
	print(istr1 is String)
	print(istr1 is StringName)
	print(mstr1 is String)
	print(mstr1 is StringName)

	var mstr2 = String("ddd")
	var istr2 = StringName(mstr2)
	print(istr2 is String)
	print(istr2 is StringName)
	print(mstr2 is String)
	print(mstr2 is StringName)

	var istr3 = StringName("ccc")
	var mstr3: String = istr3 # assignable
	print(istr3 is String)
	print(istr3 is StringName)
	print(mstr3 is String)
	#print(mstr3 is StringName) # but edit time error

	var mstr4 = String("eee")
	var istr4: StringName = mstr4 # assignable
	#print(istr4 is String) # but same error
	print(istr4 is StringName)
	print(mstr4 is String)
	print(mstr4 is StringName)

[gotnospirit]

The "incorrect" lines for the first two blocks are marked as unsafe (line number in gray)
Maybe implementing the warning would make things more obvious

godot/modules/gdscript/gdscript_analyzer.cpp

Lines 2178 to 2179 in 24115be

	// TODO: Warning.
	mark_node_unsafe(p_binary_op);

The problem for me is the assignment lines, they are not marked as unsafe and don't throw any error.
Even

	var istr3: StringName = StringName("ccc")
	var mstr3: String = istr3 # assignable

works while an error is thrown for "istr3 is String" test

[gotnospirit]

My bad, StringName<->String are valid for assignments

godot/core/variant/variant.cpp

Lines 578 to 583 in 24115be

	case STRING: {
	static const Type valid[] = {
	NODE_PATH,
	STRING_NAME,
	NIL
	};

[gotnospirit]

After more thoughts about that, everything seems normal.
Static type is aimed to make errors before runtime.
The conversion between StringName & String are conversion of the value, not the type (it would be strange to silently change the declared type).

So I think it just that unsafe lines are not very obvious (not sure if there's documentation about what the color means).
Maybe a tooltip instead of a warning would be enough

[ydipeepo]

Sorry for the late reply. Thanks for the strong matters addition.
It really helped me a lot to understand this issue.
Based on it, I have rewritten it a bit to organize the issue.

	var untyped_imm = StringName("SN")
	var untyped_mut = String("N")
	print(untyped_imm is StringName) # OK
	print(untyped_imm is String)     # OK
	print(untyped_mut is StringName) # OK
	print(untyped_mut is String)     # OK

	var typed_imm: StringName
	var typed_mut: String
	typed_imm = get_name()
	typed_mut = typed_imm

	# Below 2 lines are worked perfectly.
	# And typed_imm and typed_mut appears to be cast correctly.
	print(typed_imm, typeof(typed_imm), "=", TYPE_STRING_NAME)
	print(typed_mut, typeof(typed_mut), "=", TYPE_STRING)

	# These four lines should issue a warning, not an error.
	print(typed_imm is StringName)  # No error/warning.
	#print(typed_imm is String)     # Yea, error thrown at edit time.
	#print(typed_mut is StringName) # Yup, same as above.
	print(typed_mut is String)      # No error/warning.

So I think it just that unsafe lines are not very obvious (not sure if there's documentation about what the color means).
Maybe a tooltip instead of a warning would be enough

I mostly agree, but the transparent casting of String<>StringName is very helpful,
so I thought it would be clearer to warn against like "unnecessary type checks on (primitive) typed var by is operator.".

GDScript Reference: Keywords
https://docs.godotengine.org/en/latest/tutorials/scripting/gdscript/gdscript_basics.html#keywords

Tests whether a variable extends a given class, or is of a given built-in type.

I thought it would be more appropriate to refer to the last four lines as a warning rather than an error or nothing happening.

[gotnospirit]

Maybe it's their name that is confusing but StringName and String are two separate things, they don't share the same inheritance tree.
This is why "is" produces an error when the variables have declared type.

print(untyped_imm is String)     # OK

is "OK" but still returns false at runtime

but the transparent casting of String<>StringName is very helpful,

it's not a cast but a conversion/copy of the value from one type to the other

[ydipeepo]

Yup, I am aware of that (cast/convert).
And now that I have a better understanding,
I would like to say that I hope these of all lines are throw warning throw warning or be correct:

	var typed_imm: StringName
	var typed_mut: String
	typed_imm = get_name()
	typed_mut = typed_imm

	print(typed_imm is StringName)
	print(typed_imm is String)      # Error at beta1&2, but it should be a warning or correct
	print(typed_mut is StringName)  # Same error at beta1&2
	print(typed_mut is String)

I modified the text and the code above a bit because it was not clear.
I thought maybe it should be a warning because it's doing redundant is checks on typed vars,
but now I can't run it before that. (beta 1, 2)

[vnen]

This is not a bug, it's intended. A variable of type String will never contain a value of StringName and vice-versa, so the type check is redundant and will never be true. The error is intended to avoid a logic mistake.

If you are considering the assignment, it will convert the value to the variable type when that is set, so it still is correct.

If anything, this could be change to just be a warning (although I would probably make it an error by default).

[ydipeepo]

@vnen Thanks for ur reply.
If so, but I think these 2 type checks is redundant too:

var typed_imm: StringName = ""
var typed_mut: String = ""
print(typed_imm is StringName) # always true
print(typed_mut is String) # same

[vnen]

Yes, the reverse case is the same (for builtin types at least), but it hasn't been implemented.

I do think those might be better as warnings, even if they are set to error by default. Won't be done for 4.0 release though, since that is too close.

[dalexeev]

As discussed in the GDScript team meeting (see #75170 (comment)), String and StringName are different types, neither is a subtype of the other, so type checking with the is operator returns false, just like the typeof() function returns different values.

However, these types are compatible with each other, meaning you can pass a String where a StringName is expected (and vice versa), just as you can pass an int where a float is expected; the value will be implicitly converted.

So you can use String and StringName interchangeably in most places in GDScript and the Godot API (see PR #68747). Other things, such as type checking, still distinguish the two types, it's not a bug. However, I suggest keeping this issue open for now and fixing it by a godot-docs PR.