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

Aliases for Ripper Translation #2423

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

noahgibbs
Copy link
Contributor

@noahgibbs noahgibbs commented Feb 15, 2024

Handle more aliases, including fully handling aliases.txt.

As part of handling more aliases, handle symbols that are keywords, constants or operations. Handle interpolated symbols. Handle global variable references.

Better testing of prism ripper CLI and a test for it. I had broken it in renaming Translation::Ripper. Now it's tested. It also prints better output when Prism or Ripper Translation gets an exception during parsing.

Copy link
Collaborator

@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 couple of small changes, but overall looks good.

@@ -220,7 +220,7 @@ module Prism
source, filepath = read_source(argv)

ripper = Ripper.sexp_raw(source)
prism = Prism::RipperCompat.sexp_raw(source)
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we specifically rescue NotImplementedError (I'm assuming that's what your handling)? I wouldn't want this to hide a different problem.

Suggested change
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error
prism =
begin
Prism::Translation::Ripper.sexp_raw(source)
rescue NotImplementedError
:parse_error
end

Copy link
Contributor Author

@noahgibbs noahgibbs Feb 15, 2024

Choose a reason for hiding this comment

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

I mostly use it to make sure that if something's not working yet in Translation::Ripper that I still see CRuby's Ripper output. When it says "parse_error" that should only hide another problem if I'm expecting a parse error. But if I'm midway through making a modification and it might get an error, this means that "prism ripper" is still useful. I guess I could add a mode that doesn't actually use Prism's Ripper and just prints out CRuby's output without trying Prism?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh that's fine. We can keep it for now, I just want this to go away eventually.

test/prism/ripper_test.rb Show resolved Hide resolved
# no block call. It can be hard to tell which of multiple equivalent
# structures it will produce. This method attempts to return a normalized
# comparable structure.
def normalized_sexp(parsed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine having this for now, but we'll definitely want to get rid of this before we close this ticket out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll definitely try. I'm having trouble figuring out when Ripper does and doesn't emit this, even looking through parse.y.

@noahgibbs
Copy link
Contributor Author

Found a minor TruffleRuby Ripper incompatibility. Turning off that test on Truffle, filed a bug: oracle/truffleruby#3457

@noahgibbs noahgibbs force-pushed the ripper_compat_equiv_testing branch 5 times, most recently from 1fcfb88 to e2ae276 Compare February 15, 2024 15:11
@noahgibbs
Copy link
Contributor Author

Turning off the command line test on Windows. Aside from issues like not being able to just run bin/prism (need "ruby bin/prism" or rough equivalent) there's also not a great "which" equivalent.

@noahgibbs
Copy link
Contributor Author

noahgibbs commented Feb 15, 2024

Yeah, I'm just removing the command line test. That's annoying, I wanted to add it because the utility broke. But I'm not figuring out how to locate bin/prism to run it reliably, even if I exclude Windows. I can find it in dev checkouts (__dir__ + ../../bin/prism) and in installed gems (which prism). But in test-all it still doesn't seem to be there.

@@ -220,7 +220,7 @@ module Prism
source, filepath = read_source(argv)

ripper = Ripper.sexp_raw(source)
prism = Prism::RipperCompat.sexp_raw(source)
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh that's fine. We can keep it for now, I just want this to go away eventually.

@kddnewton kddnewton merged commit 0e0a50c into ruby:main Feb 15, 2024
54 checks passed
@noahgibbs noahgibbs deleted the ripper_compat_equiv_testing branch February 22, 2024 13:54
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