tree-sitter / tree-sitter-typescript

TypeScript grammar for tree-sitter
MIT License
335 stars 104 forks source link

Add stack graph rules #209

Closed hendrikvanantwerpen closed 1 year ago

hendrikvanantwerpen commented 2 years ago

Add stack graph rules and corresponding tests. The tests are executed against the rules using the tree-sitter-stack-graphs CLI. Stack graph tests are added to CI runs as well, so that the grammar changes do not accidentally break the stack graph rules.

Repository Layout

The tree-sitter-stack-graphs CLI finds stack graph rules automatically in queries/stack-graphs.tsg, similar to tags, locals, etc. Optionally a file with predefined symbols, such as builtins or standard library definitions are automatically loaded from queries/builtins.ts.

Tests are currently located in typescript/test/stack-graphs. This is not prescribed by tree-sitter-stack-graphs so it is easy to move them if another locations is better.

CI

The stack graph tests are executed during CI. For this, the tree-sitter-stack-graphs CLI is installed using cargo, which means that is a dependency to be able to execute npm run test.

To manage the tension between having stack graph rules included and tested in CI, and grammar contributions from contributors that may not have the knowledge to update stack graph rules, I propose that the Semantic Code team gets involved for PRs where this is a problem. They can either provide direct help, or decide the PR may be merged with stack graph tests temporarily disabled, so that they can update them at a more convenient time, without blocking the grammar contribution.

Checklist

maxbrunsfeld commented 2 years ago

This is amazing; it's very cool to see all of these passing tests showing TypeScript/JavaScript's complexities being successfully modeled with stack graphs! I think it's a great thing to make these stack graph rules open source and invite collaboration from other teams who may be interested in this functionality.

My only concerns are about how the maintenance of this repo is going to work. I saw the section that you added to the README about the contributing guidelines, about potentially disabling the stack graphs CI on an as-needed basis. I'm a little bit worried about the friction that this may add to the contribution procedure, given that it's already a bit challenging to be responsive to PRs from the community.

Would it be worth scheduling a zoom call sometime soon, so that we could check in about this stuff?

hendrikvanantwerpen commented 2 years ago

@maxbrunsfeld I think it's an excellent idea to schedule a zoom call to discuss the maintenance challenges.

hendrikvanantwerpen commented 1 year ago

It seems likely that adding stack graph definitions and their tests to the grammar repos would increase friction to grammar contributions. We decided it would be better to decouple the stack graph definitions, and have them depend on specific grammar versions. The TypeScript rules are now part of github/stack-graphs#142. A versioned release will hopefully follow soon as well.

maxbrunsfeld commented 1 year ago

Yeah, I like the idea of decoupling them and having them depend on specific versions, as opposed to keeping them in-sync in the repo. Thanks for the update @hendrikvanantwerpen.

dcreager commented 1 year ago

I restored the branch for now because we're still pinning to one of its commit SHAs. Once we update our production job to point at something on master we'll delete the branch again.

hendrikvanantwerpen commented 1 year ago

🤦 I think I cleaned this up. So sorry about that! I didn't think about that dependency.

Another reason why it is good to depend on versioned releases, they are much less likely to suddenly disappear.