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.

Sign up for GitHub

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

Jump to bottom

User functions #15

Merged
merged 9 commits into from
Feb 18, 2021
Merged

User functions #15

merged 9 commits into from
Feb 18, 2021

Conversation

cd1m0
Copy link
Collaborator

@cd1m0 cd1m0 commented Feb 9, 2021 •
edited by blitz-1306
Loading

This PR aims to add support for user-defined functions in scribble. At the moment only the grammar part is done, along with some refactoring. As it is likely gonna grow large, starting the review now. More specifically this PR:

Cleanup

  • bumps ts-pegjs to 0.3.0 and switches it over to internally maintained fork until --allowed-start-rules cli option is broken. metadevpro/ts-pegjs#53 gets fixed
  • renamed the type_grammar.pegjs and type_parser.ts to typeString_grammar.pegjs and typeString_parser.ts as their name was confusing
  • Re-factored expr_grammar.pegjs to include rules for the annotations themselves
  • Deprecated annotation_grammar.pegjs
  • Re-factored annotations.ts to do a single-pass parse, instead of the complicated 2-staging parsing we did before, just to emit graceful messages on missing semicolons

New code

  • Added AST nodes for declarations - SAnnotation, SProperty, SUserFunctionDefinition
  • Refactored the Annotation class from annotations.ts into a base AnnotationMD and child PropertyMD and UserDefinedFunctionMd classes. These hold a parsed definition node, as well as additional metadata needed to provide pretty errors and error line locations
  • Added test for new code in parse.spec.ts

Notes

  • Currently there is one failing test, due to the fact that the error message for missing semicolon changed. Its a little uglier now, but on the plus side the code is much simpler. Given that in the near future these annotation comments may transition to natspec, I am willing to pay the price of a slightly uglier error message.

cd1m0 added 4 commits February 8, 2021 14:00
@cd1m0
Bump ts-pegjs to 0.3.0 to get fix for --allowed-start-rules cli optio…
45684ac 
…n; Clarify naming for typeString parser;
@cd1m0
- Add new SAnnotation, SProperty and SUserFunctionDefinition nodes
6e63f4d 
 - Add annotation rules to expr_grammar.pegjs (should probably rename
   that)
 - Add multiple starting rules for expr_grammar.pegjs
@cd1m0
Re-factored annotations.md to support new definitions and remove old …
f74e7ac 
…hacky 2-stage parsing
@cd1m0
- update parser-generation command with multiple starting rules
b7dd279 
 - temporarily switch ts-pegjs to my own repo until metadevpro/ts-pegjs#53 gets fixed
@cd1m0 cd1m0 requested a review from blitz-1306 February 9, 2021 07:07
blitz-1306
blitz-1306 reviewed Feb 9, 2021
View reviewed changes
Copy link
Contributor

@blitz-1306 blitz-1306 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions. Generally. I think this is a good progress. Let's see how far will it go.

src/instrumenter/annotations.ts Outdated Show resolved Hide resolved
src/instrumenter/annotations.ts Outdated Show resolved Hide resolved
src/instrumenter/annotations.ts Outdated Show resolved Hide resolved
src/instrumenter/annotations.ts Outdated Show resolved Hide resolved
src/spec-lang/typeString_parser.ts Outdated Show resolved Hide resolved
cd1m0 added 4 commits February 10, 2021 20:27
@cd1m0
Type-checking support
4d901fe
@cd1m0
Transpile user functions.
b821290
@cd1m0
Fix 2 failing tests; Add a new TypeEnv class that combines type map…
6b91a1f 
…s and user function maps. Refactor `TypeMap` into the new `TypeEnv` througout the code
@cd1m0
- propagate TranspilingContext in generateExprAST
187665e 
  - attach a map from spec user function definitions to transpiled
    function definitions in InstrumentationContext
  - fix some remaining @todos
@cd1m0 cd1m0 changed the title [WIP] User functions User functions Feb 17, 2021
blitz-1306
blitz-1306 previously approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@blitz-1306 blitz-1306 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few code-related tweak suggestions to keep code-base better readable.

src/instrumenter/annotations.ts Show resolved Hide resolved
src/instrumenter/instrument.ts Outdated Show resolved Hide resolved
src/spec-lang/ast/declarations/annotation.ts Show resolved Hide resolved
src/spec-lang/expr_parser_header.ts Outdated Show resolved Hide resolved
src/spec-lang/tc/typecheck.ts Outdated Show resolved Hide resolved
@cd1m0 cd1m0 merged commit 8a51dc7 into develop Feb 18, 2021
@blitz-1306 blitz-1306 deleted the user_functions branch March 2, 2021 09:21
@blitz-1306 blitz-1306 added the enhancement New feature or request label Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blitz-1306 blitz-1306 blitz-1306 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cd1m0 @blitz-1306