From d743a2b10fb50dc4702ba4e73955c4f94b3bf700 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Fri, 29 Sep 2023 10:12:47 -0400 Subject: [PATCH] Use Prism for parsing renders when available Instead of ripper --- .../lib/action_view/dependency_tracker.rb | 2 +- .../{ripper_tracker.rb => ruby_tracker.rb} | 2 +- actionview/lib/action_view/render_parser.rb | 194 +++--------------- .../render_parser/prism_render_parser.rb | 127 ++++++++++++ .../ripper_render_parser.rb} | 161 ++++++++++++++- .../test/template/dependency_tracker_test.rb | 6 +- 6 files changed, 309 insertions(+), 183 deletions(-) rename actionview/lib/action_view/dependency_tracker/{ripper_tracker.rb => ruby_tracker.rb} (98%) create mode 100644 actionview/lib/action_view/render_parser/prism_render_parser.rb rename actionview/lib/action_view/{ripper_ast_parser.rb => render_parser/ripper_render_parser.rb} (51%) diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb index 33be6782a31a8..f14ba7598491d 100644 --- a/actionview/lib/action_view/dependency_tracker.rb +++ b/actionview/lib/action_view/dependency_tracker.rb @@ -9,7 +9,7 @@ class DependencyTracker # :nodoc: extend ActiveSupport::Autoload autoload :ERBTracker - autoload :RipperTracker + autoload :RubyTracker @trackers = Concurrent::Map.new diff --git a/actionview/lib/action_view/dependency_tracker/ripper_tracker.rb b/actionview/lib/action_view/dependency_tracker/ruby_tracker.rb similarity index 98% rename from actionview/lib/action_view/dependency_tracker/ripper_tracker.rb rename to actionview/lib/action_view/dependency_tracker/ruby_tracker.rb index 99fd46b2cc9a9..2f12b4eb1386a 100644 --- a/actionview/lib/action_view/dependency_tracker/ripper_tracker.rb +++ b/actionview/lib/action_view/dependency_tracker/ruby_tracker.rb @@ -2,7 +2,7 @@ module ActionView class DependencyTracker # :nodoc: - class RipperTracker # :nodoc: + class RubyTracker # :nodoc: EXPLICIT_DEPENDENCY = /# Template Dependency: (\S+)/ def self.call(name, template, view_paths = nil) diff --git a/actionview/lib/action_view/render_parser.rb b/actionview/lib/action_view/render_parser.rb index b7b7b614363c3..55008bf3f5f9b 100644 --- a/actionview/lib/action_view/render_parser.rb +++ b/actionview/lib/action_view/render_parser.rb @@ -1,176 +1,19 @@ # frozen_string_literal: true -require "action_view/ripper_ast_parser" - module ActionView - class RenderParser # :nodoc: - def initialize(name, code) - @name = name - @code = code - @parser = RipperASTParser - end - - def render_calls - render_nodes = @parser.parse_render_nodes(@code) - - render_nodes.map do |method, nodes| - nodes.map { |n| send(:parse_render, n) } - end.flatten.compact - end - - private - def directory - File.dirname(@name) - end - - def resolve_path_directory(path) - if path.include?("/") - path - else - "#{directory}/#{path}" - end - end - - # Convert - # render("foo", ...) - # into either - # render(template: "foo", ...) - # or - # render(partial: "foo", ...) - def normalize_args(string, options_hash) - if options_hash - { partial: string, locals: options_hash } - else - { partial: string } - end - end - - def parse_render(node) - node = node.argument_nodes - - if (node.length == 1 || node.length == 2) && !node[0].hash? - if node.length == 1 - options = normalize_args(node[0], nil) - elsif node.length == 2 - options = normalize_args(node[0], node[1]) - end - - return nil unless options + module RenderParser # :nodoc: + ALL_KNOWN_KEYS = [:partial, :template, :layout, :formats, :locals, :object, :collection, :as, :status, :content_type, :location, :spacer_template] + RENDER_TYPE_KEYS = [:partial, :template, :layout] - parse_render_from_options(options) - elsif node.length == 1 && node[0].hash? - options = parse_hash_to_symbols(node[0]) - - return nil unless options - - parse_render_from_options(options) - else - nil - end - end - - def parse_hash(node) - node.hash? && node.to_hash - end - - def parse_hash_to_symbols(node) - hash = parse_hash(node) - - return unless hash - - hash.transform_keys do |key_node| - key = parse_sym(key_node) - - return unless key - - key - end - end - - ALL_KNOWN_KEYS = [:partial, :template, :layout, :formats, :locals, :object, :collection, :as, :status, :content_type, :location, :spacer_template] - - RENDER_TYPE_KEYS = - [:partial, :template, :layout] - - def parse_render_from_options(options_hash) - renders = [] - keys = options_hash.keys - - if (keys & RENDER_TYPE_KEYS).size < 1 - # Must have at least one of render keys - return nil - end - - if (keys - ALL_KNOWN_KEYS).any? - # de-opt in case of unknown option - return nil - end - - render_type = (keys & RENDER_TYPE_KEYS)[0] - - node = options_hash[render_type] - - if node.string? - template = resolve_path_directory(node.to_string) - else - if node.variable_reference? - dependency = node.variable_name.sub(/\A(?:\$|@{1,2})/, "") - elsif node.vcall? - dependency = node.variable_name - elsif node.call? - dependency = node.call_method_name - else - return - end - - object_template = true - template = "#{dependency.pluralize}/#{dependency.singularize}" - end - - return unless template - - if spacer_template = render_template_with_spacer?(options_hash) - virtual_path = partial_to_virtual_path(:partial, spacer_template) - renders << virtual_path - end - - if options_hash.key?(:object) || options_hash.key?(:collection) || object_template - return nil if options_hash.key?(:object) && options_hash.key?(:collection) - return nil unless options_hash.key?(:partial) - end - - virtual_path = partial_to_virtual_path(render_type, template) - renders << virtual_path - - # Support for rendering multiple templates (i.e. a partial with a layout) - if layout_template = render_template_with_layout?(render_type, options_hash) - virtual_path = partial_to_virtual_path(:layout, layout_template) - - renders << virtual_path - end - - renders - end - - def parse_str(node) - node.string? && node.to_string - end - - def parse_sym(node) - node.symbol? && node.to_symbol + class Base # :nodoc: + def initialize(name, code) + @name = name + @code = code end private - def render_template_with_layout?(render_type, options_hash) - if render_type != :layout && options_hash.key?(:layout) - parse_str(options_hash[:layout]) - end - end - - def render_template_with_spacer?(options_hash) - if options_hash.key?(:spacer_template) - parse_str(options_hash[:spacer_template]) - end + def directory + File.dirname(@name) end def partial_to_virtual_path(render_type, partial_path) @@ -180,9 +23,22 @@ def partial_to_virtual_path(render_type, partial_path) partial_path end end + end - def layout_to_virtual_path(layout_path) - "layouts/#{layout_path}" - end + # Check if prism is available. If it is, use it. Otherwise, use ripper. + begin + require "prism" + rescue LoadError + require "ripper" + require_relative "render_parser/ripper_render_parser" + Parser = RipperRenderParser + else + require_relative "render_parser/prism_render_parser" + Parser = PrismRenderParser + end + + def self.new(name, code) + Parser.new(name, code) + end end end diff --git a/actionview/lib/action_view/render_parser/prism_render_parser.rb b/actionview/lib/action_view/render_parser/prism_render_parser.rb new file mode 100644 index 0000000000000..94a21d3f86fa0 --- /dev/null +++ b/actionview/lib/action_view/render_parser/prism_render_parser.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +module ActionView + module RenderParser + class PrismRenderParser < Base # :nodoc: + def render_calls + queue = [Prism.parse(@code).value] + templates = [] + + while (node = queue.shift) + queue.concat(node.compact_child_nodes) + next unless node.is_a?(Prism::CallNode) + + options = render_call_options(node) + next unless options + + render_type = (options.keys & RENDER_TYPE_KEYS)[0] + template, object_template = render_call_template(options[render_type]) + next unless template + + if options.key?(:object) || options.key?(:collection) || object_template + next if options.key?(:object) && options.key?(:collection) + next unless options.key?(:partial) + end + + if options[:spacer_template].is_a?(Prism::StringNode) + templates << partial_to_virtual_path(:partial, options[:spacer_template].unescaped) + end + + templates << partial_to_virtual_path(render_type, template) + + if render_type != :layout && options[:layout].is_a?(Prism::StringNode) + templates << partial_to_virtual_path(:layout, options[:layout].unescaped) + end + end + + templates + end + + private + # Accept a call node and return a hash of options for the render call. + # If it doesn't match the expected format, return nil. + def render_call_options(node) + # We are only looking for calls to render or render_to_string. + name = node.name.to_sym + return if name != :render && name != :render_to_string + + # We are only looking for calls with arguments. + arguments = node.arguments + return unless arguments + + arguments = arguments.arguments + length = arguments.length + + # Get rid of any parentheses to get directly to the contents. + arguments.map! do |argument| + current = argument + + while current.is_a?(Prism::ParenthesesNode) && + current.body.is_a?(Prism::StatementsNode) && + current.body.body.length == 1 + current = current.body.body.first + end + + current + end + + # We are only looking for arguments that are either a string with an + # array of locals or a keyword hash with symbol keys. + options = + if (length == 1 || length == 2) && !arguments[0].is_a?(Prism::KeywordHashNode) + { partial: arguments[0], locals: arguments[1] } + elsif length == 1 && + arguments[0].is_a?(Prism::KeywordHashNode) && + arguments[0].elements.all? do |element| + element.is_a?(Prism::AssocNode) && element.key.is_a?(Prism::SymbolNode) + end + arguments[0].elements.to_h do |element| + [element.key.unescaped.to_sym, element.value] + end + end + + return unless options + + # Here we validate that the options have the keys we expect. + keys = options.keys + return if (keys & RENDER_TYPE_KEYS).empty? + return if (keys - ALL_KNOWN_KEYS).any? + + # Finally, we can return a valid set of options. + options + end + + # Accept the node that is being passed in the position of the template + # and return the template name and whether or not it is an object + # template. + def render_call_template(node) + object_template = false + template = + if node.is_a?(Prism::StringNode) + path = node.unescaped + path.include?("/") ? path : "#{directory}/#{path}" + else + dependency = + case node.type + when :class_variable_read_node + node.slice[2..] + when :instance_variable_read_node + node.slice[1..] + when :global_variable_read_node + node.slice[1..] + when :local_variable_read_node + node.slice + when :call_node + node.name + else + return + end + + "#{dependency.pluralize}/#{dependency.singularize}" + end + + [template, object_template] + end + end + end +end diff --git a/actionview/lib/action_view/ripper_ast_parser.rb b/actionview/lib/action_view/render_parser/ripper_render_parser.rb similarity index 51% rename from actionview/lib/action_view/ripper_ast_parser.rb rename to actionview/lib/action_view/render_parser/ripper_render_parser.rb index c2f655450ae1a..e14963daa4b5c 100644 --- a/actionview/lib/action_view/ripper_ast_parser.rb +++ b/actionview/lib/action_view/render_parser/ripper_render_parser.rb @@ -1,10 +1,8 @@ # frozen_string_literal: true -require "ripper" - module ActionView - class RenderParser - module RipperASTParser # :nodoc: + module RenderParser + class RipperRenderParser < Base # :nodoc: class Node < ::Array # :nodoc: attr_reader :type @@ -183,16 +181,161 @@ def on_paren(content) end end - extend self - - def parse_render_nodes(code) - parser = RenderCallExtractor.new(code) + def render_calls + parser = RenderCallExtractor.new(@code) parser.parse parser.render_calls.group_by(&:first).to_h do |method, nodes| [ method.to_sym, nodes.collect { |v| v[1] } ] - end + end.map do |method, nodes| + nodes.map { |n| parse_render(n) } + end.flatten.compact end + + private + def resolve_path_directory(path) + if path.include?("/") + path + else + "#{directory}/#{path}" + end + end + + # Convert + # render("foo", ...) + # into either + # render(template: "foo", ...) + # or + # render(partial: "foo", ...) + def normalize_args(string, options_hash) + if options_hash + { partial: string, locals: options_hash } + else + { partial: string } + end + end + + def parse_render(node) + node = node.argument_nodes + + if (node.length == 1 || node.length == 2) && !node[0].hash? + if node.length == 1 + options = normalize_args(node[0], nil) + elsif node.length == 2 + options = normalize_args(node[0], node[1]) + end + + return nil unless options + + parse_render_from_options(options) + elsif node.length == 1 && node[0].hash? + options = parse_hash_to_symbols(node[0]) + + return nil unless options + + parse_render_from_options(options) + else + nil + end + end + + def parse_hash(node) + node.hash? && node.to_hash + end + + def parse_hash_to_symbols(node) + hash = parse_hash(node) + + return unless hash + + hash.transform_keys do |key_node| + key = parse_sym(key_node) + + return unless key + + key + end + end + + def parse_render_from_options(options_hash) + renders = [] + keys = options_hash.keys + + if (keys & RENDER_TYPE_KEYS).size < 1 + # Must have at least one of render keys + return nil + end + + if (keys - ALL_KNOWN_KEYS).any? + # de-opt in case of unknown option + return nil + end + + render_type = (keys & RENDER_TYPE_KEYS)[0] + + node = options_hash[render_type] + + if node.string? + template = resolve_path_directory(node.to_string) + else + if node.variable_reference? + dependency = node.variable_name.sub(/\A(?:\$|@{1,2})/, "") + elsif node.vcall? + dependency = node.variable_name + elsif node.call? + dependency = node.call_method_name + else + return + end + + object_template = true + template = "#{dependency.pluralize}/#{dependency.singularize}" + end + + return unless template + + if spacer_template = render_template_with_spacer?(options_hash) + virtual_path = partial_to_virtual_path(:partial, spacer_template) + renders << virtual_path + end + + if options_hash.key?(:object) || options_hash.key?(:collection) || object_template + return nil if options_hash.key?(:object) && options_hash.key?(:collection) + return nil unless options_hash.key?(:partial) + end + + virtual_path = partial_to_virtual_path(render_type, template) + renders << virtual_path + + # Support for rendering multiple templates (i.e. a partial with a layout) + if layout_template = render_template_with_layout?(render_type, options_hash) + virtual_path = partial_to_virtual_path(:layout, layout_template) + + renders << virtual_path + end + + renders + end + + def parse_str(node) + node.string? && node.to_string + end + + def parse_sym(node) + node.symbol? && node.to_symbol + end + + def render_template_with_layout?(render_type, options_hash) + if render_type != :layout && options_hash.key?(:layout) + parse_str(options_hash[:layout]) + end + end + + def render_template_with_spacer?(options_hash) + if options_hash.key?(:spacer_template) + parse_str(options_hash[:spacer_template]) + end + end end end end diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb index 0c88b734d2aa1..776460973570f 100644 --- a/actionview/test/template/dependency_tracker_test.rb +++ b/actionview/test/template/dependency_tracker_test.rb @@ -55,7 +55,7 @@ def test_returns_empty_array_if_no_tracker_is_found end end -# Tests run with both ERBTracker and RipperTracker +# Tests run with both ERBTracker and RubyTracker module SharedTrackerTests def test_dependency_of_erb_template_with_number_in_filename template = FakeTemplate.new("<%= render 'messages/message123' %>", :erb) @@ -228,11 +228,11 @@ def make_tracker(name, template) end end -class RipperTrackerTest < Minitest::Test +class RubyTrackerTest < Minitest::Test include SharedTrackerTests def make_tracker(name, template) - ActionView::DependencyTracker::RipperTracker.new(name, template) + ActionView::DependencyTracker::RubyTracker.new(name, template) end def test_dependencies_skip_unknown_options