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

Prism::RipperCompat differences with Ripper #1972

Closed
schneems opened this issue Dec 1, 2023 · 2 comments
Closed

Prism::RipperCompat differences with Ripper #1972

schneems opened this issue Dec 1, 2023 · 2 comments

Comments

@schneems
Copy link

schneems commented Dec 1, 2023

I found that Ripper and Prism::RipperCompat produced different results on this invalid ruby code %Q* lol. This is using %Q to define a string and then declaring it will use the * character to end the string. It's invalid because it is missing a second * like %Q* lol*.

Reproduction

Define subclasses:

require 'prism'
require 'ripper'

class RipperErrors < Ripper
  attr_reader :errors

  # Comes from ripper, called
  # on every parse error, msg
  # is a string
  def on_parse_error(msg)
    @errors ||= []
    @errors << msg
  end

  alias_method :on_alias_error, :on_parse_error
  alias_method :on_assign_error, :on_parse_error
  alias_method :on_class_name_error, :on_parse_error
  alias_method :on_param_error, :on_parse_error
  alias_method :compile_error, :on_parse_error

  def call
    @run_once ||= begin
      @errors = []
      parse
      true
    end
    self
  end
end

class PrismErrors < Prism::RipperCompat
  attr_reader :errors

  # Comes from ripper, called
  # on every parse error, msg
  # is a string
  def on_parse_error(msg)
    @errors ||= []
    @errors << msg
  end

  alias_method :on_alias_error, :on_parse_error
  alias_method :on_assign_error, :on_parse_error
  alias_method :on_class_name_error, :on_parse_error
  alias_method :on_param_error, :on_parse_error
  alias_method :compile_error, :on_parse_error

  def call
    @run_once ||= begin
      @errors = []
      parse
      true
    end
    self
  end
end

Expected

I would expect that the output of these two are the same

Actual

Ripper reports an error but prism does not:

puts RipperErrors.new('%Q* lol').call.errors.inspect
# => ["unterminated string meets end of file"]
puts PrismErrors.new('%Q* lol').call.errors.inspect
# => []

It's also worth noting that Ripper can parse the valid code but prism cannot:

puts RipperErrors.new('%Q* lol*').call.errors.inspect
# => []
puts PrismErrors.new('%Q* lol*').call.errors.inspect
/Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/node.rb:15270:in `accept': undefined method `visit_string_node' for #<PrismErrors:0x00000001065e67d8 @source="%Q* lol*", @result=#<Prism::ParseResult:0x00000001065e62d8 @value=@ ProgramNode (location: (1,0)-(1,8)) (NoMethodError)
├── locals: []
└── statements:
    @ StatementsNode (location: (1,0)-(1,8))
    └── body: (length: 1)
        └── @ StringNode (location: (1,0)-(1,8))
            ├── flags: ∅
            ├── opening_loc: (1,0)-(1,3) = "%Q*"
            ├── content_loc: (1,3)-(1,7) = " lol"
            ├── closing_loc: (1,7)-(1,8) = "*"
            └── unescaped: " lol"
, @comments=[], @magic_comments=[], @errors=[], @warnings=[], @source=#<Prism::Source:0x00000001065e6710 @source="%Q* lol*", @start_line=1, @offsets=[0]>>, @lineno=1, @column=0, @errors=[]>

      visitor.visit_string_node(self)
             ^^^^^^^^^^^^^^^^^^
Did you mean?  visit_integer_node
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:95:in `visit'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:123:in `block in visit_statements_node'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:122:in `each'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:122:in `inject'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:122:in `visit_statements_node'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/node.rb:15161:in `accept'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:95:in `visit'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:144:in `visit_program_node'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/node.rb:13305:in `accept'
	from /Users/rschneeman/.gem/ruby/3.1.4/gems/prism-0.18.0/lib/prism/ripper_compat.rb:85:in `parse'
	from (irb):104:in `call'
	from (irb):123:in `<main>'
	from /Users/rschneeman/.rubies/ruby-3.1.4/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
	from /Users/rschneeman/.rubies/ruby-3.1.4/bin/irb:25:in `load'
	from /Users/rschneeman/.rubies/ruby-3.1.4/bin/irb:25:in `<main>'

This is with prism 0.18.0 and Ruby 3.1.4 (also tested with 3.2.2, same result).

@schneems
Copy link
Author

schneems commented Dec 1, 2023

FWIW Prism.parse('%Q* lol').errors.map(&:message) gives me what I need so this isn't blocking:

Prism.parse('%Q* lol').errors.map(&:message)
# => ["Expected a closing delimiter for the interpolated string"]

The message is different, but that's fine. I just need a message.

schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 1, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo: 

- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 1, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo: 

- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
@kddnewton
Copy link
Collaborator

I just pushed an update to main that should help this.

Unfortunately ripper compat is not done, but the documentation didn't say that. So it says that now. On the otherhand, I made it so that it reports errors in the same way as ripper, so that itself is done now. It also won't error on missing node implementations now, it will just skip them. Not great, but much better. I'm going to close this for now since the actual bug you reported has been fixed.

schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 4, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 4, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 4, 2023
Initial support for the prism parser

Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 4, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
schneems added a commit to ruby/syntax_suggest that referenced this issue Dec 4, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism
matzbot pushed a commit to ruby/ruby that referenced this issue Dec 5, 2023
Prism will be the parser in Ruby 3.3. We need to support 3.0+ so we will have to "dual boot" both parsers.

Todo:

- LexAll to support Prism lex output
- Add tests that exercise both Ripper and prism codepaths on CI
- Handle ruby/prism#1972 in `ripper_errors.rb`
- Update docs to not mention Ripper explicitly
- Consider different/cleaner APIs for separating out Ripper and Prism

ruby/syntax_suggest@a7d6991cc4
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

No branches or pull requests

2 participants