GDScript: Allow quoteless strings as arguments in some annotations

[vnen]

For some annotation, strings arguments are validated. In such cases, we allow not found or not constant identifiers to be used as a string, because invalid values will raise error messages removing the problem of typos.

It also allows type names to be used as strings if the argument supports it.

This commit applies it to @rpc, @warning_ignore, and @export_node_path (the latter now validated the arguments to be valid Node types).

Incidentally, this also improve completion for @export_node_path by
also adding custom nodes to the list.

[dalexeev]

Sorry, but I don't understand what problem this solves. There is an explanation in #71634 why this is inconsistent and confusing. There is no reason to prevent using a constant as an annotation argument (except for the @icon annotation, which must be resolved in the parser, and therefore its argument must be a string literal, but quotes are still required).

Strings in annotations have no performance impact, unlike method/signal strings in 3.x. Moreover, you are not replacing strings with something else, but only hiding the quotes in some cases, forcing the identifier to be treated as a string.

The @warning_ignore and @rpc annotation arguments already have autocompletion and validation, we can do the same for @export_node_path.

It also contradicts the @export_node_path signature:

If we had a Type type (first class types), then this might work.

[vnen]

Sorry, but I don't understand what problem this solves.

One of the big things in Godot 4.0 was the lesser reliance of strings (mostly signals and methods, which now can be used without strings). The main point of it is validating at compile time instead of trying to figure out why something does not work only to realize you made a typo.

Once constant expressions (and thus constant identifiers) was supported, it made sense to require quotes because otherwise typos would be a big problem. With this PR, we have actual validation in some cases, so the quotes are not necessary. Being quoteless might not mean much, but at a glance it is more obvious that the value is significant and just any arbitrary string.

I actually remember some backlash for that change by requiring quotes on some annotations that accept only a handful of possible values. It is not pretty.

There is an explanation in #71634 why this is inconsistent and confusing. There is no reason to prevent using a constant as an annotation argument (except for the @icon annotation, which must be resolved in the parser, and therefore its argument must be a string literal, but quotes are still required).

But this does not "prevent using a constant as an annotation argument". You can still use constants. The only difference is that if the name does not exist it is treated as a string. Which is fine because it's validated.

The only argument against this, which would be option 4 in that discussion, is the following:

would mean that introducing a new constant or variable could change the behavior of an unrelated part of your code just because you happened to name something the same. I can see this happening if somebody uses @export_enum(DOG, CAT) var companion and later defines a const DOG = "Stella", for example.

Yes, this would be the case but that's why this PR introduces this behavior only for annotations that limits the string argument to a known list, which means the particular example wouldn't happen, since @export_enum would still require the quotes.

Sure, you can still do something like this:

const call_local = "call_remote"

@rpc(call_local)
func foo():
	pass

And that would be pretty odd. But that's a very far fetched example, I doubt it would cause problems in actual projects. If anything, this could be made a warning as well.

Strings in annotations have no performance impact, unlike method/signal strings in 3.x.

This has nothing to do with performance, it is about usability. Signals/Callables are probably less performant because it has to construct in the extra object and in the end is still just storing a string to pass to the connect/call.

Moreover, you are not replacing strings with something else, but only hiding the quotes in some cases, forcing the identifier to be treated as a string.

The point is not replacing, just providing a better interface for the users. It does not matter if it's a string in the end, what is important is the presentation.

It also contradicts the @export_node_path signature:

If we had a Type type (first class types), then this might work.

Well, I don't think we have to wait until a proper Type type is implemented before we offer this usability improvement to the end users. In particular for @export_node_path that can only have Node types as argument, requiring quotes is quite annoying to say the least. And given it can be any arbitrary string, it does not prevent typos (it does with this PR, with or without quotes).

