feat: complete the grammar and tidy up the repo #6

amaanq · 2023-03-02T09:46:45Z

This PR has a lot of changes, so do take your time to look over them. But I hope you'll be pleased with this, as I've fixed several bugs and added a much smoother CI/CD action, as well as general touches making the repo look a lot nicer. I've decided to bump the version to v0.8.0 accordingly, for the actions you can set the secrets yourself if you choose to, just let me know and I can send/email you the CARGO token

amaanq · 2023-03-04T02:14:31Z

Ping @drom, just hoping you see this! :)

drom · 2023-03-04T02:46:28Z

Thank you for picking up the project @amaanq . LGTM
Let me know when you are ready to merge.

amaanq · 2023-03-04T02:54:27Z

Sure thing, it's ready imo. For the publish actions, just add your npm token as a secret in the repo, and does emailing you work for my crates secret?

drom

I would prefer a set of more focused PRs
It is rather hard to review so many unrelated changes.
It all looks good to me from 11KM height ;)

.eslintrc.js

drom · 2023-03-04T02:53:23Z

.github/workflows/ci.yml

+ strategy:
+ fail-fast: true
+ matrix:
+ os: [macos-latest, ubuntu-latest]


We should keep 3 separate CI test plans Linux / MacOS / Windows as it was before.
Also run on broad matrix of NodeJS versions.

I like the broad matrix of NodeJS versions idea, but what's wrong with consolidating the OS tests to one workflow file?

Linux plan usually runs very fast and if the badge is green it make sense to wait for MacOS.
MacOS and Windows plans needed mostly to fight respected OS issues.

Linux plan usually runs very fast and if the badge is green it make sense to wait for MacOS. MacOS and Windows plans needed mostly to fight respected OS issues.

Well, the mac/windows ones don't take that much longer, and I do take into account differences in the OS (at least for Windows - see the scripts in package.json, Mac is Unix-like so it's ok), see an example of the CI badge here: https://github.com/amaanq/tree-sitter-kdl

amaanq · 2023-03-04T03:16:29Z

I would prefer a set of more focused PRs It is rather hard to review so many unrelated changes. It all looks good to me from 11KM height ;)

Sorry about that 😅 but most of the changes would sort-of depend on each other, with the exception of the manifest/dotfile updates

drom · 2023-03-04T03:47:02Z

@amaanq BTW i hope you are looking into the latest FIRRTL spec https://github.com/chipsalliance/firrtl-spec/releases/tag/v1.2.0

amaanq · 2023-03-04T04:45:31Z

@amaanq BTW i hope you are looking into the latest FIRRTL spec https://github.com/chipsalliance/firrtl-spec/releases/tag/v1.2.0

Is there anything new as far as syntax/file structure goes? I did refer to this when making my changes

drom · 2023-03-04T05:19:46Z

@amaanq BTW i hope you are looking into the latest FIRRTL spec https://github.com/chipsalliance/firrtl-spec/releases/tag/v1.2.0

Is there anything new as far as syntax/file structure goes? I did refer to this when making my changes

There are some syntax changes in master https://github.com/chipsalliance/firrtl-spec/blob/main/revision-history.yaml#L7-L14

amaanq · 2023-03-04T07:33:58Z

Oh wow, quite some changes. Do we want to reflect these changes in the grammar? Like the removal of fixed point types, partial connections, and validif?

amaanq · 2023-03-04T07:34:46Z

The spec pdf still has these removed items, and no mentions of the const type qualifier FYI (same with the ANTLR grammar), so idk where that would go

drom · 2023-03-05T01:07:50Z

.npmignore

@@ -0,0 +1,5 @@
+/test


instead .npmignore I would keep the files section inside package.json to list what ned to be published to NPM

Hmm gotcha, I do think being a bit more explicit here rather than reading through package.json to find the published files is easier on the user/dev, totally your call though

Package.swift

drom · 2023-03-05T01:11:57Z

bin/install.js

@@ -1,36 +0,0 @@
-#!/usr/bin/env node


Why have you deleted instal script?

I don't exactly know when you made this repo, but it seems to be quite a while ago from before tree-sitter handled installation for a repository. If you check out my branch and run npm install, it seems to run almost the same thing as this did, if not anything more/new

bindings/node/binding.cc

bindings/node/index.js

bindings/rust/build.rs

drom · 2023-03-05T01:16:51Z

