Fix reviewed points

Author: tompng

lib/rexml/xpath_parser.rb

@@ -345,16 +345,42 @@ def position_dependency(predicate_expr)
       # [number], [position()=number], [position() < number], [position() > number]
       return :simple if position_operation(predicate_expr)
 
-      # expressions that return number.
-      return :complex if %i[div mod mult plus minus neg].include?(predicate_expr[0])
-      return :complex if predicate_expr[0] == :function && %w[number ceiling round floor string-length sum count].include?(predicate_expr[1])
-      # Numeric literal including Integer and Float: [2] means [position() = 2]
-      return :complex if predicate_expr[0] == :literal && Numeric === predicate_expr[1]
-      # A variable could resolve to a number at runtime: [$n] means [position() = $n].
-      return :complex if predicate_expr[0] == :variable
-
       # expressions that contain position-dependent functions
       return :complex if calls_position_dependent_function?(predicate_expr)
+
+      # Even if expression is followed by path steps, the analysis of
+      # position dependency is the same as the expression itself.
+      case predicate_expr[0]
+      when :union, :or, :and, :eq, :neq, :lt, :lteq, :gt, :gteq, :not
+        # Expressions that doesn't evaluate to number is position independent
+        # if it doesn't contain position-dependent functions.
+        nil
+      when :div, :mod, :mult, :plus, :minus, :neg
+        # expressions that return number. eg. `[position() = @attr + 1]`
+        :complex
+      when :literal
+        # Integer literal is handled in `position_operation(predicate_expr)`.
+        # We treat this as complex(no-optimization) because this is not a normal case
+        # eg. `/foo["hi"]` `/bar[true]` `/baz[3.14]`
+        :complex
+      when :variable
+        # A variable could resolve to a number at runtime.
+        # It's possible to optimize this by checking the actual value of the variable.
+        :complex
+      when :function
+        # functions that return number is position dependent. eg. `[position() = string-length(@attr)]`
+        %w[number ceiling round floor string-length sum count].include?(predicate_expr[1]) ? :complex : nil
+      when :group
+        position_dependency(predicate_expr[1])
+      when :descendant, :descendant_or_self, :ancestor, :ancestor_or_self,
+           :following, :following_sibling, :preceding, :preceding_sibling,
+           :document, :child, :self, :parent, :attribute, :namespace
+        # paths are position independent. `foo[path[1]]` doesn't depend on the position of `foo`
+        nil
+      else
+        # Every other unhandled expressions are treated as complex for safety
+        :complex
+      end
     end
 
     # Recursively checks if the expression contains position-dependent functions such as position() or last()
@@ -514,15 +540,14 @@ def non_optimized_nodesets_select(nodesets, tester, selector)
         seen.to_a
       else
         operator, value = selector
-        nodes = nodesets.flatten
         nodes =
           case operator
           when :==
             nodesets.map {|nodeset| nodeset[value - 1] if value >= 1 }.compact
           when :<
             nodesets.map {|nodeset| nodeset[0...value - 1] if value >= 1 }.compact.flatten
           when :>
-            nodesets.map {|nodeset| value < 0 ? nodeset : nodeset[value..-1] }.flatten
+            nodesets.map {|nodeset| value < 0 ? nodeset : nodeset.drop(value) }.flatten
           end
         seen = Set.new.compare_by_identity
         nodes.each {|node| seen << node }

test/xpath/test_predicate.rb

@@ -103,6 +103,42 @@ def test_predicate_variable_as_position
       assert_equal(["b"], parser.parse("/r/*[$x]", doc).map(&:name))
     end
 
+    def test_predicate_out_of_range_position
+      doc = REXML::Document.new("<r><a/><b/><c/><d/></r>")
+      parser = REXML::XPathParser.new
+      base = '/r/*'
+      assert_equal([], parser.parse("#{base}[-1]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base}[0]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base}[5]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base}[position()>5]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base}[position()<-1]", doc).map(&:name))
+      assert_equal(%w[a b c d], parser.parse("#{base}[position()>-1]", doc).map(&:name))
+      assert_equal(%w[a b c d], parser.parse("#{base}[position()<10]", doc).map(&:name))
+
+      # non-optimizable case
+      base_no_opt = '/r/*[position()!=name()]'
+      assert_equal([], parser.parse("#{base_no_opt}[-1]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base_no_opt}[0]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base_no_opt}[5]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base_no_opt}[position()>5]", doc).map(&:name))
+      assert_equal([], parser.parse("#{base_no_opt}[position()<-1]", doc).map(&:name))
+      assert_equal(%w[a b c d], parser.parse("#{base_no_opt}[position()>-1]", doc).map(&:name))
+      assert_equal(%w[a b c d], parser.parse("#{base_no_opt}[position()<10]", doc).map(&:name))
+    end
+
+    def test_predicate_parenthesized_position
+      doc = REXML::Document.new("<r><a/><b/><c/><d/></r>")
+      parser = REXML::XPathParser.new
+      assert_equal(["b"], parser.parse("/r/*[(2)]", doc).map(&:name))
+    end
+
+    def test_position_dependent_function_predicates
+      doc = REXML::Document.new("<r><a/><b/><c/><d/></r>")
+      parser = REXML::XPathParser.new
+      assert_equal(["b"], parser.parse("/r/*['2'=string(-(0 - position())*1)]", doc).map(&:name))
+      assert_equal(%w[a b c d], parser.parse("/r/*['4'=string(-(0 - last())*1)]", doc).map(&:name))
+    end
+
     def test_get_no_siblings_terminal_nodes
       source = <<-XML
 <a>