It does not make sense to accept an arbitrary string to something that has to be the name of a class that inherits Node. It makes more sense to have the name of the class itself. Since it has to be validated, the quotes become extra, they are not needed. The highlighting in the editor also makes it more interesting and easier to see it is a class name. Also, since they are global names, you cannot create a constant with the name of an existing class which would change the behavior of the code.

[vnen]

PS: About @rpc, ideally they would be an enum value. But creating an enum for this would be redundant, and using MultiplayerPeer.TRANSFER_MODE_UNRELIABLE would be quite annoying. Similar for @warning_ignore. Annotations can and do have different rules, we should not be tied up by some self-imposed restriction.

[dalexeev]

It's hard for me to argue with your authority, but for me this still does not solve any real problem and does not increase convenience. It's just a "let's remove the quotes in a few random places because they look ugly" argument. This makes things inconsistent and confusing (even if it seems minor at first glance).

The main point of it is validating at compile time instead of trying to figure out why something does not work only to realize you made a typo.

String completion and validation is possible and already available for @warning_ignore and @rpc, regardless of quotes. If something can be improved (including @export_node_path), then we should do so rather than remove the quotes instead.

One of the big things in Godot 4.0 was the lesser reliance of strings (mostly signals and methods, which now can be used without strings).

In the case of methods and signals, we achieved this in a different way: we added new Callable and Signal types. We have not made, for example, the quotes of the first argument of emit_signal() optional.

The only difference is that if the name does not exist it is treated as a string. Which is fine because it's validated.

It's still an inconsistency, even with these restrictions. But most importantly, I don’t understand why this is needed, apart from saving a couple of characters and subjective preferences. I would prefer consistency and uniformity wherever a significant gain in convenience has not been proven.

Also, in my opinion, this is bad because of the addition of unnecessary context: the same identifier is interpreted differently depending on the place where it is placed (in one place it is a "quoteless string", in another it is a real identifier). Imagine that we don't have a separate Lua-style dictionary, and we treat unknown identifiers in keys as strings, or generally treat non-existent identifiers as strings (old PHP versions have this behavior).

Annotations can and do have different rules, we should not be tied up by some self-imposed restriction.

If annotations were a "thing in itself" (like the export syntax in 3.x), then I would agree with you. But since an annotation argument is an expression, I think it must follow all the rules of expressions (with the additional restriction that the expression must be constant). Nowhere else can you omit quotes without changing the meaning of the expression.

[vnen]

The benefit is cosmetic but I believe it's a benefit nevertheless. Consistency is not that important, especially not in detriment of other improvements.

As an illustration:

Having CustomNode highlighted there makes you understand this is a class name, not an arbitrary string. You can even see the difference between custom and native classes because they have a different color:

Also, even if in the future we introduce a core Type abstraction, technically we cannot change this annotation because it would break compatibility. IMO having this midway would be a good compromise.

[dalexeev]

Having CustomNode highlighted there makes you understand this is a class name, not an arbitrary string.

In my opinion, language behavior should not be based on highlighting in the editor. In the future, we may add highlighting for "strings with special meaning" if we deem it important.

To me, expression consistency (no conflicts between string literals and identifiers) is more important than highlighting in the editor.

The only place in GDScript where "quoteless" strings also exist is in the keys of Lua-style dictionaries. But unlike this PR, there is no conflict, it's part of the syntax, "quoteless" string cannot appear in the main flow of an expression. This is generic behavior, with no special cases. If I give abstract examples of dictionaries, you can easily figure out where the key is an identifier, and where is a string:

{x: 1, y: 2} # x and y are identifiers x and y (variables, constants, etc.).
{x = 1, y = 2} # x and y are strings "x" and "y".

But if I give an abstract example of an annotation, you can't tell if an argument is an identifier or a string without knowing what the annotation is and what the argument is:

@a(x, y) # x and y are identifiers or strings?

I expressed my opinion and concerns. Let's take this to the next PR review meeting.

[vnen]

But if I give an abstract example of an annotation, you can't tell if an argument is an identifier or a string without knowing what the annotation is and what the argument is:

