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

Parse simple programs (integer, string, float, or rational) with Prism #25

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

egiurleo
Copy link

@egiurleo egiurleo commented Jun 5, 2024

This PR is best read commit-by-commit.

Motivation

  1. Add the --parser=prism flag to the Sorbet CLI options, allowing the user to choose to parse with Prism
  2. Depending on the value of this flag, the pipeline diverges at the parsing stage and parses with prism instead of the Sorbet parser
  3. Prism parses the source, then walks through the resulting AST in order to convert it into the Sorbet AST format
  4. So far, it can handle Program, Statement, Integer, Float, Rational, and String nodes 😁

Manual tests

You should be able to compile Sorbet:

./bazel build //main:sorbet --config=dbg --define RUBY_PATH="/path/to/your/ruby"

Then typecheck any Ruby program that is ONLY an integer, string, rational, or float, e.g.:

bazel-bin/main/sorbet -e "42" --parser=prism 
bazel-bin/main/sorbet -e "1.0r" --parser=prism
bazel-bin/main/sorbet -e "'hello world'" --parser=prism 
bazel-bin/main/sorbet -e "4.2" --parser=prism 

Automated tests

Still working on this part!

Copy link

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

A great start, nice work!

I've added a bunch of comments. They can be addressed now or later, since this is WIP I'm not overly worried about them at this point. Most of them were just to convey information as opposed to request changes.

main/pipeline/pipeline.cc Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Outdated Show resolved Hide resolved
main/pipeline/pipeline.cc Show resolved Hide resolved
main/options/options.h Outdated Show resolved Hide resolved
Copy link

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Sick!!!

main/pipeline/pipeline.cc Outdated Show resolved Hide resolved

unique_ptr<parser::Node> parseTree;

if (parser == "prism") {

Choose a reason for hiding this comment

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

Could you please switch this to a switch (pun intended), and add a default case that errors-out?

If we hit that, it indicates that we changed main/options/options.cc, but forgot to handle the new value here.

Copy link
Author

@egiurleo egiurleo Jun 10, 2024

Choose a reason for hiding this comment

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

So I attempted to implement this because it makes sense, but I realized that Sorbet is implemented in a way that would actually make it impossible to reach the default part of the switch statement. I can program the options.cc file to recognize if an invalid parser option is passed, then just use the default option (sorbet) instead, making any further processing unnecessary.

egiurleo and others added 5 commits June 11, 2024 09:57
…et or prism

If `--parser=prism`, diverge in the `pipeline.cc` file and run a separate method that parses the source with prism.
This involves setting up a `convertPrismToSorbet` method that checks against every
possible node type. Program and Statements are basically skipped because Sorbet doesn't
use them, and then Integer is converted from a prism node to a Sorbet node.

Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>
@egiurleo egiurleo marked this pull request as ready for review June 12, 2024 16:24
@egiurleo
Copy link
Author

@Morriar @amomchilov @KaanOzkan how would you feel about merging this? I'm setting up an automated testing pipeline but the changes are going to be somewhat significant because testing with Bazel is complicated 😓 I don't want to have to add a lot more changes to this PR if I don't have to.

@egiurleo egiurleo merged commit a97ed9b into proj-parsing-w-prism-in-sorbet Jun 12, 2024
@egiurleo egiurleo deleted the emily/parsing branch August 6, 2024 20:22
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.

5 participants