Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify language definitions #652

Closed
9 tasks done
Xanewok opened this issue Nov 13, 2023 · 2 comments
Closed
9 tasks done

Unify language definitions #652

Xanewok opened this issue Nov 13, 2023 · 2 comments
Assignees

Comments

@Xanewok
Copy link
Contributor

Xanewok commented Nov 13, 2023

At the moment we have three separate ways to define the language:

  1. YAML spec (aka v0)
  2. DSL v1
  3. DSL v2

Over the time, the definitions became slightly divergent, which is problematic since there are now different sources of truth. We plan to migrate to DSL v2 (#638, #637), however we need to settle on the final definition in the process. This issue should track any divergences and how we plan to resolve them.

It's worth noting that unifying the definition in a specific way (e.g. by re-introducing the List suffix for lists in DSL v2) does not prevent us from rolling back the unifying change for (all of the) language definition at some point; we only care about unification here.

The main goal is to agree on the unified, correct definition using the least amount of effort. Once we have that, we can proceed with #637, #638, and then be free to change or update the definition only once.

Known differences

These change the CST shape for our parser as shown in #650, so we should tackle most (if not all) of these until we can proceed with that PR.

Recent pull requests that have helped with this:

@Xanewok Xanewok self-assigned this Nov 13, 2023
@Xanewok Xanewok moved this to In Progress in Slang - 2023 Q4 Nov 13, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 15, 2023
Part of #652

- Fixes versioning for `UsingDirectiveSymbol` in DSL v1
- Fixes event parameter definition in DSL v2
- Fixes `ElseBranch` sequence fields to be required themselves in DSL v2
- Syncs ordered choice for `(Yul)Statement` across DSLs (helps minimize
diff for #650)

I'll probably bundle remaining renames and restructures for DSL v2 in
#650 already, but wanted to land this here since it fixes some tests and
definitions separately from migrating the parser to DSL v2.
github-merge-queue bot pushed a commit that referenced this issue Nov 15, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 15, 2023
Part of #652

Seems convenient for the AST/CST (to not need to unwrap another layer)
but we lose the ability to test it as a named parser and the generated
parser code gets a bit bloated, since we now inline the choice.

Maybe it'd be better if we somehow allow to "outline" select enums in
v2, like this one? @OmarTawfik
github-merge-queue bot pushed a commit that referenced this issue Nov 21, 2023
Part of #652 

Helps with #650 

These make more sense, given that these are the names used in the
official grammar (fixed-bytes, signed-integer-type,
unsigned-integer-type) and "XXXKeyword" name implies a single atom and
not a more complex rule (e.g. looking at the `fixed` definition...).

I think we should revert the accidental name change in v2 and later
decide if we want to change these.

EDIT: This now does the opposite and backports the name change to v0/v1.
github-merge-queue bot pushed a commit that referenced this issue Nov 21, 2023
Part of #652 

Let's unify these definitions to be in line with what's in v0/v1 as to
not change the definition and the CST shape once we switch to using v2
as our source of truth for the parser.

This is independent whether we will want that *eventually* or not. For
now, we should keep things consistent during the migration period. After
that, we can make a decision whether we want to keep these empty nodes
in the CST or not in the absence of any separated items.
github-merge-queue bot pushed a commit that referenced this issue Nov 22, 2023
Part of #652

These are a bug-fixes along with regression tests added for each case,
we use a definition from v0/v1 for these.
github-merge-queue bot pushed a commit that referenced this issue Nov 28, 2023
- fix v2 to use the same name as v0/v1 for `PathImport` and
`NamedImport`
- sync all three versions to use the same names for
`ImportDeconstruction` and `UsingDeconstruction`, which is consistent
with similar node kinds in the grammar.
- also fixed a few versioning bugs in v1, that seemed to cause more
issues downstream.

Ref #652
github-merge-queue bot pushed a commit that referenced this issue Nov 28, 2023
Use the same names across all three grammars, and make sure
`TupleMember` refers to the deconstructed (optional) symbol, not the
potentially empty parent (that is now called `DeconstructionElement`).

Ref #652
@Xanewok
Copy link
Contributor Author

Xanewok commented Dec 6, 2023

A lot was done to unify the language definitions in the meantime; with #637 done we will have only a single (and thus unified) definition.

@Xanewok
Copy link
Contributor Author

Xanewok commented Dec 13, 2023

The DSL v2 is treated as our source of truth with v0 slated for removal as part of #637, so I'll be closing this one for clarity (not much to be done in terms of unification; we just need to remove the old v0). @OmarTawfik Feel free to re-open if that's not the case.

@Xanewok Xanewok closed this as completed Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Progress
Development

No branches or pull requests

1 participant