godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix error spam when naming a func at the end of the script

Open MewPurPur opened this issue 1 year ago • 5 comments

An attempt at fixes #63946. At least I couldn't think of something better.

First tackling with the parser, so an experienced review would be neat. To my understanding, here's what's going on:

As you're writing the name of a function at the end of a file, you have:

  • Coke: func causes multiline parsing, since brackets must be able to be parsed even if they are on different lines. Because of this, the parser considers that the next token is EOF.
  • Mentos: func can declare a function or a lambda, so there doesn't exist a prefix rule and the expression has to advance here to avoid an infinite loop.

So when these properties of func mix together, you get an explosion of errors.

I'm thinking there might be a nicer way to do this than a condition for expression statements. If you GDScript folk come up with a better but far more complex way, I'll probably not implement it because I'd need too much handholding.

MewPurPur avatar Feb 16 '23 04:02 MewPurPur

For context, this is fixing a regression from https://github.com/godotengine/godot/pull/62900. CC @cdemirer

akien-mga avatar Feb 16 '23 07:02 akien-mga

I'd say there are good reasons to assume the parser would never look for a statement at the EOF token because in all expectable cases there would be some non-EOF token after where the statement would be. Even if a statement would be the last thing at the last line of a script without indentation, there would still be a newline token after it because the tokenizer always inserts one at the end as necessary.

Apparently this assumption can become false when the parser continues to look for errors after the first, but fails to synchronize properly (as in the synchronize() function). As mentioned by @MewPurPur, parse_function consumes the newline before the EOF. But the parser continues in a bad state and attempts to parse statements for the function body.

I think it makes sense to have an is_at_end check in parse_statement. Though I think it might be better to have it at the top and to return early. IMO the root cause of the issue is more relevant to parsing (at the wrong time) statements in general than expression statements specifically.

  • Ideally it would also print an error about the parser attempting to parse a statement at EOF, but that would bring us back to the error spam problem.

The error is only "spammed" when saving / loading the project. It seems that the script is parsed multiple times in a row for some reason, thus having the error spammed.

Besides the spam issue, I think the error is somewhat accurate and the proper fix is probably fixing the synchronization of parse_function. There seems to be a somewhat related TODO here: https://github.com/godotengine/godot/blob/ff641463f70462085dac8098534360528a5f8df6/modules/gdscript/gdscript_parser.cpp#L1371

cdemirer avatar Feb 16 '23 16:02 cdemirer

Yeah, errors should only happen when the script is saved in an invalid state, but this is a bug in the parser so it happens as you're typing. I said "spammed" because it happens every time you add a letter to the function name, so for example if you type func abcdef slowly, it will throw the error at least 6 times (for me it happens up to 18 times, I'm not sure why)

So the is_at_end() check is fair and I should move it earlier? Makes sense, will do it soon.

MewPurPur avatar Feb 16 '23 17:02 MewPurPur

@cdemirer Where do you suggest to place the check? Even before the switch?

Edit: Doned.

MewPurPur avatar Feb 17 '23 01:02 MewPurPur

I think it should not even try to call parse_statement() in this case. IMO the check should be in parse_suite() before doing the call to parse_statement().

The synchronization definitely can be improved but this error spam did not exist before so it's not exactly related.

vnen avatar Mar 07 '23 13:03 vnen

Thanks!

akien-mga avatar Mar 08 '23 08:03 akien-mga

Cherry-picked for 4.0.1.

YuriSizov avatar Mar 14 '23 12:03 YuriSizov