Add a value field `Nodes::BindParam`

This is part of a greater refactoring to have the `BindParam` nodes hold
onto their values. We want to generally keep the AST decoupled from what
you're actually doing with those values, but ultimately the usage of
`BindParam` is almost identical to how you'd use `Casted` or `Quoted`.
Forcing consumers of Arel's API to maintain the bind values separately
from the AST makes manipulating the AST essentially impossible, as you
would need to perform a full walk of the AST to determine whether a
given node contains bind parameters, and which value it maps to.

By storing the value on the bind parameter directly, we can collect them
in another AST pass (realistically it'll be part of the same pass that
performs SQL construction for performance reasons). This will
dramatically simplify AST manipulation for Rails or any other consumers
that work with bind params.

As part of this change I've removed the `BindVisitor`, which appears to
be dead code, and had tests break from this change.

10 files changed, 26 insertions, 82 deletions

diff --git a/test/collectors/test_sql_string.rb b/test/collectors/test_sql_string.rb
index 769c2e6d53..8651296ff8 100644
--- a/test/collectors/test_sql_string.rb
+++ b/test/collectors/test_sql_string.rb
@@ -27,7 +27,7 @@ module Arel
       end
 
       def test_compile
-        bv = Nodes::BindParam.new
+        bv = Nodes::BindParam.new(nil)
         collector = collect ast_with_binds bv
 
         sql = collector.compile ["hello", "world"]
diff --git a/test/collectors/test_bind_collector.rb b/test/collectors/test_substitute_bind_collector.rb
index fef157782b..adcaf98319 100644
--- a/test/collectors/test_bind_collector.rb
+++ b/test/collectors/test_substitute_bind_collector.rb
@@ -4,7 +4,7 @@ require 'arel/collectors/substitute_binds'
 
 module Arel
   module Collectors
-    class TestBindCollector < Arel::Test
+    class TestSubstituteBindCollector < Arel::Test
       def setup
         @conn = FakeRecord::Base.new
         @visitor = Visitors::ToSql.new @conn.connection
@@ -28,14 +28,14 @@ module Arel
       end
 
       def test_leaves_binds
-        node = Nodes::BindParam.new
+        node = Nodes::BindParam.new(nil)
         list = compile node
         assert_equal node, list.first
         assert_equal node.class, list.first.class
       end
 
       def test_adds_strings
-        bv = Nodes::BindParam.new
+        bv = Nodes::BindParam.new(nil)
         list = compile ast_with_binds bv
         assert_operator list.length, :>, 0
         assert_equal bv, list.grep(Nodes::BindParam).first
@@ -43,7 +43,7 @@ module Arel
       end
 
       def test_substitute_binds
-        bv = Nodes::BindParam.new
+        bv = Nodes::BindParam.new(nil)
         collector = collect ast_with_binds bv
 
         values = collector.value
@@ -60,7 +60,7 @@ module Arel
       end
 
       def test_compile
-        bv = Nodes::BindParam.new
+        bv = Nodes::BindParam.new(nil)
         collector = collect ast_with_binds bv
 
         sql = collector.compile ["hello", "world"]
diff --git a/test/nodes/test_bind_param.rb b/test/nodes/test_bind_param.rb
index 011de6410c..482bb24f33 100644
--- a/test/nodes/test_bind_param.rb
+++ b/test/nodes/test_bind_param.rb
@@ -4,12 +4,17 @@ require 'helper'
 module Arel
   module Nodes
     describe 'BindParam' do
-      it 'is equal to other bind params' do
-        BindParam.new.must_equal(BindParam.new)
+      it 'is equal to other bind params with the same value' do
+        BindParam.new(1).must_equal(BindParam.new(1))
+        BindParam.new("foo").must_equal(BindParam.new("foo"))
       end
 
       it 'is not equal to other nodes' do
-        BindParam.new.wont_equal(Node.new)
+        BindParam.new(nil).wont_equal(Node.new)
+      end
+
+      it 'is not equal to bind params with different values' do
+        BindParam.new(1).wont_equal(BindParam.new(2))
       end
     end
   end
diff --git a/test/test_update_manager.rb b/test/test_update_manager.rb
index 85abbf3875..187ded12f6 100644
--- a/test/test_update_manager.rb
+++ b/test/test_update_manager.rb
@@ -13,7 +13,7 @@ module Arel
       table = Table.new(:users)
       um = Arel::UpdateManager.new
       um.table table
-      um.set [[table[:name], Arel::Nodes::BindParam.new]]
+      um.set [[table[:name], Arel::Nodes::BindParam.new(nil)]]
       um.to_sql.must_be_like %{ UPDATE "users" SET "name" =  ? }
     end
 
