[Autocomplete] Avoid prepending literals when the character has already been typed

[Thought-Weaver]

When trying to autocomplete node paths and string literals in a script editor when the relevant editor settings were enabled, if you had already typed ^"" or &"", autocomplete would insert another character to make it ^^"..." or &&"...", causing a syntax error.

The autocomplete functions weren't aware of where the script was being autocompleted, so even though the string of code itself was passed, there wasn't a check to validate whether the literal was already prepended or not.

Caret.Fix.mp4

Fixes: #107758

[Thought-Weaver]

This is my first time working in Godot's codebase! Any and all feedback on this approach to refine it further is appreciated. Thank you in advance!

I'm especially wondering about the most appropriate type for the script language extension is, since default ints aren't supported, and if there are any other cases where it might benefit to have this context so the editor can be cleverer about autocompletion.

EDIT: Thanks for the feedback! Now we don't have to worry about the ints being passed around and will instead do a lookup for the cursor character each time instead.

[HolonProduction]

I don't think that doing additional checks on the code is a good idea. Autocompletion should work with the parse tree instead. In the cases were we do not want the additional path char there has to exist a literal node already so the check should just be based on completion_context.current_node being a node path or string literal.

Edit: Why is this better:

• the node gives us a view into the code that is far more stable than relying on own analysis of the line
• the node gives us positions of the literal, this is not important at the moment, but it will be in the future once we transition the LSP to calculate the insertion diff completely on the server side

[HolonProduction]

This has nothing to do with the auto completion issue. And at least in 4.4.1 dropping a property in between quotes does still drop the additional quotes from the property so having this content aware is no feature at the moment. It should be tackled in a separate PR IMO

[Thought-Weaver]

Trying to use the parsed expression for the node path was the first thing I tried! But it seems the prepended symbol is being discarded -- it didn't appear as part of the string when converting the value of the node. It seemed much less risky to make a change to how autocomplete worked as part of the code editor than to make a change to GDScript's parser.

[Thought-Weaver]

Ah wait, this is probably because in my first attempt it looks like I used the reduced expression rather than guessing the expression type and using the value. If that works, I'll definitely switch over -- I'd much prefer to use the parser's context.

EDIT: No, that doesn't seem to have worked either -- it's still missing from the node's value when guessing the expression type of the argument passed to the call.

[HolonProduction]

You should have a LiteralNode which contains an arbitrary Variant value. You should be able to check the variant type of this value to get the required info.

If that does not work for some reason you can still use the location information of the node to check whether the source code contains the characters you are looking for.

Consider this case: (cursor is |) ^"a|" now your spitted line does not begin with ^" anymore. With your current approach you will end up writing a second string parser if you want to do it correctly. That's just not feasible since GDScript strings are complex. Take for example """ ^"| """.

[Thought-Weaver]

Perhaps I'm missing something, but in various attempts to evaluate the value of the argument in the call function, it hasn't included the character -- which I've found a bit odd because I agree that the literal should be the actual value of the string that the parser has parsed, unless there's a good reason to discard the prepended character:

if (_guess_expression_type(p_context, p_call->arguments[p_argidx], value)) {
	if (value.type.is_constant) {
		const String &literal_value = value.value; // Doesn't contain '^'
		if (literal_value[0] != '^') {
			opt = "^" + opt;
		}
	}
}

Is there another method I'm not seeing or a means of accessing the literal value directly? I fully agree with all your points, but like I said, I tried doing it that way initially and ran into these same issues.

String complexities could be avoided by using the current argument value for a more complex search, but that gets even deeper into the territory of relying on context from the code string itself.

[HolonProduction]

You don't have to guess any types here. String literals are already resolved to their runtime value by the tokenizer. So what you should be doing is something like (untested, actually location of the enums and such might be slightly different but the idea should be clear)

// GDSP = GDScriptParser for sake of brevity
if(p_call->arguments[p_argidx].type == GDSP::Node::LITERAL) {
    GDSP::LiteralNode *literal = static_cast<GDSP::LiteralNode *>(p_call->arguments[p_argidx]);
    Variant literal_value = literal->value;
    if(literal_value.type == Variant::STRING_NAME) {
        ...
    }
}

Although I'd recommend to skip the roundtrip through a call node and use completion_context.current_node directly, since this logic has nothing to do with calls, it just so happens that we only have string completion in calls at the moment.

[Thought-Weaver]

I tried this approach as well, and it yielded the same result: The argument is a NodePath and the character isn't part of the literal value. Also, wouldn't the node from the parser context in the reported bug case in _find_call_arguments be a Call? Maybe there's something about the parser I'm misunderstanding.

I'll keep digging and see if there's a way to get the right value or context from the parser, but I'm not currently sure it's available.

[HolonProduction]

The argument is a NodePath and the character isn't part of the literal value.

Why is it important to have the ^ character as part of some value? The fact that the literal is a NodePath tells us that ^ is present in the source code. If the literal was a StringName the & character would be part of the source code and if it was a String none of those characters are present in the source code. Shouldn't that be enough information?

Also, wouldn't the node from the parser context in the reported bug case in _find_call_arguments be a Call?

Yeah my bad, sorry. Usually completion_context.node contains the node under the cursor, but in some completion types it's used differently. Seems like for call arguments it just contains the call.

[Thought-Weaver]

Ah, I see! That's what I was misunderstanding: I thought the argument type would always be a NodePath not realizing it'll be a subtype iff the character exists. Let me give that a shot -- thanks for the clarification!