Skip to content

Commit

Permalink
Fix dstr unparsing
Browse files Browse the repository at this point in the history
* This is an entirely new approach.
* Instead to find the "correct" dstr segments we simply try all and unparse the first one
  that round trips.
* This so far guarantees we always get good concrete syntax, but it can be time intensive as
  the combinatoric space of possible dynamic string sequence is quadratic with the dstr children size.
* For this reason we try above (currently) dstr children to unparse as heredoc first.
* Passes the entire corpus and fixes bugs.

[fix #249]
  • Loading branch information
mbj committed Jun 22, 2024
1 parent 8575e52 commit 44baa84
Show file tree
Hide file tree
Showing 33 changed files with 496 additions and 294 deletions.
9 changes: 5 additions & 4 deletions bin/parser-round-trip-test
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ class Test
:rubies
)

EXPECT_FAILURE = {}.freeze
STATIC_LOCAL_VARIABLES = %i[foo bar baz].freeze
EXPECT_FAILURE = {}.freeze

def legacy_attributes
default_builder_attributes.reject do |attribute_name, value|
Expand Down Expand Up @@ -77,9 +78,9 @@ class Test

# rubocop:disable Metrics/AbcSize
def validation
identification = name.to_s
identification = name.to_s

generated_source = Unparser.unparse_either(node)
generated_source = Unparser.unparse_either(node, static_local_variables: STATIC_LOCAL_VARIABLES)
.fmap { |string| string.dup.force_encoding(parser_source.encoding).freeze }

generated_node = generated_source.bind do |source|
Expand All @@ -99,7 +100,7 @@ class Test

def parser
Unparser.parser.tap do |parser|
%w[foo bar baz].each(&parser.static_env.method(:declare))
STATIC_LOCAL_VARIABLES.each(&parser.static_env.method(:declare))
end
end

Expand Down
23 changes: 17 additions & 6 deletions lib/unparser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ def initialize(message, node)
end
end

# Error raised when unparser encounders AST it cannot generate source for that would parse to the same AST.
class UnsupportedNodeError < RuntimeError
end

# Unparse an AST (and, optionally, comments) into a string
#
# @param [Parser::AST::Node, nil] node
Expand All @@ -57,14 +61,20 @@ def initialize(message, node)
# if the node passed is invalid
#
# @api public
def self.unparse(node, comment_array = [])
def self.unparse(node, comment_array = [], static_local_variables: Set.new)
return '' if node.nil?

local_variable_scope = AST::LocalVariableScope.new(
node: node,
static_local_variables: static_local_variables
)

Buffer.new.tap do |buffer|
Emitter::Root.new(
buffer,
node,
Comments.new(comment_array)
buffer: buffer,
comments: Comments.new(comment_array),
local_variable_scope: local_variable_scope,
node: node
).write_to_buffer
end.content
end
Expand Down Expand Up @@ -93,8 +103,8 @@ def self.unparse_validate(node, comment_array = [])
# @param [Parser::AST::Node, nil] node
#
# @return [Either<Exception, String>]
def self.unparse_either(node)
Either.wrap_error(Exception) { unparse(node) }
def self.unparse_either(node, **options)
Either.wrap_error(Exception) { unparse(node, **options) }
end

# Parse string into AST
Expand Down Expand Up @@ -224,6 +234,7 @@ def self.buffer(source, identification = '(string)')
require 'unparser/writer'
require 'unparser/writer/binary'
require 'unparser/writer/dynamic_string'
require 'unparser/writer/regexp'
require 'unparser/writer/resbody'
require 'unparser/writer/rescue'
require 'unparser/writer/send'
Expand Down
1 change: 0 additions & 1 deletion lib/unparser/ast.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ module AST
kwoptarg
lvasgn
optarg
procarg0
restarg
].to_set.freeze

Expand Down
46 changes: 26 additions & 20 deletions lib/unparser/ast/local_variable_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ module AST

# Calculated local variable scope for a given node
class LocalVariableScope
include Enumerable, Adamantium
include Adamantium, Anima.new(:static_local_variables, :node)

# Initialize object
#
Expand All @@ -15,11 +15,16 @@ class LocalVariableScope
#
# @api private
#
def initialize(node)
def initialize(*)
super(*)

items = []
LocalVariableScopeEnumerator.each(node) do |*scope|
items << scope
end

LocalVariableScopeEnumerator.each(
node: node,
stack: static_local_variables.dup
) { |*scope| items << scope }

@items = items
end

Expand Down Expand Up @@ -53,6 +58,15 @@ def local_variable_defined_for_node?(node, name)
end
end

# mutant:disable
def local_variables_for_node(needle)
@items.each do |node, current|
return current if node.equal?(needle)
end

return Set.new
end

# Test if local variables where first assigned in body and read by conditional
#
# @param [Parser::AST::Node] body
Expand Down Expand Up @@ -90,21 +104,13 @@ class LocalVariableScopeEnumerator
#
# @api private
#
def initialize
@stack = [Set.new]
def initialize(stack:)
@stack = [stack]
end

# Enumerate each node with its local variable scope
#
# @param [Parser::AST::Node] node
#
# @return [self]
#
# @api private
#
def self.each(node, &block)
new.each(node, &block)
self
def self.each(node:, stack:, &block)
new(stack: stack).each(node: node, &block)
end

# Enumerate local variable scope scope
Expand All @@ -117,7 +123,7 @@ def self.each(node, &block)
#
# @api private
#
def each(node, &block)
def each(node:, &block)
visit(node, &block)
end

