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

Implement parsing through enums/structs instead of vecs/strings #158

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

itsjunetime
Copy link
Contributor

... and adjust rest of workspace to work with it.

Closes #134.

Sorry that this is all one commit, I couldn't think of a way to split it up since basically every part of this depends on another part.

The big change comes in the rs/parser/src/nodes.rs file - this implements all the various structs and enums that exist in the grammar, as best as I could determine. When I was converting the main crate over to parsing this AST, I ran into a few situations where the grammar in glicol.pest and the way that it was turned into glicol_synth nodes seemed to disagree, and I just had to make my best judgement in those situations. Sometimes I modified the struct in the AST, and sometimes I just added a panic! with an explicit error message about how that feature/syntax isn't supported yet. If the panic message is inaccurate and that feature won't ever be supported, let me know and I'll modify the structs to fail at parsing that sort of thing.

The second biggest changes are in rs/main/src/{lib,util}.rs. Two things happen here:

  1. In util.rs, the AST is converted into nodes to be placed on the graph. This was fairly straightforward, and allowed us to remove a ton of unimplemented! calls (since all enums used are now exhaustive and have no invalid states, instead of just using GlicolPara as a catch-all).
  2. In lib.rs, I changed the way we update the engine with code. Previously, we would update the code, then set the flag needs_update, then actually diff and update the graph the next time next_block was called. I felt like this was not ideal, as it causes the AST and code to be out-of-sync. So instead I switched it to make update_with_code actually do the diffing and updating of the syntax tree, and next_block just gets the next block of sound.
    This required some undesirable combination of methods into one large method that does a bunch of stuff, but I think that it's for the best. It allows us to remove a bunch of members from the Engine struct that are only there temporarily for diffing, and put those into another struct that borrows from self and the new YokedAst for diffing, instead of copying data over to the temporary members of self during the diffing process.
    I also got rid of the lcs_diff::diff method and elected to do diffing manually, as that method required cloning of each AST struct that was passed in, and that's an unnecessary cost to pay. Instead, we just iterate through each new and old node + chain and see what stayed the same, what was removed, and what was added.

A few other minor things:

  • GlicolPara is now generic over S so that we can use it in situations where we want to borrow from something else to create like a GlicolPara::Reference to avoid another allocation
  • The code that writes an EngineError to a &mut [u8] is now moved to the wasm crate, since that's the only thing that actually needs that code.
  • The wasm crate is now part of the workspace since it needs to access pest (to match details of the EngineError, so its profile details have been moved to a profile called wasm-release (instead of just using the release profile, as was previously done)
  • pest errors are now boxed in our return types since they are quite large and thus make every Result type that contains them quite large as well, incurring a significant cost on method returns even if we don't encounter an error. Now, they shouldn't be quite as large, at the cost of only one allocation if we run into an error (as opposed to incurring costs before regardless of if we ran into an error).

The main issue with this all is that I haven't actually tested it yet. Before merging, I'd like to get some more formal tests written inside the parser crate, and I'd also like to just try this out with something like glicol-cli to make sure everything seems to still work just fine.

On that note, this should also allow much clearer error messages in glicol-cli that can point to exactly where we failed to parse. This should make for a much better experience there, once this gets merged and I can push a PR to glicol-cli to work with this new API.

Of course, this will also change the public API, so all these crates should be bumped to 0.14.0 if this gets merged.

If you have any questions, obviously let me know! And please don't merge until I've got some tests in (just as some motivation for myself).

@itsjunetime
Copy link
Contributor Author

I've ran this with some of my own code and it looks like the parsing works correctly now (though I'd still like to write some more tests for it), but the diffing has issues (it panicked upon a code update being ran with glicol-cli) so I need to fix that.

@itsjunetime
Copy link
Contributor Author

Ok, I think I may have fixed the panicking thing (i forgot a single !) - there are still more tests I want to add, but I think the main part of the code is reviewable now.

@chaosprint
Copy link
Owner

I just tested the WASM part a little bit: when there is an syntax error, we get a panic and the sound stops. Ideally, the music should continue and only report the error.

@itsjunetime
Copy link
Contributor Author

Would you be able to share how you're testing it in wasm, or what the stack trace is? I've only been trying this with glicol-cli so far, so I haven't had a chance to try it out with the wasm module, though I did try to make sure it still worked there.

@chaosprint
Copy link
Owner

chaosprint commented May 13, 2024

Would you be able to share how you're testing it in wasm, or what the stack trace is? I've only been trying this with glicol-cli so far, so I haven't had a chance to try it out with the wasm module, though I did try to make sure it still worked there.

First run build.sh in the ./rs/wasm folder. But since the wasm folder is now part of the workspace, we need to accordingly change the ./target folder path, which I have done in this branch:

https://github.com/chaosprint/glicol/tree/new-parser-js-test

Diffs can be found here: 351605a

Next, in the ./js folder, run pnpm i and then pnpm dev --open (you need to have pnpm installed).

In the browser (preferably Chrome) we can see a simple editor for testing and all the error message from the browser console.

Let me know if you have any questions. But in general, I think it's fine to leave the wasm/js part broken and solve it later since this is a breaking change targeting 0.14 anyway. I will test glicol-cli as well tmr.

@itsjunetime
Copy link
Contributor Author

Ok, I finally tried out the js build and it seems to work fine on my machine - the pretty-print errors even appear in the console now, which seems cool.

As one side note - it might be smart to run wasm-opt (from binaryen) on the final wasm bundle before uploading to npm. It took it from ~3.4M to 2.3M on my machine, which would probably be nice for end users.

Also, would you (@chaosprint) be able to look at the TODOs that I have in the code in this PR and see what ones you could answer/fix? Some of them regard the naming of fields in structs, and some others are about specific nodes for which I couldn't find example syntax (and thus couldn't write test cases for). I'm also wondering about some syntax ambiguities within the seq node and hoping to hear your thoughts about those ambiguities before trying to fix the failing tests there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Enums for the AST
2 participants