Updated parser #85

obycode · 2022-01-27T05:51:06Z

I am still doing some testing, but it is in good enough shape to start a review.

codecov-commenter · 2022-01-27T06:11:03Z

Codecov Report

Merging #85 (e6c9a96) into develop (b112962) will increase coverage by 4.94%.
The diff coverage is 82.73%.

❗ Current head e6c9a96 differs from pull request most recent head f20e58c. Consider uploading reports for the commit f20e58c to get more accurate results

@@             Coverage Diff             @@
##           develop      hirosystems/clarity-repl#85      +/-   ##
===========================================
+ Coverage    49.01%   53.95%   +4.94%     
===========================================
  Files           90       96       +6     
  Lines        24194    28751    +4557     
===========================================
+ Hits         11858    15514    +3656     
- Misses       12336    13237     +901

Flag	Coverage Δ
unittests	`53.95% <82.73%> (+4.94%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/repl/mod.rs	`90.00% <ø> (ø)`
src/repl/settings.rs	`5.26% <ø> (ø)`
src/repl/interpreter.rs	`48.65% <47.00%> (-7.05%)`	⬇️
src/repl/ast/mod.rs	`52.38% <52.38%> (ø)`
src/repl/ast/parser/lexer/error.rs	`55.00% <55.00%> (ø)`
src/repl/ast/parser/lexer/token.rs	`60.00% <60.00%> (ø)`
src/repl/ast/parser/error.rs	`61.11% <61.11%> (ø)`
src/clarity/diagnostic.rs	`44.44% <62.50%> (+0.21%)`	⬆️
src/repl/session.rs	`38.50% <71.42%> (-5.62%)`	⬇️
src/repl/ast/parser/mod.rs	`83.74% <83.74%> (ø)`
... and 11 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 b112962...f20e58c. Read the comment docs.

src/repl/ast/parser/error.rs

src/repl/ast/parser/mod.rs

src/repl/interpreter.rs

src/repl/session.rs

lgalabru

This improvements are super exciting, and the implementation is really elegant, thank you @obycode!
I added a few feedbacks, curious to get your thoughts!

lgalabru

LGTM, thank you @obycode!

This parser should have better performance and generate better errors. Resolves: #74

Some diagnostics do not have a proper column number set, leaving it as a 0. The code here subtracts one from the value to translate it to the number of spaces to insert, but if it is 0, this will cause a panic, `attempt to subtract with overflow`.

Comments are still not properly handled yet, but at least they are being properly skipped like whitespace for now.

If there was a comment with no content before a newline, then the next line was being sucked into the comment.

When there is a newline at the end of a file, and an error is reported at the very end (for example, for a missing ')'), this would crash when attempting to print out the line. This is fixed by only advancing the line number when the next token is not the end of the file.

This will be useful during the transition period as we switch from the old parser to the new parser.

While v2 is new, run both parsers and check that the ASTs are equivalent. If not, report a warning and revert to v1.

To match the current implementation in the blockchain, identifiers should be allowed to have special characters like `*`, `/`, `<`, etc. in them, although, a future specification of the Clarity language may make those illegal. Now, the parser will report a warning for those cases. This change required some modified handling of the trait identifiers (ex. `<my-trait>`), so the changes are more significant than might be expected.

obycode · 2022-02-04T22:57:31Z

The only change in that last push was just rebasing on the latest develop branch (I guess that wasn't the right way to do it). Can you approve again? Thanks!

lgalabru

👍

obycode requested a review from lgalabru January 27, 2022 05:51

obycode force-pushed the feat/parser2 branch 2 times, most recently from 8b2de3e to bfac747 Compare January 27, 2022 06:00

obycode mentioned this pull request Jan 27, 2022

feat: update to work with new parser hirosystems/clarinet#230

Merged

obycode force-pushed the feat/parser2 branch 2 times, most recently from 6b9a34e to c38d6b5 Compare February 2, 2022 01:51