Repository: elixir-ecto/ecto (elixir)

Issue:
Macro expansion goes too far and/or Ecto does not understand orelse/andalso
### Environment

* Elixir version (elixir -v):
  ```zsh
  Erlang/OTP 20 [erts-9.0] [source] [64-bit] [smp:2:2] [ds:2:2:10] [async-threads:10] [hipe] [kernel-poll:false]
  
  Elixir 1.6.0-dev (0beeef8) (compiled with OTP 20)
  ```
* Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.):  PostgreSQL 9.7
* Ecto version (mix deps):  ecto 2.2.1
* Database adapter and version (mix deps):  postgrex 0.13.3
* Operating system:  Linux 3.10.0-514.26.2.el7.x86_64 #1 SMP Fri Jun 30 05:26:04 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

### Current behavior

Given a simple macro like:
```elixir
  defmacrop status_active(var) do
    quote do
      unquote(var) != "I" or is_nil(unquote(var))
    end
  end
```
And trying to use it like:
```elixir
          join(query, qual, [spriden],
            goremal in DB.Banner.GOREMAL,
            goremal.goremal_pidm == spriden.spriden_pidm and status_active(goremal.goremal_status_ind)
          )
```
Causes this error:
```elixir
== Compilation error in file lib/my_server/banner.ex ==
** (Ecto.Query.CompileError) `:erlang.orelse(goremal.goremal_status_ind() <> "I", is_nil(goremal.goremal_status_ind()))` is not a valid query expression
    expanding macro: Ecto.Query.join/5
    lib/my_server/banner.ex:108: MyServer.Banner.query_fullprofile/2
    (elixir) lib/kernel/parallel_compiler.ex:121: anonymous fn/4 in Kernel.ParallelCompiler.spawn_compilers/1
```
I've tried various things including building the AST manually:
```elixir
  defmacrop status_active(var) do
    {:or, [],
     [{:!=, [], [var, "I"]},
      {:is_nil, [], [var]}]}
  end
```
Not bringing in the kernel import:
```elixir
  defmacrop status_active(var) do
    import Kernel, except: [or: 2]
    quote do
      unquote(var) != "I" or is_nil(unquote(var))
    end
  end
```
As well as inside the macro:
```elixir
  defmacrop status_active(var) do
    quote do
      import Kernel, except: [or: 2]
      unquote(var) != "I" or is_nil(unquote(var))
    end
  end
```
The last example of which gives a different error of:
```elixir
== Compilation error in file lib/my_server/banner.ex ==
** (Ecto.Query.CompileError) `(
  import(Kernel, except: [or: 2])
  goremal.goremal_status_ind() != "I" or is_nil(goremal.goremal_status_ind())
)` is not a valid query expression.

* If you intended to call a database function, please check the documentation
  for Ecto.Query to see the supported database expressions

* If you intended to call an Elixir function or introduce a value,
  you need to explicitly interpolate it with ^

    expanding macro: Ecto.Query.join/5
    lib/my_server/banner.ex:108: MyServer.Banner.query_fullprofile/2
    (elixir) lib/kernel/parallel_compiler.ex:121: anonymous fn/4 in Kernel.ParallelCompiler.spawn_compilers/1
```

### Expected behavior

Would expect it to only expand the macro's once, not recursively, or to know what the `:erlang.orelse` and the corresponding `:erlang.andalso` calls are so it can do the right thing in the sql as well for those.

Current workaround is to put it in a fragment, but then I lose the ability to make sure some casts are done right in some cases without also introducing in extra arguments, thus cluttering the API (the example above is a simplified example).

Pull request description:
Fix macro invocation
Fixed #2194.

This changed `Macro.expand` to be `Macro.expand_once` in the Ecto.Query.Builder expansion functions.  This function already recurses back in to itself if the expanded expression already matches.

Added a test to test using a macro in a when expression as well as a macro within a macro call to test recursion as well in the query expressions.

I also had to fix two specs in `changeset.ex` or the project would just not compile at all with the errors of:
```elixir
== Compilation error in file lib/ecto/changeset.ex ==
** (SyntaxError) lib/ecto/changeset.ex:1306: "{" is missing terminator "}". unexpected token: "]" at line 1306
    (elixir) lib/kernel/parallel_compiler.ex:121: anonymous fn/4 in Kernel.ParallelCompiler.spawn_compilers/1
```
And:
```elixir
== Compilation error in file lib/ecto/changeset.ex ==
** (SyntaxError) lib/ecto/changeset.ex:1345: "{" is missing terminator "}". unexpected token: "]" at line 1345
    (elixir) lib/kernel/parallel_compiler.ex:121: anonymous fn/4 in Kernel.ParallelCompiler.spawn_compilers/1
```
I'm unsure how master was compiling before this actually...

All tests pass:
```elixir
╰─➤  mix test                         
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
........................................................................................................................
...................

Finished in 12.8 seconds
1099 tests, 0 failures

Randomized with seed 697025
```

Patch:
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index 1cdd9684..667cd5cd 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -1303,7 +1303,7 @@ defmodule Ecto.Changeset do
       [title: {"cannot be foo", []}]
 
   """
-  @spec validate_change(t, atom, (atom, term -> [{atom, String.t} | {atom, {String.t, Keyword.t}])) :: t
+  @spec validate_change(t, atom, (atom, term -> [{atom, String.t} | {atom, {String.t, Keyword.t}}])) :: t
   def validate_change(%Changeset{} = changeset, field, validator) when is_atom(field) do
     %{changes: changes, errors: errors} = changeset
     ensure_field_exists!(changeset, field)
@@ -1342,7 +1342,7 @@ defmodule Ecto.Changeset do
       [title: :useless_validator]
 
   """
-  @spec validate_change(t, atom, term, (atom, term -> [{atom, String.t} | {atom, {String.t, Keyword.t}])) :: t
+  @spec validate_change(t, atom, term, (atom, term -> [{atom, String.t} | {atom, {String.t, Keyword.t}}])) :: t
   def validate_change(%Changeset{validations: validations} = changeset,
                       field, metadata, validator) do
     changeset = %{changeset | validations: [{field, metadata}|validations]}
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index 7d3e3a51..066bcd0a 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -469,7 +469,7 @@ defmodule Ecto.Query.Builder do
   end
 
   defp try_expansion(expr, type, params, vars, {env, fun} = env_fun) do
-    case Macro.expand(expr, env) do
+    case Macro.expand_once(expr, env) do
       ^expr ->
         error! """
         `#{Macro.to_string(expr)}` is not a valid query expression.


Test patch:
diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs
index 3f71812d..c6474de4 100644
--- a/test/ecto/query_test.exs
+++ b/test/ecto/query_test.exs
@@ -32,6 +32,13 @@ defmodule Ecto.QueryTest do
       from(p in "posts", select: [macro_map(^key)])
     end
 
+    defmacrop macrotest(x), do: quote(do: is_nil(unquote(x)) or unquote(x) == "A")
+    defmacrop deeper_macrotest(x), do: quote(do: macrotest(unquote(x)) or unquote(x) == "B")
+    test "allows macro in where" do
+      _ = from(p in "posts", where: p.title == "C" or macrotest(p.title))
+      _ = from(p in "posts", where: p.title == "C" or deeper_macrotest(p.title))
+    end
+
     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 ->