@a(x, y) # x and y are identifiers or strings?

Well, an "abstract example" wouldn't fall in the rules from this PR, since I add only for a few cases where the arguments are validated and not for any arbitrary string, so I don't think this is a fair comparison. In this abstract example I would say they are identifiers, because they do not have obvious meaning and by default annotations do not allow quoteless strings.

OTOH, with a concrete example:

@warning_ignore(return_value_discarded)

It's pretty clear what the argument means. You wouldn't really question if this is an identifier.

Another way to see this: return_value_discarded is not a string. It is an identifier that is defined in a scope accessible only by the @warning_ignore annotation.

[adamscott]

Moved the 4.1 milestone to 4.2, and added it to the list of PRs to team review.

[dalexeev]

I tried to be objective, but I would recommend re-reading the thread before the meeting, if possible. This summary is just an attempt to help with the discussion.

Summary

Pros

One of the big things in Godot 4.0 was the lesser reliance of strings (mostly signals and methods, which now can be used without strings). The main point of it is validating at compile time instead of trying to figure out why something does not work only to realize you made a typo.

I actually remember some backlash for that change by requiring quotes on some annotations that accept only a handful of possible values. It is not pretty.

1. Argument autocompletion (usability).
   • Counter-argument: This can be implemented without removing quotes (and is already implemented for other annotations).
2. Argument validation (protection from accidental typos).
   • CA: This can be implemented without removing quotes (and is already implemented for other annotations).
3. Different syntax highlighting built-in/custom types.
   • CA: Is it really important to us? GitHub and external text editors won't be able to highlight the difference. Highlighting should depend on the language syntax, and not vice versa.
4. "Looks better" for "enum" and type arguments (look like pre-existing identifiers, not arbitrary strings).
   • CA: Beauty is subjective. The quotes here do not take up much space and do not take up much time.
5. In 4.x, we've dropped strings in a lot of places, thanks to first class Callable and Signal. It would be more consistent to drop quotes where possible.
   • CA: Subjectively too, removing quotes should not be a goal in itself. There are no functional differences (in performance, autocompletion and validation), these are the same strings, only without quotes. They are not real constants and not a special type.

Cons

1. Conflict between identifier and quoteless string. You are forced to add + "" to force the identifier to be interpreted as identifier.
   • CA: Probably such a conflict is unlikely in practice.
2. Inconsistency. This does not work for all annotations and arguments. You can't tell by looking at the abstract example @annotation(argument) whether argument is an identifier or the string "argument". This is a special case that we will have to document and support.
3. Inconsistency. Annotation arguments support expressions (even if only constants), not just literals. Nowhere else in the context of an expression is there such a double interpretation of one thing. There is Lua style dictionaries, but this is a special syntax so there is no collision.
   • Note: If annotations didn't support constant expressions, but only literals (as they did before 4.0 beta 16), then this would make sense.
4. Formal inconsistency of the unquoted type string with the annotation signature. The argument must be a String (not a hypothetical Type, Object, or Variant), and we're passing a pseudo-GDScriptNativeClass or pseudo-GDScript (but it's actually still a string).
   • NB: This can confuse users if they try to pass a const preloaded script as a type argument.
5. Users now have a choice: to use quotes or not? We don't have the "Zen of GDScript" that says "There's only one way", but we should still think about it. What is the meaning of several options, which ones should we recommend and why.
6. Users are used to the current syntax. There were a lot of "why annotations are broken" questions during the beta phase, but after all users updated their projects, I didn't see any new "why annotation quotes are required" questions.
7. If we add custom annotations in the future, will there be any problems?

[adamscott]

Discussed in a GDScript meeting. Mixed reception, the group don't really support the idea of "exceptions" for some annotations (quoteless strings), but not others. And there's already validation for wrong values some annotations, ie. @rpc

Thanks for the PR nonetheless, @vnen !