Godot: str2var crashes the application when processing a non-convertible string - not very useful behavior

[blurymind]

A common use case for str2var for me is to have it go through a collection of string variables and have it convert them to the appropriate type.
This however becomes a problem, as str2var will crash godot if it encounters a variable that is supposed to be a string in the first place.

I think it would be more useful if the function simply returns the original input string in that case, rather than crashing the engine.

I made an ugly workaround function to get around that- just to stop it from crashing:

func cusStr2Var(inputString):
    if !isItAString(inputString):
        return str2var(inputString)
    else: return inputString

func isItAString(string): ##fixed
    var NameRegEx = RegEx.new()
    NameRegEx.compile('[A-Za-z]+')
    if NameRegEx.search(string) != null:
        return true
    else: return false

Its messy, might have to rewrite it, the idea is to avoid crashing

But would really like it if I didn't have to :)
If you want to keep the crash message as to why it failed to convert it, why not turn it into a warning instead?

[blurymind]

One thing that could be nice is to at least have it return a null instead of blowing up. That way I can avoid using a regular expression to guess

[vnen]

For one side it's good to have it error, since it can avoid bugs. OTOH this current behavior makes it impossible to recovery, since Godot offers no error handling.

I don't like returning the original string, it makes impossible to know if it was a success or not. TBH this is also the case for null, it's totally possible to the original value was a Null since typeof(str2var('null')) == TYPE_NIL is true.

Also note that for a string to parsed correctly, it needs to be enclosed in double-quotes: print(str2var('"This will print nicely"')).

[blurymind]

@vnen I agree that having some kind of error feedback is nice to have, but can't we make it a warning rather than a crash somehow? In many cases the extracted text that I am running through str2var doesn't have the the extra quotation marks and the current design breaks it. When you put it that way- yes the current design does make alot of sense, but it is not convenient- especially when gdscript by design has avoided a 'try' command like the one that can be found in python.
So there is no way for me to catch the code exploding during the str2var operation - I am forced to mess with the data before it gets to it. If we cant return anything to indicate that it failed, can we at least have it not crash godot?

How about this, what if we pass an optional parameter to return something if it fails:
str2var(inputVar,returnIfFailsVal)
so example use can be

var myVar ="this might blow up"
str2var(myVar) # explodes as usual, so we haven't changed the current design
str2var(myVar ,null) # returns null instead of exploding 
str2var(myVar ,myVar )# returns original input instead of exploding 

This will definitely solve it for me and save me lots of patch code and headaches.
I can't think of anything else to suggest, perhaps someone will have a better idea to approach this?
What do you think :)

[willnationsdev]

I'm not quite sure of the internals, but wouldn't passing no second parameter end up being equivalent to passing a null second parameter? The parameters don't end up in an array where you can distinctly count the size. They ultimately call a C++ function with actual Variants as parameters. You might have to completely rewrite strtovar to account for that, and even then, you'd have to put it under a different name to avoid naming conflicts since I don't think function overloading is an option here(?).

[blurymind]

@willnationsdev the general idea is to optionally prevent godot from crashing when a non-convertible string has been passed to it

The second optional parameter could just as well be a simple boolean to tell it that some of the information that is being parsed to it might be intended to be a string to begin with.

So if it can't convert it to something, at least don't crash- it could just show a warning in the debugger instead

str2var("please dont crash",silent=false) ## silent bool - when true- str2var will allow non convertible strings pass through without crashing the entire application

Anything is better than having it crash the entire process imo. The current behavior leads to this becoming a bug in the application to the end user - in some corner cases

[willnationsdev]

I'm not necessarily opposed to changing things to prevent a crash, but I don't think a second parameter is the answer in this case. A more efficient solution might be to have it return an ERR_* integer in case of failure. Doesn't seem as though it would be that difficult to implement. Granted, I don't know how it's string parsing works or whether it can be evaluated in advance, prior to the crash check.

[blurymind]

@willnationsdev I think we talked about that solution with @vnen and he brought up a very good point that str2var returns different type of variables and it would be a problem if it returns anything when failing- I suggested that it returns a null.
Your proposal is similar and has the same problem - if it returns an error integer, there is still a good chance that its the same value as an integer that you are converting from a string with it!

Suggestions

Option A:
I think that it should simply return the original string by default- as it detected that it is a string that can not be converted and also give some sort of a warning in the debugger that it failed to convert it to anything- instead of crashing the entire application. Anything but crash the application - the crash is the main problem.

We could check if it failed by comparing the original string with what it is returning:

var myInputVar = "this is a string, dont crash"
if str2var(myInputVar) == myInputVar: ##might crash when comparing two different variable types though
  print("I will return the original string, but note that I failed to convert it")

else:
 print("I will convert the original string to another type")

But the check is not what I'm after here, as much as preventing the crash from even be possible

Option B:
If developers want to keep the crash behavior, then at least add an optional parameter for those of us who would like to disable it and use the behavior in Option A

[willnationsdev]

Hmm. I wonder if you could just test whether wrapping the content in double quotes creates a successful conversion, and then if so, return that. I haven't looked at the source code, but that would be an interesting approach. If you did it as a last resort, then you'd be able to just supply raw text to it and it would automatically output it as a string because it failed to match it against the string syntax and all of the other object syntaxes

[vnen]

Actually, returning the original string sounds like the best solution. You can compare the input and the output and if they're the same it means the parsing failed.

[willnationsdev]

If that's the case, the error might be fixed with modifications to core/variant_parser.cpp, line 1255.

} else {
    r_err_str = "Expected value, got " + String(tk_name[token.type]) + ".";
    return ERR_PARSE_ERROR;
}

...and subsequent changes to _parse_array and _parse_dictionary. Just my guess since I'm not that familiar with the code.

Mind, the original call happens in gdscript_functions.cpp, line 700. There are also references to "str2var" in modules/visual_script/visual_script_builtin_funcs.cpp and modules/mono/editor/bindings_generator.cpp, among other places.

[vnen]

No, I think it should be changed only for GDScript (since this is a GDScript-only function), so just change this if block to just return the original argument instead of creating an error:

https://github.com/godotengine/godot/blob/1fa9aac3e415f53a095e955c8a37000629d56dde/modules/gdscript/gdscript_functions.cpp#L702-L708

People using the VariantParser directly can detect the error themselves.

[vnen]

The only problem with this change is that if people don't check the return value, they might let a parsing error slip by, so I'm still not sure if this is wanted. str2var() is intended to be used with the result from var2str(), it should never fail.

[blurymind]

@vnen Godot's debugger is capable of printing warning messages - when for example a signal is fired to run a non-existent function - the application will continue running but a warning will be printed in the debugger. Why not use a warning instead of the termination?

Alternatively it could be optional to run str2var with a parameter to disable it from terminating the application and the current behavior be kept as the default.

str2var("This is a string") ## still crashes the application
str2var("This is a string",true) ## silent=true so instead of crashing the application, the original string  is returned

It would be great to be able to just stream a bunch of strings to str2var and not be worried that one of them will crash the application. In many cases returning the original string will be more beneficial than trying to catch it from getting passed to str2var.

[Anutrix]

Was this fixed in #22934?

[akien-mga]

Should be yeah, closing as fixed by #22934. Please comment if you can still reproduce it.