Fix GDScript parser sometimes crashing when issuing warning for unreachable pattern

[MinusKube]

Fix parser crashing when trying to issue a warning for an "unreachable pattern" of a match branch with no pattern (this can happen when the pattern has a parse error).
Fixes #62409

[cdemirer]

This seems to prevent the specific issue (and probably good to merge). But the underlying cause of the issue is a bit horrifying. The parser is very greedy when parsing match statements. When the repro instructions in the issue are followed, the parser tries to parse line 140 (var category := ...) (which is supposed to be outside the outer match) as a bind pattern for the outer match, which is why has_wildcard_without_continue is true, and the crash happens when trying to parse some other line below as yet another branch...

Is there a way to prevent this? All the DEDENTs get consumed by the failed multi-line parse_expression, so there's no way for parse_match to know about them... So the only way seems to be to always check for null/empty stuff.

[cdemirer]

It looks like this also fixes #57711

[Spartan322]

Isn't this more so hiding the underlying reason for problem then specifically correcting it? Are we sure this issue won't crop up in other places?

[MinusKube]

Isn't this more so hiding the underlying reason for problem then specifically correcting it? Are we sure this issue won't crop up in other places?

Well, I believe there are actually two issues:

• The one that this PR solves, which can be easily reproduced with this code:

var test = "hello"
match test:
	_: # have_wildcard_without_continue will be true for next branch
		print("Anything")
	: # No pattern for this branch

• The underlying "greedy parser" problem, for which the crash is fixed by this PR (at least in the two linked cases, but maybe in some cases there are other crashes), but which is not really solved here

[Spartan322]

The underlying "greedy parser" problem

Yeah and that's what I was thinking, I don't know about any of the maintainers, but I'd personally feel safer to at least note down this greedy parser problem in another issue as well, (I'd even be willing to do that, but I don't know the parser that well and as a result I'm not sure if I can explain the problem well enough) least would give us hope that its marked down to be fixed, unless it already is noted down but I can't seem to find anything about that. This is a horrifying issue as cdemirer said and it means that there might be other places where this can crop up when semantically it makes no sense.

[cdemirer]

I think the "greedy parser" may not be that much of a problem.

1. The parser should never crash for any input.
2. When the parser skips some chunk of tokens as a result of a Parser Error, the calling function "sees" a different source code than what the user intended. It still shouldn't crash, since the user could technically have written that code in the first place.
3. The most (2) can cause is many (meaningless) errors being generated like a chain reaction. Running unexpected script logic is not a problem because it won't compile/run until all errors are dealt with anyway.
4. The positive side is that it helps find "defects" in the parser by triggering crashes, as in this case. As @MinusKube's repro shows, the crash could be triggered even with "valid" code.

(3) is where the "continue parsing anyway" approach has a trade-off. In most cases, it helps notifying the user of multiple "actual" mistakes in the code at once, instead of finding and showing them one-by-one.

[Spartan322]

Something to be fair about, its fine for the parser to be greedy in places where it makes sense, my concern is mostly where the parser shouldn't be greedy in places it doesn't like mistaking a statement for an expression, or in the example above trying to parse an empty pattern as though it were valid, in this specific case it should be entirely capable for the parser to recognize that the semantics are wrong without crashing and try to move outside the section perhaps, especially when the crash is only incidental thanks to a warning, even in failing later this is a problem because the root of issue is still rather hidden. The standard should be to fail early and to fail predictably, preferably as said in a manner that doesn't crash the system, in which case the parser could perhaps skip over the unknowns until the point it knows something that could be valid. If we crash early, it won't in the least take any debugger forensics to figure out the exact reason for the problem. (beyond a basic stack trace I mean)

[vnen]

Crashing should never happen and in many cases the solution is really checking for empty stuff before accessing. I've been thinking of ways to mitigate this, such as having empty nodes instead of nullptr for instance. But this as discussion for elsewhere.

Regarding the parser being "greedy", that's not much of an issue because it's only problematic when the script is invalid. The idea is that it needs to keep going so it can catch more errors instead of first reporting the first error it found. It might cascade into nonsensical errors, but that's less of a concern. I'm not saying it's perfect, but it's a system that can be improved, it doesn't need to be removed.

Feel free to open a discussion about this subject if any of you want to.

[vnen]

Thanks!