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

refactor: Retype ASTs #739

Merged
merged 7 commits into from Jan 14, 2022
Merged

refactor: Retype ASTs #739

merged 7 commits into from Jan 14, 2022

Conversation

samestep
Copy link
Collaborator

@samestep samestep commented Dec 31, 2021

Description

This PR adds types to packages/core/src/parser/ParserUtil.ts to get rid of all warnings in that file. Doing this required a few additional changes beyond just adding types to ParserUtil:

  • new SourceRange type in types/ast
  • slight change to the type predicate used in rangeOf
  • rewrite of tokenEnd to fix bugs
  • new test for tokenEnd

and also a substantial reworking of our AST types:

  • every AST node type is now generic in whether or not it extends SourceRange
  • types/ast now provides A and C types to put in that generic slot, corresponding to "abstract" or "concrete"
  • every usage of those types has been updated to provide that generic type parameter
  • most ended up being A, because even in our compilers we generate a lot of synthetic nodes
  • for other types holding nodes, everything that could be C was made C, but some were forced to be A due to above
  • error types use A instead of C just for convenience
  • this rewrite found a few missing nodeType and children fields in synthetic nodes, which were fixed

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings

@samestep samestep requested a review from wodeni December 31, 2021 14:56
@cloudflare-pages
Copy link

cloudflare-pages bot commented Dec 31, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5dc6030
Status: ✅  Deploy successful!
Preview URL: https://84b1286e.penrose-panes.pages.dev

View logs

@samestep
Copy link
Collaborator Author

Oh... the build is failing mostly because all the parsers pass ASTNodes to these functions instead of ConcreteNodes, so their types say that the start and end fields are optional; I don't know how to fix this so I'm turning this PR back to a draft for now, but @wodeni if you have any ideas of how to deal with this then let me know.

@samestep samestep marked this pull request as draft December 31, 2021 15:07
@samestep samestep changed the title chore: Type ParserUtil.ts refactor: Retype ASTs Jan 13, 2022
@samestep samestep requested a review from wodeni January 13, 2022 23:03
@samestep samestep marked this pull request as ready for review January 13, 2022 23:03
@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #739 (5dc6030) into main (7d5538b) will increase coverage by 0.00%.
The diff coverage is 79.48%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #739   +/-   ##
=======================================
  Coverage   68.85%   68.86%           
=======================================
  Files          61       61           
  Lines        7725     7765   +40     
  Branches     1454     1454           
=======================================
+ Hits         5319     5347   +28     
- Misses       2393     2405   +12     
  Partials       13       13           
Impacted Files Coverage Δ
packages/core/src/renderer/Resample.ts 23.63% <0.00%> (-0.44%) ⬇️
packages/core/src/synthesis/Mutation.ts 22.97% <16.66%> (ø)
packages/core/src/synthesis/Synthesizer.ts 20.32% <18.86%> (-0.39%) ⬇️
packages/core/src/engine/PropagateUpdate.ts 36.84% <66.66%> (ø)
packages/core/src/analysis/SubstanceAnalysis.ts 53.47% <75.60%> (+0.91%) ⬆️
packages/core/src/utils/Error.ts 34.49% <78.57%> (ø)
packages/core/src/engine/Evaluator.ts 64.57% <85.71%> (ø)
packages/core/src/synthesis/Search.ts 88.09% <88.46%> (-0.24%) ⬇️
packages/core/src/parser/ParserUtil.ts 96.96% <91.66%> (-1.45%) ⬇️
packages/core/src/engine/EngineUtils.ts 69.26% <95.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d5538b...5dc6030. Read the comment docs.

@samestep samestep merged commit 6a89b03 into main Jan 14, 2022
@samestep samestep deleted the parserutil-types branch January 14, 2022 15:06
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.

None yet

2 participants