Fix XPath variable handling and invalid value contamination

[tompng]

Fixes #15

Fix nil and other invalid value contamination. Nil can be injected through variables and through id function.
Unimplemented function id() is fixed to return [] instead of nil.
Add variable coerce to variable read, and applies predicates if it exists after variable.

Minor behavior changes for an invalid XPath match:

doc = REXML::Document.new("<root/>")
REXML::XPath.match(doc, '($x)[1<2]', {}, {'x'=>42})
#=> [42] → []
REXML::XPath.match(doc, '$x[1<2]', {}, {'x'=>42})
#=> [42] → []

It may raise TypeError (because $x wasn't evaluated to a nodeset), though, it shoudn't return [42]

Note

Although nodeset as a variable has been accepted before, the code added in pull request explicitly permits it.
Nokogiri only accepts string value as a variable, so there's an option to limit the value types here.
Accepting nodeset may cause a problem if the passed nodes doesn't belong to a single document root. (or just consider such case as unsupported)

[Copilot]

Pull request overview

This PR adjusts REXML’s XPath evaluation to prevent nil/invalid values from “contaminating” results, primarily by making id() return an empty node-set and by coercing variable values + applying remaining predicates/steps after variable/group evaluation.

Changes:

• Make unimplemented XPath function id() return [] (empty node-set) instead of nil.
• Add variable coercion and ensure remaining predicates/steps are applied after variable and grouped expressions.
• Add/extend tests covering id() and variable handling (including invalid predicate application on non-node-set variables).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
test/xpath/test_base.rb	Adds regression tests for id() and variable coercion/predicate behavior.
lib/rexml/xpath_parser.rb	Implements variable coercion and applies remaining predicates after variable/group evaluation.
lib/rexml/functions.rb	Changes id() to return an empty node-set ([]).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[naitoh]

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.

According to the XPath specification, anything other than a node set results in an error.

https://www.w3.org/TR/1999/REC-xpath-19991116/#node-sets

It is an error if the expression to be filtered does not evaluate to a node-set.

It is an error if the expression does not evaluate to a node-set.

I think it would be better to raise an error, as that would make the processing clearer.
What do you think? @kou @tompng

[naitoh]

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.

According to the XPath specification, if a variable name is not bound to any value, an error occurs.

https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Basics

A VariableReference evaluates to the value to which the variable name is bound in the set of variable bindings in the context.

I think it would be better to raise an error, as that would make the processing clearer.
What do you think? @kou @tompng

[tompng]

I think referencing unbound variable should raise error. Concern: when to do that, and what kind of error should it raise.

when :variable
  var_name = path_stack.shift
  # Handle unbound variables
  raise unless @variables.key?(var_name)

  # Handle conversion and validation of variable value
  # @variables[var_name]==nil case is handled here
  value = coerce_variable(@variables[var_name])
  ...

Even if we reject unbound variables, this TODO comment for bound variables (reject bound invalid-value variable or convert it to a fallback value "") still remains.