Improve error messages for invalid property access/calls in GDScript #46241
Conversation
The reversed order of the messages now matches Python. See godotengine#46214.
| @@ -2186,7 +2186,7 @@ void GDScriptAnalyzer::reduce_identifier_from_base(GDScriptParser::IdentifierNod | |||
| } else { | |||
| switch (base.builtin_type) { | |||
| case Variant::NIL: { | |||
| push_error(vformat(R"(Invalid get index "%s" on base Nil)", name), p_identifier); | |||
| push_error(vformat(R"(Value of type Nil does not have the attribute "%s".)", name), p_identifier); | |||
There was a problem hiding this comment.
Can we have information about what identifier is of type Nil? Or would that be explicit enough from the stacktrace?
There was a problem hiding this comment.
Wonder why the keyword literal is null and its name is nil. They have different meanings in some other languages (like Objective C, Scala). In general, NULL is 0 or '\0' in ASCII and Nil is emptiness. (I'd like it as null)
There was a problem hiding this comment.
We discussed this in a GDScript meeting and suggest indeed referring to null directly. Maybe this could be:
| push_error(vformat(R"(Value of type Nil does not have the attribute "%s".)", name), p_identifier); | |
| push_error(vformat(R"(Could not get attribute "%s" on a null value.)", name), p_identifier); |
There was a problem hiding this comment.
Or the same phrasing as used in your VM changes to be consistent.
There was a problem hiding this comment.
Or the same phrasing as used in your VM changes to be consistent.
Which changes are you referring to? I may have forgotten something 🙂
There was a problem hiding this comment.
I mean the sentence you used for the null case in gdscript_vm.cpp:
err_text = "Tried to access attribute " + v + " on a value that is null.";
|
Looks good to me from a quick review. Would be nice to change in |
|
This PR works as expected, but when using an invalid index on array, the wording doesn't make much sense:
I'll need to detect whether the type is an array or PackedArray and display a different message in this case. We can also use this opportunity to display the number of elements in the array in the error message 🙂 |
| if (!src->is_null()) { | ||
| err_text = "Tried to access attribute " + v + " on a value that is null."; | ||
| } else { | ||
| err_text = "Value of type '" + _get_var_type(src) + "' does not have the attribute " + v + "."; | ||
| } |
There was a problem hiding this comment.
Isn't the condition backwards here?
|
I do not think we use the word "attribute" for these things, I would discuss with @vnen what works bet |
|
Superseded by #66948. |
Marking as draft since I haven't had time to test this yet (especially the
nullcheck).The reversed order of the messages now matches Python.
See #46214.