Fix XPath variable handling and invalid value contamination

lib/rexml/functions.rb

@@ -57,6 +57,7 @@ def count( node_set )
     # Since REXML is non-validating, this method is not implemented as it
     # requires a DTD
     def id( object )
+      []
     end
 
     def local_name(node_set=nil)

lib/rexml/xpath_parser.rb

@@ -244,8 +244,10 @@ def expr( path_stack, nodeset, context=nil )
           end
         when :variable
           var_name = path_stack.shift
-          return @variables[var_name]
+          value = coerce_variable(@variables[var_name])
+          return value if path_stack.empty?
 
+          nodeset = apply_remaining_predicates(path_stack, value)
         when :eq, :neq, :lt, :lteq, :gt, :gteq
           left = expr( path_stack.shift, nodeset.dup, context )
           right = expr( path_stack.shift, nodeset.dup, context )
@@ -319,15 +321,9 @@ def expr( path_stack, nodeset, context=nil )
         when :group
           sub_expression = path_stack.shift
           result = expr(sub_expression, nodeset, context)
-          if result.is_a?(Array)
-            # If result is a nodeset, apply following predicates
-            path_stack.unshift(:node)
-            nodeset = step(path_stack) do
-              [:iterate_nodesets, [XPathParser.sort(result)]]
-            end
-          else
-            return result
-          end
+          return result if path_stack.empty?
+
+          nodeset = apply_remaining_predicates(path_stack, result)
         else
           raise "[BUG] Unexpected path: <#{op.inspect}>: <#{path_stack.inspect}>"
         end
@@ -337,6 +333,31 @@ def expr( path_stack, nodeset, context=nil )
       leave(:expr, path_stack, nodeset) if @debug
     end
 
+    def apply_remaining_predicates(path_stack, value)
+      # If evaluated value is not a nodeset, treat it as an empty nodeset.
+      # TODO: Decide whether REXML should raise type error or keep this behavior.
+      value = [] unless value.is_a?(Array)
+      path_stack.unshift(:node)
+      step(path_stack) do
+        [:iterate_nodesets, [XPathParser.sort(value)]]
+      end
+    end
+
+    # Coerces a variable value to a type that can be used in XPath expressions.
+    # TODO: Decide whether REXML should warn, raise, or ignore when a variable value is invalid.
+    def coerce_variable(value)
+      case value
+      when REXML::Node
+        [value]
+      when Array
+        value.grep(REXML::Node).uniq
+      when Numeric, String, true, false
+        value
+      else
+        ""
+      end
+    end
+
     # Determines if a predicate expression is dependent on the position of nodes.
     # Returns false if the expression is guaranteed to be position-independent.
     # Returns true if the expression might be position-dependent.

test/xpath/test_base.rb

@@ -1523,5 +1523,39 @@ def test_reverse_axis_function_argument_sort
       assert_equal(["e"], XPath.match(doc, "//e[10 + preceding-sibling::* = 11]").map(&:name))
       assert_equal(["e"], XPath.match(doc, "//e[preceding-sibling::* = '1']").map(&:name))
     end
+
+    def test_unimplemented_id_should_not_contaminate_nil
+      doc = Document.new("<root/>")
+      assert_equal([], XPath.match(doc, 'id("foo")'))
+      assert_equal([], XPath.match(doc, 'id("foo")[1]'))
+      assert_equal([], XPath.match(doc, 'id("foo")/bar'))
+    end
+
+    def test_variables
+      doc = Document.new("<a><b><c/></b><d><e/></d></a>")
+      a, b, c, d, e = XPath.match(doc, '//*')
+      assert_equal([''], XPath.match(doc, '$x', nil, {}))
+      assert_equal([''], XPath.match(doc, '$x', nil, { 'x' => nil }))
+      assert_equal([''], XPath.match(doc, '$x', nil, { 'x' => Object.new }))
+      assert_equal([3], XPath.match(doc, 'count($x)', nil, { 'x' => [b, c, d] }))
+      assert_equal([3], XPath.match(doc, 'count($x)', nil, { 'x' => [a, a, b, b, c, c] }))
+      assert_equal([b, c, d], XPath.match(doc, '$x', nil, { 'x' => [d, c, b] }))
+      assert_equal([a], XPath.match(doc, '//*[name()=$x]', nil, { 'x' => 'a' }))
+      assert_equal([c, e], XPath.match(doc, '$x/*', nil, { 'x' => [b, d] }))
+      assert_equal([c], XPath.match(doc, '$x/*', nil, { 'x' => [b] }))
+      assert_equal([c], XPath.match(doc, '$x/*', nil, { 'x' => b }))
+    end
+
+    def test_variables_invalid_predicates
+      doc = Document.new("<root/>")
+      # Predicates after variable may be invalid depending on variable type.
+      # It can raise an exception such as TypeError, or treat the predicate result as an empty node set,
+      # but it should not return the variable value itself.
+      valid_result = [:exception, []]
+      actual = (XPath.match(doc, '$x[1<2]', nil, { 'x' => 42 }) rescue :exception)
+      assert_includes(valid_result, actual)
+      actual = (XPath.match(doc, '($x)[1<2]', nil, { 'x' => 42 }) rescue :exception)
+      assert_includes(valid_result, actual)
+    end
   end
 end