Make nil comparison error message more specific (#4561)

Author: greg-rychlewski

lib/ecto/query/builder.ex

@@ -360,19 +360,25 @@ defmodule Ecto.Query.Builder do
 
     if is_nil(left) or is_nil(right) do
       error!(
-        "comparison with nil is forbidden as it is unsafe. " <>
+        "comparison with nil in `#{Macro.to_string(expr)}` is forbidden as it is unsafe. " <>
           "If you want to check if a value is nil, use is_nil/1 instead"
       )
     end
 
     ltype = quoted_type(right, vars)
     rtype = quoted_type(left, vars)
 
-    {left, params_acc} = escape(left, ltype, params_acc, vars, env)
-    {right, params_acc} = escape(right, rtype, params_acc, vars, env)
+    {escaped_left, params_acc} = escape(left, ltype, params_acc, vars, env)
+    {escaped_right, params_acc} = escape(right, rtype, params_acc, vars, env)
 
     {params, acc} = params_acc
-    {{:{}, [], [comp_op, [], [left, right]]}, {params |> wrap_nil(left) |> wrap_nil(right), acc}}
+
+    params =
+      params
+      |> wrap_nil(escaped_left, Macro.to_string(right))
+      |> wrap_nil(escaped_right, Macro.to_string(left))
+
+    {{:{}, [], [comp_op, [], [escaped_left, escaped_right]]}, {params, acc}}
   end
 
   # mathematical operators
@@ -585,18 +591,18 @@ defmodule Ecto.Query.Builder do
   defp validate_json_field!(unsupported_field),
     do: error!("`#{Macro.to_string(unsupported_field)}` is not a valid json field")
 
-  defp wrap_nil(params, {:{}, _, [:^, _, [ix]]}),
-    do: wrap_nil(params, length(params) - ix - 1, [])
+  defp wrap_nil(params, {:{}, _, [:^, _, [ix]]}, compare_str),
+    do: wrap_nil(params, length(params) - ix - 1, compare_str, [])
 
-  defp wrap_nil(params, _other), do: params
+  defp wrap_nil(params, _other, _compare_str), do: params
 
-  defp wrap_nil([{val, type} | params], 0, acc) do
-    val = quote do: Ecto.Query.Builder.not_nil!(unquote(val))
+  defp wrap_nil([{val, type} | params], 0, compare_str, acc) do
+    val = quote do: Ecto.Query.Builder.not_nil!(unquote(val), unquote(compare_str))
     Enum.reverse(acc, [{val, type} | params])
   end
 
-  defp wrap_nil([pair | params], i, acc) do
-    wrap_nil(params, i - 1, [pair | acc])
+  defp wrap_nil([pair | params], i, compare_str, acc) do
+    wrap_nil(params, i - 1, compare_str, [pair | acc])
   end
 
   defp expand_and_split_fragment(query, env) do
@@ -1184,13 +1190,13 @@ defmodule Ecto.Query.Builder do
   @doc """
   Called by escaper at runtime to verify that a value is not nil.
   """
-  def not_nil!(nil) do
+  def not_nil!(nil, compare_str) do
     raise ArgumentError,
-          "comparison with nil is forbidden as it is unsafe. " <>
+          "comparing `#{compare_str}` with `nil` is forbidden as it is unsafe. " <>
             "If you want to check if a value is nil, use is_nil/1 instead"
   end
 
-  def not_nil!(not_nil) do
+  def not_nil!(not_nil, _compare_str) do
     not_nil
   end
 

test/ecto/query_test.exs

@@ -56,17 +56,23 @@ defmodule Ecto.QueryTest do
 
     test "does not allow nils in comparison at compile time" do
       assert_raise Ecto.Query.CompileError,
-                   ~r"comparison with nil is forbidden as it is unsafe", fn ->
+                   ~r"comparison with nil in `p.id == nil` is forbidden as it is unsafe", fn ->
         quote_and_eval from p in "posts", where: p.id == nil
       end
     end
 
     test "does not allow interpolated nils at runtime" do
+      id = nil
+
       assert_raise ArgumentError,
-                   ~r"comparison with nil is forbidden as it is unsafe", fn ->
-        id = nil
+                   ~r"nil given for `id`. comparison with nil is forbidden as it is unsafe", fn ->
         from p in "posts", where: [id: ^id]
       end
+
+      assert_raise ArgumentError,
+                   ~r"comparing `p.id` with `nil` is forbidden as it is unsafe", fn ->
+        from p in "posts", where: p.id == ^id
+      end
     end
 
     test "allows arbitrary parentheses in where" do