src/grammar.json

@@ -0,0 +1,2518 @@
+{


why do we commit generated files to git?

It is so those that want to install the parser do not need to generate from the grammar file, which nvim-treesitter (neovim plugin) does. This way, all one needs to install this parser is a C compiler, which reduces time spent on installing and resources as well, at the cost of having this file in the repo. Diffs and PRs won't show changes to these files because of the .gitattributes file I added if that's a concern of yours

I am for keeping only source code under source revision control.
Published NPM package will include all generated files.
Same for Cargo and other Package managing systems.
Keeping large generated files under source code revision control leading to confusion and mismatch IMHO

nvim-treesitter, helix, emacs, nixpkgs, and whatever other editor/pkg management system almost always pulls the git version as oftentimes repos don't publish often enough to fix bugs/add features. I would like to say that the .gitattributes file prevents unnecessary noise in viewing diffs / changes online. See example here:

So, I'd like to counter the idea that this would lead to confusion/mismatches, provided that PRs changing the grammar also update the generated files (ideally with a "generate" commit of sorts)

Git is not a very good "package manager". If some systems can't download / untar NPM package we can create separate branch "package" or "release" or something, similar to "gh-pages" where we will "publish" releases of generated C, H, JSON files (no source) and that would be a "pure-man package manager". How about this?

I don't particularly like that as any sources that leverage tree-sitter packages have it configured to either generate from npm if there's no generated files, or just simple compile parser.c if it's present.

Just doing a quick check in nvim-treesitters parsers.lua list, about 30 of the ~200 languages we support require this generation process. So for the most part, grammars contain these files so people on lower end machines won't struggle as much and the only dependency is a C99 compiler then (making it much more cross-platform/universally available)..

Of course it's your final call as it's your repo, but to me it'd have far more benefits than downsides to add them here

drom · 2023-03-05T01:17:10Z

src/node-types.json

@@ -0,0 +1,1507 @@
+[


why do we commit generated files to git?

drom · 2023-03-05T01:17:48Z

src/parser.c

@@ -0,0 +1,17178 @@
+#include <tree_sitter/parser.h>


why do we commit generated files to git?
It is not the source code.

src/scanner.cc

drom · 2023-03-05T01:58:25Z

Oh wow, quite some changes. Do we want to reflect these changes in the grammar? Like the removal of fixed point types, partial connections, and validif?

Agree, let's limit this PR to v1.2.0 spec. And make all radical changes when v2.0 will be out.

package.json

bindings/node/binding.cc

drom · 2023-03-05T02:05:26Z

src/grammar.json

@@ -0,0 +1,2518 @@
+{


I am for keeping only source code under source revision control.
Published NPM package will include all generated files.
Same for Cargo and other Package managing systems.
Keeping large generated files under source code revision control leading to confusion and mismatch IMHO

drom · 2023-03-10T02:02:49Z

grammar.js

- optseq('<', $.intLit, '>'),
- optseq('<', '<', $.intLit, '>', '>')
+
+ module: $ => choice(


@amaanq
Uh, This is total revamping of content, style, organization, and structure.
Could you summarize factual changes?
What bugs it fixes?
What features where added?
For example, does it fix #2 ?

Been a while now, but let me summarize it from memory as best as I can

mux and other reserved keywords could not be used as identifiers, however in the main repository (firrtl/regress/FPU.fir) mux is used as an identifier, so adding the keyword_identifier rule aliased to identifier and inlining it solves this.

better distinguishing of escape sequences

add optional const qualifier support in port

extract any rules in statements into their own rule, which then enables setting statement as a supertype. supertypes allow us to use them in a query, but hides them in a node - inline doesn't allow us to use them in a query as well.

yes, when-else is fixed, see:

make queries (imo) perfect to neovim treesitter standards (we will standardize this with helix/other editors soon, as Max the tree-sitter creator & others have agreed last-wins is best - aka Neovim convention)

use snake_case over camelcase as is convention in tree-sitter grammars

ensure every FIRRTL file parses successfully and correctly

fill out docs as best as possible in manifests and libraries/packages

add swift package

format other files like rust, swift header, c/c++ based on appropriate standards

update tests & ci as unix-like work together just fine, then windows is its own thing

I also added support to vim/neovim for auto detection of FIRRTL files so anyone who installs the grammar (after I add it to nvim-treesitter after this is merged) will have highlights, code folding, and indents working out of the box :)

See:

…and locals

amaanq · 2023-03-10T02:19:05Z

Just an FYI with the generated files, after this is merged the .gitattributes file will auto hide diffs from those for any future commits/PRs

...and as you can see in the failing CI, your eslint config still has quite some complaints, e.g.

I updated package.json accordingly for this

drom · 2023-03-10T04:54:01Z

grammar.js


-/* eslint camelcase: 0 */
-/* eslint no-undef: 0 */


no-undef and no-unused-vars was in this file because they are specific to this file only.

amaanq force-pushed the master branch 3 times, most recently from 2028a57 to 13e93b5 Compare March 2, 2023 10:03

drom requested changes Mar 4, 2023

View reviewed changes

amaanq force-pushed the master branch from 13e93b5 to 3670c88 Compare March 4, 2023 03:15

drom requested changes Mar 5, 2023

View reviewed changes

amaanq force-pushed the master branch from 5bdf116 to 6497128 Compare March 5, 2023 01:56

drom requested changes Mar 5, 2023

View reviewed changes

amaanq requested a review from drom March 6, 2023 07:31

drom requested changes Mar 10, 2023

View reviewed changes

amaanq added 5 commits March 9, 2023 21:07

feat: fix misc bugs and complete the grammar, add file docs

d0dbcd1

feat(queries): update highlights, folds, and indents, add injections …

61ff3df

…and locals

feat: add test script

6dcfe84

chore: update tests accordingly

4478766

docs: update LICENSE and README

3e5dada

amaanq force-pushed the master branch from 6497128 to 7281f46 Compare March 10, 2023 02:08

amaanq added 3 commits March 9, 2023 21:40

chore: generate

a9385a3

feat: update bindings

aacde27

feat: add Swift bindings

a63028f

ci: move OS tests to ci.yml, add lint and publish actions

c1e6254

amaanq force-pushed the master branch from 7281f46 to 445461b Compare March 10, 2023 02:41

amaanq added 3 commits March 9, 2023 21:47

chore: update manifests

270165f

feat: v0.8.0

741298f

feat: add support for const qualifiers

58e9655

amaanq force-pushed the master branch from 445461b to 58e9655 Compare March 10, 2023 02:47

drom requested changes Mar 10, 2023

View reviewed changes

amaanq and others added 4 commits June 20, 2023 03:42

feat: rewrite the scanner in C

795709e

chore: update manifests

572ac53

chore: fix clang-tidy warnings

2b5adae

fix(grammar): adapt to upstream regex change

8503d3a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: complete the grammar and tidy up the repo #6

feat: complete the grammar and tidy up the repo #6

amaanq commented Mar 2, 2023

amaanq commented Mar 4, 2023

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023

drom left a comment

drom Mar 4, 2023

amaanq Mar 4, 2023

drom Mar 4, 2023

amaanq Mar 4, 2023 •

edited

Loading

amaanq commented Mar 4, 2023

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023 •

edited

Loading

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023

amaanq commented Mar 4, 2023 •

edited

Loading

drom Mar 5, 2023

amaanq Mar 5, 2023

drom Mar 5, 2023

amaanq Mar 5, 2023

drom Mar 5, 2023

amaanq Mar 5, 2023

drom Mar 5, 2023

amaanq Mar 5, 2023

drom Mar 10, 2023

amaanq Mar 10, 2023

drom Mar 5, 2023

drom Mar 5, 2023

drom commented Mar 5, 2023

drom Mar 5, 2023

drom Mar 10, 2023

amaanq Mar 10, 2023

amaanq Mar 10, 2023

amaanq commented Mar 10, 2023 •

edited

Loading

drom Mar 10, 2023

feat: complete the grammar and tidy up the repo #6

Are you sure you want to change the base?

feat: complete the grammar and tidy up the repo #6

Conversation

amaanq commented Mar 2, 2023

amaanq commented Mar 4, 2023

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023

drom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaanq Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

amaanq commented Mar 4, 2023

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023 • edited Loading

drom commented Mar 4, 2023

amaanq commented Mar 4, 2023

amaanq commented Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drom commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaanq commented Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

amaanq Mar 4, 2023 •

edited

Loading

amaanq commented Mar 4, 2023 •

edited

Loading

amaanq commented Mar 4, 2023 •

edited

Loading

amaanq commented Mar 10, 2023 •

edited

Loading