search

godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.53k stars 151 forks source link

Fix errors in grammar syntax #416

Closed DaelonSuzuka closed 1 year ago

DaelonSuzuka commented 2 years ago

Per #415, this PR fixes some regexes that appeared to work in VSCode/onigumura but do not work in linguist using PCRE.

When this PR is done, I need to remember to post in github/linguist#3924 and let the linguist project know that we've fixed our grammars.

Additionally, @lildude has documented a process here that we can use to verify that these changes satisfy linguist's requirements. It's worth investigating if this process or a derivative can be integrated into our CI, which will at least provide syntax validation of our grammar files.

All three of the errors are invalid uses of positive lookbehind using a non-fixed length.

Problem areas:

I've already fixed the first problem, in the GDScript grammar, but unfortunately I've discovered that there was a regression sometime between 1.3.1 and now. The GDShader syntax doesn't even run, and there aren't any error messages about failing to load the grammar.

I'll have to find the cause of this regression before I can fix the two regex errors in GDShader.

DaelonSuzuka commented 2 years ago

I have absolutely no idea how this happened, but there were a ton of regex special characters that were missing the second backslash(they need to be double escaped because reasons). I recopied the grammar from @alfishsoftware's extension, fixed our handful of differences, and it's working again.

I replaced the invalid lookbehinds and it seems to work. Next, I'll run linguist's grammar compiler and see if we pass.

DaelonSuzuka commented 2 years ago

I followed the steps to set up linguist, ran script/bootstrap, manually replaced the grammars it download with the fixed versions from this PR, ran script/grammar-compiler add vendor/grammars/godot-vscode-plugin/, and I believe this is a positive result:

daelon@isotope:~/linguist$ sudo script/grammar-compiler add vendor/grammars/godot-vscode-plugin/
latest: Pulling from linguist/grammar-compiler
Digest: sha256:203514bbf04b83d3594245338fc08097301e54139ccec20ab05861302c23ab64
Status: Image is up to date for linguist/grammar-compiler:latest
docker.io/linguist/grammar-compiler:latest
OK! added grammar source 'vendor/grammars/godot-vscode-plugin/'
        new scope: source.gdshader
        new scope: source.gdresource
        new scope: source.gdscript
daelon@isotope:~/linguist$

@lildude does this mean that your compiler is happy now?

@Calinou I think this is done, but it might be safer to wait for the thumbs-up from the linguist people before merging.

AlfishSoftware commented 2 years ago

By the way, I didn't test it in VSCode (no time at the moment), so I don't know if the dot changes affect .length(). It might be the case where it picks .length as identifierField instead of length() as a identifierFunction as before, so you might want to double-check that length is colored correctly as a function, just in case.

DaelonSuzuka commented 2 years ago

I'll definitely double check that, thanks for the tip.

DaelonSuzuka commented 2 years ago

It might be the case where it picks .length as identifierField instead of length() as a identifierFunction

It was doing exactly this. Fantastic catch, as usual.

AlfishSoftware commented 2 years ago

Oh. I just realized review comments aren't published right away, it seems? Sorry about that, I guess you didn't see the first one (marked outdated) before.

DaelonSuzuka commented 2 years ago

Oh. I just realized review comments aren't published right away, it seems? Sorry about that, I guess you didn't see the first one (marked outdated) before.

I replied to every comment I can see, and I marked the first one as 'resolved' after I (tried to) fix the relevant bits.

If there are other comments I didn't interact with, then I definitely blame github's rather obtuse UI.

DaelonSuzuka commented 2 years ago

@Calinou This PR is also done. I rebased it to master myself just to confirm that #419 doesn't conflict(both PRs touched the GDScript grammar).

DaelonSuzuka commented 2 years ago

I checked this latest commit with linguist again, and there's no warnings/errors.

I've decided that I'm definitely going to set up that grammar unit testing tool, but I don't have time right now.

DaelonSuzuka commented 1 year ago

I checked the latest changes with linguist and it looks like they're good to go.

AlfishSoftware commented 1 year ago

Cool! Later I'll replicate those changes to gdshader (removing look-behinds) back in my own extension too, just in case.

aaronfranke commented 1 year ago

Does this PR fix https://github.com/godotengine/godot-vscode-plugin/issues/415?

DaelonSuzuka commented 1 year ago

Does this PR fix https://github.com/godotengine/godot-vscode-plugin/issues/415?

It does, but there's been some delay in communicating with the Github LInguist project to make sure the original source of the reported issue is fixed. I intended to close all the related items together but then time got away from me.

Thanks for noticing my miss!