Improve ERB dependency detection.

The current implementation can't handle some special cases of oddly-formatted Ruby. Now we are able to detect them:

* Multi-line arguments on the `render` call
* Strings containing quotes, e.g. `"something's wrong"`
* Multiple kinds of identifiers - instance variables, class variables and globals
* Method chains as arguments for the `render` call

Also, this fix reduces the rate of "false positives" which showed up when we had calls/access to identifiers containing `render`, like `surrender` and `rendering`.

2 files changed, 145 insertions, 21 deletions

diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb
index b2e8334077..748fa81221 100644
--- a/actionview/lib/action_view/dependency_tracker.rb
+++ b/actionview/lib/action_view/dependency_tracker.rb
@@ -1,7 +1,7 @@
 require 'thread_safe'
 
 module ActionView
-  class DependencyTracker
+  class DependencyTracker # :nodoc:
     @trackers = ThreadSafe::Cache.new
 
     def self.find_dependencies(name, template)
@@ -23,9 +23,36 @@ module ActionView
       @trackers.delete(handler)
     end
 
-    class ERBTracker
+    class ERBTracker # :nodoc:
       EXPLICIT_DEPENDENCY = /# Template Dependency: (\S+)/
 
+      # A valid ruby identifier - suitable for class, method and specially variable names
+      IDENTIFIER = /
+        [[:alpha:]_] # at least one uppercase letter, lowercase letter or underscore
+        [[:word:]]*  # followed by optional letters, numbers or underscores
+      /x
+
+      # Any kind of variable name. e.g. @instance, @@class, $global or local.
+      # Possibly following a method call chain
+      VARIABLE_OR_METHOD_CHAIN = /
+        (?:\$|@{1,2})?            # optional global, instance or class variable indicator
+        (?:#{IDENTIFIER}\.)*      # followed by an optional chain of zero-argument method calls
+        (?<dynamic>#{IDENTIFIER}) # and a final valid identifier, captured as DYNAMIC
+      /x
+
+      # A simple string literal. e.g. "School's out!"
+      STRING = /
+        (?<quote>['"]) # an opening quote
+        (?<static>.*?) # with anything inside, captured as STATIC
+        \k<quote>      # and a matching closing quote
+      /x
+
+      # Part of any hash containing the :partial key
+      PARTIAL_HASH_KEY = /
+        (?:\bpartial:|:partial\s*=>) # partial key in either old or new style hash syntax
+        \s*                          # followed by optional spaces
+      /x
+
       # Matches:
       #   render partial: "comments/comment", collection: commentable.comments
       #   render "comments/comments"
@@ -36,11 +63,13 @@ module ActionView
       #   render(topics)         => render("topics/topic")
       #   render(message.topics) => render("topics/topic")
       RENDER_DEPENDENCY = /
-        render\s*                     # render, followed by optional whitespace
-        \(?                           # start an optional parenthesis for the render call
-        (partial:|:partial\s+=>)?\s*  # naming the partial, used with collection -- 1st capture
-        ([@a-z"'][@\w\/\."']+)        # the template name itself -- 2nd capture
-      /x
+        \brender\b                                  # render, the whole word
+        \s*\(?\s*                                   # optional opening paren surrounded by spaces
+        (?:
+          (?:.*?#{PARTIAL_HASH_KEY})?               # optional hash, up to the partial key declaration
+          (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest
+        )
+      /xm
 
       def self.call(name, template)
         new(name, template).dependencies
@@ -68,19 +97,30 @@ module ActionView
         end
 
         def render_dependencies
-          source.scan(RENDER_DEPENDENCY).
-            collect(&:second).uniq.
+          render_dependencies = []
 
-            # render(@topic)         => render("topics/topic")
-            # render(topics)         => render("topics/topic")
-            # render(message.topics) => render("topics/topic")
-            collect { |name| name.sub(/\A@?([a-z_]+\.)*([a-z_]+)\z/) { "#{$2.pluralize}/#{$2.singularize}" } }.
+          source.scan(RENDER_DEPENDENCY) do
+            add_dynamic_dependency(render_dependencies, Regexp.last_match[:dynamic])
+            add_static_dependency(render_dependencies, Regexp.last_match[:static])
+          end
 
-            # render("headline") => render("message/headline")
-            collect { |name| name.include?("/") ? name : "#{directory}/#{name}" }.
+          render_dependencies.uniq
+        end
+
+        def add_dynamic_dependency(dependencies, dependency)
+          if dependency
+            dependencies << "#{dependency.pluralize}/#{dependency.singularize}"
+          end
+        end
 
-            # replace quotes from string renders
-            collect { |name| name.gsub(/["']/, "") }
+        def add_static_dependency(dependencies, dependency)
+          if dependency
+            if dependency.include?('/')
+              dependencies << dependency
+            else
+              dependencies << "#{directory}/#{dependency}"
+            end
+          end
         end
 
         def explicit_dependencies
diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb
index 7a9b4b26ac..6540394c14 100644
--- a/actionview/test/template/dependency_tracker_test.rb
+++ b/actionview/test/template/dependency_tracker_test.rb
@@ -1,3 +1,5 @@
+# encoding: utf-8
+
 require 'abstract_unit'
 require 'action_view/dependency_tracker'
 
@@ -52,23 +54,105 @@ class ERBTrackerTest < Minitest::Test
 
   def test_dependency_of_erb_template_with_number_in_filename
     template = FakeTemplate.new("<%# render 'messages/message123' %>", :erb)
-    tracker = make_tracker('messages/_message123', template)
+    tracker = make_tracker("messages/_message123", template)
 
     assert_equal ["messages/message123"], tracker.dependencies
   end
 
   def test_finds_dependency_in_correct_directory
     template = FakeTemplate.new("<%# render(message.topic) %>", :erb)
-    tracker = make_tracker('messages/_message', template)
+    tracker = make_tracker("messages/_message", template)
 
     assert_equal ["topics/topic"], tracker.dependencies
   end
 
   def test_finds_dependency_in_correct_directory_with_underscore
     template = FakeTemplate.new("<%# render(message_type.messages) %>", :erb)
-    tracker = make_tracker('message_types/_message_type', template)
+    tracker = make_tracker("message_types/_message_type", template)
 
     assert_equal ["messages/message"], tracker.dependencies
   end
-end
 
+  def test_dependency_of_erb_template_with_no_spaces_after_render
+    template = FakeTemplate.new("<%# render'messages/message' %>", :erb)
+    tracker = make_tracker("messages/_message", template)
+
+    assert_equal ["messages/message"], tracker.dependencies
+  end
+
+  def test_finds_no_dependency_when_render_begins_the_name_of_an_identifier
+    template = FakeTemplate.new("<%# rendering 'it useless' %>", :erb)
+    tracker = make_tracker("resources/_resource", template)
+
+    assert_equal [], tracker.dependencies
+  end
+
+  def test_finds_no_dependency_when_render_ends_the_name_of_another_method
+    template = FakeTemplate.new("<%# surrender 'to reason' %>", :erb)
+    tracker = make_tracker("resources/_resource", template)
+
+    assert_equal [], tracker.dependencies
+  end
+
+  def test_finds_dependency_on_multiline_render_calls
+    template = FakeTemplate.new("<%#
+      render :object => @all_posts,
+             :partial => 'posts' %>", :erb)
+
+    tracker = make_tracker("some/_little_posts", template)
+
+    assert_equal ["some/posts"], tracker.dependencies
+  end
+
+  def test_finds_multiple_unrelated_odd_dependencies
+    template = FakeTemplate.new("
+      <%# render('shared/header', title: 'Title') %>
+      <h2>Section title</h2>
+      <%# render@section %>
+    ", :erb)
+
+    tracker = make_tracker("multiple/_dependencies", template)
+
+    assert_equal ["shared/header", "sections/section"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_for_all_kinds_of_identifiers
+    template = FakeTemplate.new("
+      <%# render $globals %>
+      <%# render @instance_variables %>
+      <%# render @@class_variables %>
+    ", :erb)
+
+    tracker = make_tracker("identifiers/_all", template)
+
+    assert_equal [
+      "globals/global",
+      "instance_variables/instance_variable",
+      "class_variables/class_variable"
+    ], tracker.dependencies
+  end
+
+  def test_finds_dependencies_on_method_chains
+    template = FakeTemplate.new("<%# render @parent.child.grandchildren %>", :erb)
+    tracker = make_tracker("method/_chains", template)
+
+    assert_equal ["grandchildren/grandchild"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_with_special_characters
+    template = FakeTemplate.new("<%# render @pokémon, partial: 'ピカチュウ' %>", :erb)
+    tracker = make_tracker("special/_characters", template)
+
+    assert_equal ["special/ピカチュウ"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_with_quotes_within
+    template = FakeTemplate.new("
+      <%# render \"single/quote's\" %>
+      <%# render 'double/quote\"s' %>
+    ", :erb)
+    tracker = make_tracker("quotes/_single_and_double", template)
+
+    assert_equal ["single/quote's", 'double/quote"s'], tracker.dependencies
+  end
+end