From 6d9ba1e014d5a5871f8a62ced582547ca8ea6b30 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Wed, 24 Apr 2024 11:58:24 -0400 Subject: [PATCH] [ruby/prism] Change inspect from recursive to a queue We would previously cause a stack overflow if we parsed a file that was too deeply nested when we were calling inspect. Instead, we now use a queue of commands to do it linearly so we don't. https://github.com/ruby/prism/commit/0f21f5bfe1 --- lib/prism.rb | 2 +- lib/prism/node_inspector.rb | 68 --------- lib/prism/prism.gemspec | 78 +++++----- .../lib/prism/inspect_visitor.rb.erb | 137 ++++++++++++++++++ prism/templates/lib/prism/node.rb.erb | 44 +----- prism/templates/lib/prism/reflection.rb.erb | 2 +- prism/templates/template.rb | 1 + 7 files changed, 184 insertions(+), 148 deletions(-) delete mode 100644 lib/prism/node_inspector.rb create mode 100644 prism/templates/lib/prism/inspect_visitor.rb.erb diff --git a/lib/prism.rb b/lib/prism.rb index 493bb92774..c733baf8c9 100644 --- a/lib/prism.rb +++ b/lib/prism.rb @@ -18,10 +18,10 @@ module Prism autoload :Dispatcher, "prism/dispatcher" autoload :DotVisitor, "prism/dot_visitor" autoload :DSL, "prism/dsl" + autoload :InspectVisitor, "prism/inspect_visitor" autoload :LexCompat, "prism/lex_compat" autoload :LexRipper, "prism/lex_compat" autoload :MutationCompiler, "prism/mutation_compiler" - autoload :NodeInspector, "prism/node_inspector" autoload :Pack, "prism/pack" autoload :Pattern, "prism/pattern" autoload :Reflection, "prism/reflection" diff --git a/lib/prism/node_inspector.rb b/lib/prism/node_inspector.rb deleted file mode 100644 index d77af33c3a..0000000000 --- a/lib/prism/node_inspector.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -module Prism - # This object is responsible for generating the output for the inspect method - # implementations of child nodes. - class NodeInspector # :nodoc: - attr_reader :prefix, :output - - def initialize(prefix = "") - @prefix = prefix - @output = +"" - end - - # Appends a line to the output with the current prefix. - def <<(line) - output << "#{prefix}#{line}" - end - - # This generates a string that is used as the header of the inspect output - # for any given node. - def header(node) - output = +"@ #{node.class.name.split("::").last} (" - output << "location: (#{node.location.start_line},#{node.location.start_column})-(#{node.location.end_line},#{node.location.end_column})" - output << ", newline: true" if node.newline? - output << ")\n" - output - end - - # Generates a string that represents a list of nodes. It handles properly - # using the box drawing characters to make the output look nice. - def list(prefix, nodes) - output = +"(length: #{nodes.length})\n" - last_index = nodes.length - 1 - - nodes.each_with_index do |node, index| - pointer, preadd = (index == last_index) ? ["└── ", " "] : ["├── ", "│ "] - node_prefix = "#{prefix}#{preadd}" - output << node.inspect(NodeInspector.new(node_prefix)).sub(node_prefix, "#{prefix}#{pointer}") - end - - output - end - - # Generates a string that represents a location field on a node. - def location(value) - if value - "(#{value.start_line},#{value.start_column})-(#{value.end_line},#{value.end_column}) = #{value.slice.inspect}" - else - "∅" - end - end - - # Generates a string that represents a child node. - def child_node(node, append) - node.inspect(child_inspector(append)).delete_prefix(prefix) - end - - # Returns a new inspector that can be used to inspect a child node. - def child_inspector(append) - NodeInspector.new("#{prefix}#{append}") - end - - # Returns the output as a string. - def to_str - output - end - end -end diff --git a/lib/prism/prism.gemspec b/lib/prism/prism.gemspec index e0134ff8ad..c98cc46bb8 100644 --- a/lib/prism/prism.gemspec +++ b/lib/prism/prism.gemspec @@ -76,10 +76,10 @@ Gem::Specification.new do |spec| "lib/prism/dot_visitor.rb", "lib/prism/dsl.rb", "lib/prism/ffi.rb", + "lib/prism/inspect_visitor.rb", "lib/prism/lex_compat.rb", "lib/prism/mutation_compiler.rb", "lib/prism/node_ext.rb", - "lib/prism/node_inspector.rb", "lib/prism/node.rb", "lib/prism/pack.rb", "lib/prism/parse_result.rb", @@ -101,46 +101,11 @@ Gem::Specification.new do |spec| "lib/prism/translation/ripper/shim.rb", "lib/prism/translation/ruby_parser.rb", "lib/prism/visitor.rb", - "src/diagnostic.c", - "src/encoding.c", - "src/node.c", - "src/pack.c", - "src/prettyprint.c", - "src/regexp.c", - "src/serialize.c", - "src/static_literals.c", - "src/token_type.c", - "src/util/pm_buffer.c", - "src/util/pm_char.c", - "src/util/pm_constant_pool.c", - "src/util/pm_integer.c", - "src/util/pm_list.c", - "src/util/pm_memchr.c", - "src/util/pm_newline_list.c", - "src/util/pm_string.c", - "src/util/pm_string_list.c", - "src/util/pm_strncasecmp.c", - "src/util/pm_strpbrk.c", - "src/options.c", - "src/prism.c", "prism.gemspec", - "sig/prism.rbs", - "sig/prism/compiler.rbs", - "sig/prism/dispatcher.rbs", - "sig/prism/dot_visitor.rbs", - "sig/prism/dsl.rbs", - "sig/prism/mutation_compiler.rbs", - "sig/prism/node.rbs", - "sig/prism/node_ext.rbs", - "sig/prism/pack.rbs", - "sig/prism/parse_result.rbs", - "sig/prism/pattern.rbs", - "sig/prism/reflection.rbs", - "sig/prism/serialize.rbs", - "sig/prism/visitor.rbs", "rbi/prism.rbi", "rbi/prism/compiler.rbi", "rbi/prism/desugar_compiler.rbi", + "rbi/prism/inspect_visitor.rbi", "rbi/prism/mutation_compiler.rbi", "rbi/prism/node_ext.rbi", "rbi/prism/node.rbi", @@ -153,7 +118,44 @@ Gem::Specification.new do |spec| "rbi/prism/translation/ripper.rbi", "rbi/prism/translation/ripper/ripper_compiler.rbi", "rbi/prism/translation/ruby_parser.rbi", - "rbi/prism/visitor.rbi" + "rbi/prism/visitor.rbi", + "sig/prism.rbs", + "sig/prism/compiler.rbs", + "sig/prism/dispatcher.rbs", + "sig/prism/dot_visitor.rbs", + "sig/prism/dsl.rbs", + "sig/prism/inspect_visitor.rbs", + "sig/prism/mutation_compiler.rbs", + "sig/prism/node_ext.rbs", + "sig/prism/node.rbs", + "sig/prism/pack.rbs", + "sig/prism/parse_result.rbs", + "sig/prism/pattern.rbs", + "sig/prism/reflection.rbs", + "sig/prism/serialize.rbs", + "sig/prism/visitor.rbs", + "src/diagnostic.c", + "src/encoding.c", + "src/node.c", + "src/options.c", + "src/pack.c", + "src/prettyprint.c", + "src/prism.c", + "src/regexp.c", + "src/serialize.c", + "src/static_literals.c", + "src/token_type.c", + "src/util/pm_buffer.c", + "src/util/pm_char.c", + "src/util/pm_constant_pool.c", + "src/util/pm_integer.c", + "src/util/pm_list.c", + "src/util/pm_memchr.c", + "src/util/pm_newline_list.c", + "src/util/pm_string_list.c", + "src/util/pm_string.c", + "src/util/pm_strncasecmp.c", + "src/util/pm_strpbrk.c" ] spec.extensions = ["ext/prism/extconf.rb"] diff --git a/prism/templates/lib/prism/inspect_visitor.rb.erb b/prism/templates/lib/prism/inspect_visitor.rb.erb new file mode 100644 index 0000000000..8e7902f0f1 --- /dev/null +++ b/prism/templates/lib/prism/inspect_visitor.rb.erb @@ -0,0 +1,137 @@ +module Prism + # This visitor is responsible for composing the strings that get returned by + # the various #inspect methods defined on each of the nodes. + class InspectVisitor < Visitor + # Most of the time, we can simply pass down the indent to the next node. + # However, when we are inside a list we want some extra special formatting + # when we hit an element in that list. In this case, we have a special + # command that replaces the subsequent indent with the given value. + class Replace # :nodoc: + attr_reader :value + + def initialize(value) + @value = value + end + end + + private_constant :Replace + + # The current prefix string. + attr_reader :indent + + # The list of commands that we need to execute in order to compose the + # final string. + attr_reader :commands + + # Initializes a new instance of the InspectVisitor. + def initialize(indent = +"") + @indent = indent + @commands = [] + end + + # Compose an inspect string for the given node. + def self.compose(node) + visitor = new + node.accept(visitor) + visitor.compose + end + + # Compose the final string. + def compose + buffer = +"" + replace = nil + + until commands.empty? + # @type var command: String | node | Replace + # @type var indent: String + command, indent = *commands.shift + + case command + when String + buffer << (replace || indent) + buffer << command + replace = nil + when Node + visitor = InspectVisitor.new(indent) + command.accept(visitor) + @commands = [*visitor.commands, *@commands] + when Replace + replace = command.value + else + raise "Unknown command: #{command.inspect}" + end + end + + buffer + end + <%- nodes.each do |node| -%> + + # Inspect a <%= node.name %> node. + def visit_<%= node.human %>(node) + commands << [inspect_node(<%= node.name.inspect %>, node), indent] + <%- node.fields.each_with_index do |field, index| -%> + <%- pointer = index == node.fields.length - 1 ? "└── " : "├── " -%> + <%- preadd = index == node.fields.length - 1 ? " " : "│ " -%> + <%- case field -%> + <%- when Prism::Template::NodeListField -%> + commands << ["<%= pointer %><%= field.name %>: (length: #{(<%= field.name %> = node.<%= field.name %>).length})\n", indent] + if <%= field.name %>.any? + <%= field.name %>[0...-1].each do |child| + commands << [Replace.new("#{indent}<%= preadd %>├── "), indent] + commands << [child, "#{indent}<%= preadd %>│ "] + end + commands << [Replace.new("#{indent}<%= preadd %>└── "), indent] + commands << [<%= field.name %>[-1], "#{indent}<%= preadd %> "] + end + <%- when Prism::Template::NodeField -%> + commands << ["<%= pointer %><%= field.name %>:\n", indent] + commands << [node.<%= field.name %>, "#{indent}<%= preadd %>"] + <%- when Prism::Template::OptionalNodeField -%> + if (<%= field.name %> = node.<%= field.name %>).nil? + commands << ["<%= pointer %><%= field.name %>: ∅\n", indent] + else + commands << ["<%= pointer %><%= field.name %>:\n", indent] + commands << [<%= field.name %>, "#{indent}<%= preadd %>"] + end + <%- when Prism::Template::ConstantField, Prism::Template::ConstantListField, Prism::Template::StringField, Prism::Template::UInt8Field, Prism::Template::UInt32Field, Prism::Template::IntegerField, Prism::Template::DoubleField -%> + commands << ["<%= pointer %><%= field.name %>: #{node.<%= field.name %>.inspect}\n", indent] + <%- when Prism::Template::OptionalConstantField -%> + if (<%= field.name %> = node.<%= field.name %>).nil? + commands << ["<%= pointer %><%= field.name %>: ∅\n", indent] + else + commands << ["<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n", indent] + end + <%- when Prism::Template::FlagsField -%> + <%- flag = flags.find { |flag| flag.name == field.kind }.tap { |flag| raise unless flag } -%> + flags = [<%= flag.values.map { |value| "(\"#{value.name.downcase}\" if node.#{value.name.downcase}?)" }.join(", ") %>].compact + commands << ["<%= pointer %><%= field.name %>: #{flags.empty? ? "∅" : flags.join(", ")}\n", indent] + <%- when Prism::Template::LocationField, Prism::Template::OptionalLocationField -%> + commands << ["<%= pointer %><%= field.name %>: #{inspect_location(node.<%= field.name %>)}\n", indent] + <%- end -%> + <%- end -%> + end + <%- end -%> + + private + + # Compose a header for the given node. + def inspect_node(name, node) + result = +"@ #{name} (" + + location = node.location + result << "location: (#{location.start_line},#{location.start_column})-(#{location.end_line},#{location.end_column})" + result << ", newline: true" if node.newline? + + result << ")\n" + end + + # Compose a string representing the given inner location field. + def inspect_location(location) + if location + "(#{location.start_line},#{location.start_column})-(#{location.end_line},#{location.end_column}) = #{location.slice.inspect}" + else + "∅" + end + end + end +end diff --git a/prism/templates/lib/prism/node.rb.erb b/prism/templates/lib/prism/node.rb.erb index f869a841c5..2606af5704 100644 --- a/prism/templates/lib/prism/node.rb.erb +++ b/prism/templates/lib/prism/node.rb.erb @@ -110,7 +110,7 @@ module Prism end # Returns a string representation of the node. - def inspect(inspector = NodeInspector.new) + def inspect raise NoMethodError, "undefined method `inspect' for #{inspect}" end @@ -280,45 +280,9 @@ module Prism <%- end -%> <%- end -%> - # def inspect(NodeInspector inspector) -> String - def inspect(inspector = NodeInspector.new) - inspector << inspector.header(self) - <%- node.fields.each_with_index do |field, index| -%> - <%- pointer, preadd = index == node.fields.length - 1 ? ["└── ", " "] : ["├── ", "│ "] -%> - <%- case field -%> - <%- when Prism::Template::NodeListField -%> - inspector << "<%= pointer %><%= field.name %>: #{inspector.list("#{inspector.prefix}<%= preadd %>", <%= field.name %>)}" - <%- when Prism::Template::ConstantListField -%> - inspector << "<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n" - <%- when Prism::Template::NodeField -%> - inspector << "<%= pointer %><%= field.name %>:\n" - inspector << inspector.child_node(<%= field.name %>, "<%= preadd %>") - <%- when Prism::Template::OptionalNodeField -%> - if (<%= field.name %> = self.<%= field.name %>).nil? - inspector << "<%= pointer %><%= field.name %>: ∅\n" - else - inspector << "<%= pointer %><%= field.name %>:\n" - inspector << <%= field.name %>.inspect(inspector.child_inspector("<%= preadd %>")).delete_prefix(inspector.prefix) - end - <%- when Prism::Template::ConstantField, Prism::Template::StringField, Prism::Template::UInt8Field, Prism::Template::UInt32Field, Prism::Template::IntegerField, Prism::Template::DoubleField -%> - inspector << "<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n" - <%- when Prism::Template::OptionalConstantField -%> - if (<%= field.name %> = self.<%= field.name %>).nil? - inspector << "<%= pointer %><%= field.name %>: ∅\n" - else - inspector << "<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n" - end - <%- when Prism::Template::FlagsField -%> - <%- flag = flags.find { |flag| flag.name == field.kind }.tap { |flag| raise unless flag } -%> - flags = [<%= flag.values.map { |value| "(\"#{value.name.downcase}\" if #{value.name.downcase}?)" }.join(", ") %>].compact - inspector << "<%= pointer %><%= field.name %>: #{flags.empty? ? "∅" : flags.join(", ")}\n" - <%- when Prism::Template::LocationField, Prism::Template::OptionalLocationField -%> - inspector << "<%= pointer %><%= field.name %>: #{inspector.location(<%= field.name %>)}\n" - <%- else -%> - <%- raise -%> - <%- end -%> - <%- end -%> - inspector.to_str + # def inspect -> String + def inspect + InspectVisitor.compose(self) end # Sometimes you want to check an instance of a node against a list of diff --git a/prism/templates/lib/prism/reflection.rb.erb b/prism/templates/lib/prism/reflection.rb.erb index 265dd7f3cd..3c1d61c6c1 100644 --- a/prism/templates/lib/prism/reflection.rb.erb +++ b/prism/templates/lib/prism/reflection.rb.erb @@ -118,7 +118,7 @@ module Prism when Prism::Template::OptionalLocationField "OptionalLocationField.new(:#{field.name})" when Prism::Template::UInt8Field, Prism::Template::UInt32Field, Prism::Template::IntegerField - "Integer.new(:#{field.name})" + "IntegerField.new(:#{field.name})" when Prism::Template::DoubleField "FloatField.new(:#{field.name})" when Prism::Template::FlagsField diff --git a/prism/templates/template.rb b/prism/templates/template.rb index d0ce6c6643..6f79a85371 100755 --- a/prism/templates/template.rb +++ b/prism/templates/template.rb @@ -629,6 +629,7 @@ module Prism "lib/prism/dispatcher.rb", "lib/prism/dot_visitor.rb", "lib/prism/dsl.rb", + "lib/prism/inspect_visitor.rb", "lib/prism/mutation_compiler.rb", "lib/prism/node.rb", "lib/prism/reflection.rb",