Thread safety Functions

[tompng]

REXML::Functions have a state and is not thread-safe.
Change it to a class, always create a new instance in XPath#match.
REXML::Functions = REXML::FunctionsClass.new is kept for untested REXML::Quickpath and for test code.

Bug reproduction code

['a', 'b'].map do |name|
  Thread.new do
    doc = REXML::Document.new '<div>' + "<#{name}/>"*100+'</div>'
    # Path that never matches.
    xpath = "//#{name}[name()!='#{name}']"
    loop do
      res = REXML::XPath.match(doc, xpath)
      raise "Shouldn't happen" unless res.empty?
    end
  end
end
sleep 100

After about 10 seconds, it raises "Shouldn't happen"

About deleted include Functions

Functions doesn't have instance methods, only have one constant for internal use. include Functions had no effect.

irb(main):001> REXML::VERSION
=> "3.4.4"
irb(main):002> REXML::Functions.instance_methods
=> []
irb(main):003> REXML::Functions.constants
=> [:INTERNAL_METHODS]

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors REXML XPath function handling to use instance-scoped state (context/namespaces/variables) instead of module-level shared state, improving isolation and enabling safer concurrent use.

Changes:

• Introduces FunctionsClass with instance variables for context, namespace_context, and variables, plus a new default singleton Functions.
• Updates XPathParser to use an instance of FunctionsClass for boolean/number/string conversions and function dispatch.
• Removes include Functions from REXML::XPath and REXML::QuickPath.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
lib/rexml/xpath_parser.rb	Switches function evaluation and coercions to an instance (@functions) rather than global Functions state.
lib/rexml/xpath.rb	Removes include Functions, altering how XPath functions are accessed through REXML::XPath.
lib/rexml/quickpath.rb	Removes include Functions, altering QuickPath’s access to XPath functions.
lib/rexml/functions.rb	Replaces module Functions with FunctionsClass and introduces a default singleton instance Functions.

Comments suppressed due to low confidence (1)

lib/rexml/functions.rb:1

• @@available_functions is automatically populated from all non-internal instance methods via method_added, and send will then invoke them via super (bypassing visibility checks). This makes it easy to accidentally expose non-XPath helper methods as callable XPath functions when new instance methods are added in the future. A safer approach is to explicitly whitelist exported XPath functions (or mark helper methods private and exclude private methods from registration), rather than implicitly exporting everything not listed in INTERNAL_METHODS.

# frozen_string_literal: false

---

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

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[tompng]

As copilot also mentions:

This makes it easy to accidentally expose non-XPath helper methods as callable XPath functions when new instance methods are added in the future.

Relying on this is not good. As a result, compare-language is exposed as a function.

Just an idea, perhaps introducing register method something like this may improve maintainability. (Similar problem also exist in Reline's key binding method)

register_function def position()
  ...
end

[naitoh]

@tompng
Could you please resolve the conflicts?

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[naitoh]

Thanks!