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

async validation via context-free grammar #7922

Merged
merged 36 commits into from
Jan 24, 2025

Conversation

NBKelly
Copy link
Collaborator

@NBKelly NBKelly commented Jan 23, 2025

I wrote a parser for our card data to try and pick out

  • (some) things that are marked async but dont complete
  • (some) things that complete, but are not marked async
    This is added in as a unit test, and (for now) only covers the card data (I'll look at expanding it to some of the core files later maybe)

And I'm also fixing the bugs it picks up.
What I picked up so far:

  • Things that were async but didn't complete: SYNC BRE, Calibration Testing, Dummy Box, Uninstall, Helheim Servers (I fixed all of these)
  • Things that are async, but did not complete: Brasilia Govt. Grid, Ash, Angelique, Adrian, The Back, Street Magic, Stim Dealer, PolOp, Dadiana, Climactic Showdown, Chatergee University, Utae, Umbrella, Persephone, ... and lots more - there were ~70 or so - I fixed all those too
  • For things that are async, the following is checked:
    • The rightmost member of the :effect function should terminate (complete it's eid, or call a function that does), or continue to another ability (via continue-ability).
    • If the rightmost member is an if, if-not, if-let, then there should be two members which both terminate
    • If the rightmost member is a when, when-not, when-let, then we've made a mistake, because that can fail to terminate
    • All the outputs of condp, cond, case and cond+ terminate, where they exist (doesn't check that an :else/fallback actually exists though - whether that's needed is unknowable by a grammar)
    • if the RHS of an effect is a def sourced from elsewhere, it's currently not validated. Maybe I'll solve that later.

This adds the instaparse package as a dev dependency - see: https://github.com/Engelberg/instaparse

Essentially this should help us pick out (most) async errors before shipping code in the future.

project.clj Outdated Show resolved Hide resolved
@NBKelly NBKelly marked this pull request as ready for review January 23, 2025 04:58
@NBKelly
Copy link
Collaborator Author

NBKelly commented Jan 23, 2025

All right, this is (if all tests pass) good to go :)

@NBKelly
Copy link
Collaborator Author

NBKelly commented Jan 23, 2025

I made it so when checking if something completes, it checks both sides of if/if-not/if-let conditionals, and all children of cond/condp conditionals. It also automatically fails on when/when-not/when-let conditionals, because that's a bad pattern.

And it picks out cancel-effects that aren't async, as well (picked up a couple more errors with that one).

ok, now I'm done.

@NBKelly NBKelly changed the title Instaparse async checking async validation via context-free grammar Jan 24, 2025
@NoahTheDuke NoahTheDuke merged commit f725e64 into mtgred:master Jan 24, 2025
3 checks passed
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.

2 participants