Add override modifier

[rmagatti]

Really new at this.

Initially this looks like more files than I'd want to commit, maybe missing a .gitignore or something.

Checklist:

• [x] All tests pass in CI.
• [x] There are sufficient tests for the new fix/feature.
• [x] Grammar rules have not been renamed unless absolutely necessary.
• [x] The conflicts section hasn't grown too much.
• [x] The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

[mjambon]

I don't know what those new files are. @maxbrunsfeld would know. My guess is either you ran a command from the wrong folder or there's been a recent change in tree-sitter and it's normal that these new files should be added.

[rmagatti]

@mjambon is this even the right way of adding the override modifier at least?

[mjambon]

It looks reasonable based on what we already have but I wonder about the order of all these modifiers. Do they have to come in this particular order e.g. static override readonly async? I'd look into the official typescript implementation and/or check the behavior in the typescript playground since I don't think there's an up-to-date spec. Maybe the old one from 2016 is still useful (https://javascript.xgqfrms.xyz/pdfs/TypeScript%20Language%20Specification.pdf).

[mjambon]

If you find out that the order of the modifiers is well defined, then we can merge the current implementation. Otherwise if it's not clear or the order doesn't matter, we can instead parse them as:

repeat(choice($.static, $.override, $.readonly, ...))

This tolerates duplicates but I think it would be fine.

[maxbrunsfeld]

I think that if @mjambon's suggestion works, we should prefer it, because that format generally results in significantly fewer parse states, which makes the parser faster and smaller to download.

There are likely to be restrictions on the allowed order, but it's reasonable to regard violations of those restrictions as semantic errors, not syntax errors.

[rmagatti]

So I did go through and fix the ordering at least in the public_field_definition piece. I'll try and do a few more tests soon.

[rmagatti]

If you find out that the order of the modifiers is well defined, then we can merge the current implementation. Otherwise if it's not clear or the order doesn't matter, we can instead parse them as:

repeat(choice($.static, $.override, $.readonly, ...))

This tolerates duplicates but I think it would be fine.

The order seems to be important indeed.

[mjambon]

@maxbrunsfeld this PR adds several new generated files, presumably due to recent versions of tree-sitter generating more files. Should they be added or git-ignored?

• tsx/Cargo.toml
• tsx/binding.gyp
• tsx/bindings/node/binding.cc
• tsx/bindings/node/index.js
• tsx/bindings/rust/build.rs
• tsx/bindings/rust/lib.rs
• [same for typescript/]

[mjambon]

@rmagatti sorry for not responding earlier. To expedite this, could please check the checkbox items?

I see that the tests don't pass because of string_fragment that was introduced in the main branch. You need to merge master or rebase.

[rmagatti]

Thanks for the review @mjambon I'll try and get back to this as soon as I can.

[rmagatti]

@mjambon would you mind kicking off the workflows? I'd like to see if things look good now. Tests seem to run fine for me locally now.

[mjambon]

@rmagatti what about my two earlier comments?

[mjambon]

@maxbrunsfeld what about these new generated files mentioned in my earlier comment?

[rmagatti]

@rmagatti what about my two earlier comments?

Oh wow, idk how I missed those, I'll check them out!

[rmagatti]

@mjambon One of the comments is addressed. The one about the conflict I'm not sure how to improve on, at least from a quick look. 😅 I'll try and give this a deeper look later though.

[rmagatti]

Yeah I don't know enough about how this works to know how to not have to add the conflict. 😞

[mjambon]

@rmagatti we need an answer from @maxbrunsfeld about all those new generated files. Do they need to be git-ignored?

As an aside:

Personally, I think generated files should not be placed in a source repo because:

1. They mess up diffs.
2. They let malicious contributors add code to the generated files that will escape review due to their large size.

Problem (2) can be avoided by requiring an admin or a bot to generate the files.

The solution we're using for semgrep is to treat generated code as build artifacts, and put them into their own git(hub) repo.

[rmagatti]

Yeah, despite having very little context on this particular project overall I agree with your side point as a general principle @mjambon.

[dcreager]

These newly generated files should be removed from this PR, because they're redundant with similarly named files in the repo root. See https://github.com/tree-sitter/tree-sitter-typescript/pull/135#issuecomment-919177504 for more details, and https://github.com/tree-sitter/tree-sitter/discussions/1243 for a top-level tree-sitter-wide discussion about generated files.

[mjambon]

Could you add a test for override and remove the yarn.lock unless it's important for some reason (I don't know much about yarn), then I think we can merge this!

[rmagatti]

Could you add a test for override and remove the yarn.lock unless it's important for some reason (I don't know much about yarn), then I think we can merge this!

I'll give it a try, haven't looked at how tests work for this yet. Good thing the weekend is right around the corner 😄

[rmagatti]

Added a simple query test @mjambon

[mjambon]

great. Merging!