Expand All @@ -132,7 +138,7 @@ def visit(node, &block)
enter(node)
yield node, current.dup, before
node.children.each do |child|
visit(child, &block) if child.is_a?(Parser::AST::Node)
visit(child, &block) if child.instance_of?(Parser::AST::Node)
end
leave(node)
end
Expand All @@ -142,7 +148,7 @@ def enter(node)
when *RESET_NODES
push_reset
when ASSIGN_NODES
define(node.children.first)
value = node.children.first and define(value)
when *INHERIT_NODES
push_inherit
end
Expand Down
1 change: 1 addition & 0 deletions lib/unparser/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ def exit_status
private

def process_target(target)
p target
validation = target.public_send(@validation)
if validation.success?
puts validation.report if @verbose
Expand Down
8 changes: 7 additions & 1 deletion lib/unparser/emitter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ module LocalVariableRoot
# @api private
#
def local_variable_scope
AST::LocalVariableScope.new(node)
AST::LocalVariableScope.new(node: node, static_local_variables: Set.new)
end

def self.included(descendant)
Expand Down Expand Up @@ -91,5 +91,11 @@ def self.emitter(buffer:, comments:, node:, local_variable_scope:)
#
abstract_method :dispatch

private

def emitter(node)
Emitter.emitter(**to_h.merge(node: node))
end

end # Emitter
end # Unparser
2 changes: 1 addition & 1 deletion lib/unparser/emitter/array_pattern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def dispatch

def emit_member(node)
if n_match_rest?(node)
writer_with(MatchRest, node).emit_array_pattern
writer_with(MatchRest, node:).emit_array_pattern
else
visit(node)
end
Expand Down
13 changes: 8 additions & 5 deletions lib/unparser/emitter/assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ def symbol_name
end

def emit_heredoc_remainders
return unless right

emitter(right).emit_heredoc_remainders
right_emitter.emit_heredoc_remainders if right
end

private
Expand All @@ -30,12 +28,17 @@ def emit_right
write(' = ')

if BINARY_OPERATOR.include?(right.type)
writer_with(Writer::Binary, right).emit_operator
writer_with(Writer::Binary, node: right).emit_operator
else
visit(right)
right_emitter.write_to_buffer
end
end

def right_emitter
emitter(right)
end
memoize :right_emitter

abstract_method :emit_left

# Variable assignment emitter
Expand Down
2 changes: 1 addition & 1 deletion lib/unparser/emitter/binary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def dispatch
end

def writer
writer_with(Writer::Binary, node)
writer_with(Writer::Binary, node:)
end
memoize :writer
end # Binary
Expand Down
6 changes: 3 additions & 3 deletions lib/unparser/emitter/block.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def write_close
end

def target_writer
writer_with(Writer::Send::Regular, target)
writer_with(Writer::Send::Regular, node: target)
end
memoize :target_writer

Expand All @@ -65,7 +65,7 @@ def emit_send_target
end

def emit_lambda_arguments
parentheses { writer_with(Args, arguments).emit_lambda_arguments }
parentheses { writer_with(Args, node: arguments).emit_lambda_arguments }
end

def numblock?
Expand All @@ -78,7 +78,7 @@ def emit_block_arguments
ws

parentheses('|', '|') do
writer_with(Args, arguments).emit_block_arguments
writer_with(Args, node: arguments).emit_block_arguments
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/unparser/emitter/def.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def emit_arguments
return if arguments.children.empty?

parentheses do
writer_with(Args, arguments).emit_def_arguments
writer_with(Args, node: arguments).emit_def_arguments
end
end

Expand Down
9 changes: 7 additions & 2 deletions lib/unparser/emitter/dstr.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,20 @@ class DStr < self
handle :dstr

def emit_heredoc_remainders
writer_with(Writer::DynamicString, node).emit_heredoc_reminder
dstr_writer.emit_heredoc_remainder
end

private

def dispatch
writer_with(Writer::DynamicString, node).dispatch
dstr_writer.dispatch
end

def dstr_writer
writer_with(Writer::DynamicString, node:)
end
memoize :dstr_writer

end # DStr
end # Emitter
end # Unparser
4 changes: 2 additions & 2 deletions lib/unparser/emitter/hash.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class Hash < self
handle :hash

def emit_heredoc_remainders
children.each(&method(:emit_heredoc_reminder_member))
children.each(&method(:emit_heredoc_remainder_member))
end

private
Expand All @@ -24,7 +24,7 @@ def dispatch
end
end

def emit_heredoc_reminder_member(node)
def emit_heredoc_remainder_member(node)
emitter(node.children.last).emit_heredoc_remainders if n_pair?(node)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/unparser/emitter/hash_pattern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def emit_member(node)
when :match_var
emit_match_var(node)
when :match_rest
writer_with(MatchRest, node).emit_hash_pattern
writer_with(MatchRest, node:).emit_hash_pattern
else
visit(node)
end
Expand Down
6 changes: 6 additions & 0 deletions lib/unparser/emitter/kwargs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ class Emitter
class Kwargs < self
handle :kwargs

def emit_heredoc_remainders
children.each do |child|
emitter(child).emit_heredoc_remainders
end
end

def dispatch
delimited(children)
end
Expand Down
4 changes: 4 additions & 0 deletions lib/unparser/emitter/pair.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ class Pair < self

children :key, :value

def emit_heredoc_remainders
emitter(value).emit_heredoc_remainders
end

private

def dispatch
Expand Down
Loading

0 comments on commit 44baa84

Please sign in to comment.