diff --git a/test/visitors/test_bind_visitor.rb b/test/visitors/test_bind_visitor.rb
deleted file mode 100644
index 3e0578a6a1..0000000000
--- a/test/visitors/test_bind_visitor.rb
+++ /dev/null
@@ -1,61 +0,0 @@
-# frozen_string_literal: true
-require 'helper'
-require 'arel/visitors/bind_visitor'
-require 'support/fake_record'
-
-module Arel
-  module Visitors
-    class TestBindVisitor < Arel::Test
-      attr_reader :collector
-
-      def setup
-        @collector = Collectors::SQLString.new
-        super
-      end
-
-      ##
-      # Tests visit_Arel_Nodes_Assignment correctly
-      # substitutes binds with values from block
-      def test_assignment_binds_are_substituted
-        table = Table.new(:users)
-        um = Arel::UpdateManager.new
-        bp = Nodes::BindParam.new
-        um.set [[table[:name], bp]]
-        visitor = Class.new(Arel::Visitors::ToSql) {
-          include Arel::Visitors::BindVisitor
-        }.new Table.engine.connection
-
-        assignment = um.ast.values[0]
-        actual = visitor.accept(assignment, collector) {
-          "replace"
-        }
-        assert actual
-        value = actual.value
-        assert_like "\"name\" = replace", value
-      end
-
-      def test_visitor_yields_on_binds
-        visitor = Class.new(Arel::Visitors::ToSql) {
-          include Arel::Visitors::BindVisitor
-        }.new nil
-
-        bp = Nodes::BindParam.new
-        called = false
-        visitor.accept(bp, collector) { called = true }
-        assert called
-      end
-
-      def test_visitor_only_yields_on_binds
-        visitor = Class.new(Arel::Visitors::ToSql) {
-          include Arel::Visitors::BindVisitor
-        }.new(nil)
-
-        bp = Arel.sql 'omg'
-        called = false
-
-        visitor.accept(bp, collector) { called = true }
-        refute called
-      end
-    end
-  end
-end
diff --git a/test/visitors/test_dot.rb b/test/visitors/test_dot.rb
index 1d27d1a5cb..8067ff5b49 100644
--- a/test/visitors/test_dot.rb
+++ b/test/visitors/test_dot.rb
@@ -74,7 +74,7 @@ module Arel
       end
 
       def test_Arel_Nodes_BindParam
-        node = Arel::Nodes::BindParam.new
+        node = Arel::Nodes::BindParam.new(nil)
         collector = Collectors::PlainString.new
         assert_match '[label="<f0>Arel::Nodes::BindParam"]', @visitor.accept(node, collector).value
       end
diff --git a/test/visitors/test_oracle.rb b/test/visitors/test_oracle.rb
index b1921f0cbc..cdadec8b15 100644
--- a/test/visitors/test_oracle.rb
+++ b/test/visitors/test_oracle.rb
@@ -127,8 +127,8 @@ module Arel
 
           it 'creates a subquery when there is limit and offset with BindParams' do
             stmt = Nodes::SelectStatement.new
-            stmt.limit = Nodes::Limit.new(Nodes::BindParam.new)
-            stmt.offset = Nodes::Offset.new(Nodes::BindParam.new)
+            stmt.limit = Nodes::Limit.new(Nodes::BindParam.new(nil))
+            stmt.offset = Nodes::Offset.new(Nodes::BindParam.new(nil))
             sql = compile stmt
             sql.must_be_like %{
               SELECT * FROM (
@@ -184,8 +184,8 @@ module Arel
 
       describe "Nodes::BindParam" do
         it "increments each bind param" do
-          query = @table[:name].eq(Arel::Nodes::BindParam.new)
-            .and(@table[:id].eq(Arel::Nodes::BindParam.new))
+          query = @table[:name].eq(Arel::Nodes::BindParam.new(nil))
+            .and(@table[:id].eq(Arel::Nodes::BindParam.new(nil)))
           compile(query).must_be_like %{
             "users"."name" = :a1 AND "users"."id" = :a2
           }
diff --git a/test/visitors/test_oracle12.rb b/test/visitors/test_oracle12.rb
index c908a51d4f..a6bbfe3077 100644
--- a/test/visitors/test_oracle12.rb
+++ b/test/visitors/test_oracle12.rb
@@ -48,8 +48,8 @@ module Arel
 
       describe "Nodes::BindParam" do
         it "increments each bind param" do
-          query = @table[:name].eq(Arel::Nodes::BindParam.new)
-            .and(@table[:id].eq(Arel::Nodes::BindParam.new))
+          query = @table[:name].eq(Arel::Nodes::BindParam.new(nil))
+            .and(@table[:id].eq(Arel::Nodes::BindParam.new(nil)))
           compile(query).must_be_like %{
             "users"."name" = :a1 AND "users"."id" = :a2
           }
diff --git a/test/visitors/test_postgres.rb b/test/visitors/test_postgres.rb
index 26cc721871..3b4fdc0427 100644
--- a/test/visitors/test_postgres.rb
+++ b/test/visitors/test_postgres.rb
@@ -176,8 +176,8 @@ module Arel
 
       describe "Nodes::BindParam" do
         it "increments each bind param" do
-          query = @table[:name].eq(Arel::Nodes::BindParam.new)
-            .and(@table[:id].eq(Arel::Nodes::BindParam.new))
+          query = @table[:name].eq(Arel::Nodes::BindParam.new(nil))
+            .and(@table[:id].eq(Arel::Nodes::BindParam.new(nil)))
           compile(query).must_be_like %{
             "users"."name" = $1 AND "users"."id" = $2
           }
diff --git a/test/visitors/test_to_sql.rb b/test/visitors/test_to_sql.rb
index 31279b0ae2..445d9c476c 100644
--- a/test/visitors/test_to_sql.rb
+++ b/test/visitors/test_to_sql.rb
@@ -17,13 +17,13 @@ module Arel
       end
 
       it 'works with BindParams' do
-        node = Nodes::BindParam.new
+        node = Nodes::BindParam.new(nil)
         sql = compile node
         sql.must_be_like '?'
       end
 
       it 'does not quote BindParams used as part of a Values' do
-        bp = Nodes::BindParam.new
+        bp = Nodes::BindParam.new(nil)
         values = Nodes::Values.new([bp])
         sql = compile values
         sql.must_be_like 'VALUES (?)'