Proposal: shallow validation of unquoted AST

lib/elixir/lib/macro.ex

@@ -814,7 +814,8 @@ defmodule Macro do
 
       iex> value = {:a, :b, :c}
       iex> quote do: unquote(value)
-      {:a, :b, :c}
+      ** (ArgumentError) tried to unquote invalid AST: {:a, :b, :c}
+      Did you forget to escape term using Macro.escape/1?
 
   `escape/2` is used to escape *values* (either directly passed or variable
   bound), while `quote/2` produces syntax trees for

lib/elixir/src/elixir_quote.erl

@@ -1,6 +1,6 @@
 -module(elixir_quote).
 -export([escape/3, linify/3, linify_with_context_counter/3, build/7, quote/2, has_unquotes/1, fun_to_quoted/1]).
--export([dot/5, tail_list/3, list/2, validate_runtime/2]). %% Quote callbacks
+-export([dot/5, tail_list/3, list/2, validate_runtime/2, shallow_validate_ast/1]). %% Quote callbacks
 
 -include("elixir.hrl").
 -define(defs(Kind), Kind == def; Kind == defp; Kind == defmacro; Kind == defmacrop; Kind == '@').
@@ -15,7 +15,8 @@
   aliases_hygiene=nil,
   imports_hygiene=nil,
   unquote=true,
-  generated=false
+  generated=false,
+  shallow_validate=false
 }).
 
 %% fun_to_quoted
@@ -215,7 +216,8 @@ build(Meta, Line, File, Context, Unquote, Generated, E) ->
     file=VFile,
     unquote=Unquote,
     context=VContext,
-    generated=Generated
+    generated=Generated,
+    shallow_validate=true
   },
 
   {Q, VContext, Acc3}.
@@ -254,6 +256,27 @@ is_valid(context, Context) -> is_atom(Context) andalso (Context /= nil);
 is_valid(generated, Generated) -> is_boolean(Generated);
 is_valid(unquote, Unquote) -> is_boolean(Unquote).
 
+shallow_validate_ast(Expr) ->
+  case shallow_valid_ast(Expr) of
+    true -> Expr;
+    false -> argument_error(
+      <<"tried to unquote invalid AST: ", ('Elixir.Kernel':inspect(Expr))/binary,
+        "\nDid you forget to escape term using Macro.escape/1?">>)
+  end.
+
+shallow_valid_ast(Expr) when is_list(Expr) -> valid_ast_list(Expr);
+shallow_valid_ast(Expr) -> valid_ast_elem(Expr).
+
+valid_ast_list([]) -> true;
+valid_ast_list([Head | Tail]) -> valid_ast_elem(Head) andalso valid_ast_list(Tail);
+valid_ast_list(_Improper) -> false.
+
+valid_ast_elem(Expr) when is_list(Expr); is_atom(Expr); is_binary(Expr); is_number(Expr); is_pid(Expr) -> true;
+valid_ast_elem({Left, Right}) -> valid_ast_elem(Left) andalso valid_ast_elem(Right);
+valid_ast_elem({Atom, Meta, Args}) when is_atom(Atom), is_list(Meta), is_atom(Args) orelse is_list(Args) -> true;
+valid_ast_elem({Call, Meta, Args}) when is_list(Meta), is_list(Args) -> shallow_valid_ast(Call);
+valid_ast_elem(_Term) -> false.
+
 quote({unquote_splicing, _, [_]}, #elixir_quote{unquote=true}) ->
   argument_error(<<"unquote_splicing only works inside arguments and block contexts, "
     "wrap it in parens if you want it to work with one-liners">>);
@@ -283,8 +306,12 @@ do_quote({quote, Meta, [Opts, Arg]}, Q) when is_list(Meta) ->
 
   {'{}', [], [quote, meta(NewMeta, Q), [TOpts, TArg]]};
 
-do_quote({unquote, Meta, [Expr]}, #elixir_quote{unquote=true}) when is_list(Meta) ->
-  Expr;
+%
+do_quote({unquote, Meta, [Expr]}, #elixir_quote{unquote=true, shallow_validate=Validate}) when is_list(Meta) ->
+  case Validate of
+    true -> {{'.', Meta, [?MODULE, shallow_validate_ast]}, Meta, [Expr]};
+    false -> Expr
+  end;
 
 %% Aliases
 

lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs

@@ -405,12 +405,16 @@ defmodule Code.Normalizer.QuotedASTTest do
     end
 
     test "range" do
-      assert quoted_to_string(quote(do: unquote(-1..+2))) == "-1..2"
+      assert quoted_to_string(quote(do: -1..+2)) == "-1..+2"
       assert quoted_to_string(quote(do: Foo.integer()..3)) == "Foo.integer()..3"
-      assert quoted_to_string(quote(do: unquote(-1..+2//-3))) == "-1..2//-3"
+      assert quoted_to_string(quote(do: -1..+2//-3)) == "-1..+2//-3"
 
       assert quoted_to_string(quote(do: Foo.integer()..3//Bar.bat())) ==
                "Foo.integer()..3//Bar.bat()"
+
+      # invalid AST
+      assert quoted_to_string(-1..+2) == "-1..2"
+      assert quoted_to_string(-1..+2//-3) == "-1..2//-3"
     end
 
     test "when" do

lib/elixir/test/elixir/kernel/expansion_test.exs

@@ -2871,11 +2871,11 @@ defmodule Kernel.ExpansionTest do
 
   test "handles invalid expressions" do
     assert_compile_error(~r"invalid quoted expression: {1, 2, 3}", fn ->
-      expand_env(quote(do: unquote({1, 2, 3})), __ENV__)
+      expand_env({1, 2, 3}, __ENV__)
     end)
 
     assert_compile_error(~r"invalid quoted expression: #Function\<", fn ->
-      expand(quote(do: unquote({:sample, fn -> nil end})))
+      expand({:sample, fn -> nil end})
     end)
 
     assert_compile_error(~r"invalid pattern in match", fn ->

lib/elixir/test/elixir/kernel/quote_test.exs

@@ -789,4 +789,29 @@ defmodule Kernel.QuoteTest.HasUnquoteTest do
 
     refute :elixir_quote.has_unquotes(ast)
   end
+
+  test "unquote with invalid AST (shallow check)" do
+    for term <- [
+          %{unescaped: :map},
+          1..10,
+          {:bad_meta, nil, []},
+          {:bad_arg, nil, 1},
+          {:bad_tuple},
+          make_ref(),
+          [:improper | :list],
+          [nested: {}]
+        ] do
+      message = """
+      tried to unquote invalid AST: #{inspect(term)}
+      Did you forget to escape term using Macro.escape/1?\
+      """
+
+      assert_raise ArgumentError, message, fn -> quote do: unquote(term) end
+    end
+  end
+
+  test "unquote with invalid AST is not checked deeply" do
+    assert quote do: unquote(foo: [1 | 2]) == [foo: [1 | 2]]
+    assert quote do: unquote(foo: [bar: %{}]) == [foo: [bar: %{}]]
+  end
 end

lib/elixir/test/elixir/macro_test.exs

@@ -839,7 +839,7 @@ defmodule MacroTest do
     end
 
     test "converts invalid AST with inspect" do
-      assert Macro.to_string(quote do: unquote(1..3)) == "1..3"
+      assert Macro.to_string(1..3) == "1..3"
     end
   end
 
@@ -1172,12 +1172,16 @@ defmodule MacroTest do
     end
 
     test "range" do
-      assert macro_to_string(quote(do: unquote(-1..+2))) == "-1..2"
+      assert macro_to_string(quote(do: -1..+2)) == "-1..+2"
       assert macro_to_string(quote(do: Foo.integer()..3)) == "Foo.integer()..3"
-      assert macro_to_string(quote(do: unquote(-1..+2//-3))) == "-1..2//-3"
+      assert macro_to_string(quote(do: -1..+2//-3)) == "-1..+2//-3"
 
       assert macro_to_string(quote(do: Foo.integer()..3//Bar.bat())) ==
                "Foo.integer()..3//Bar.bat()"
+
+      # invalid AST
+      assert macro_to_string(-1..+2) == "-1..2"
+      assert macro_to_string(-1..+2//-3) == "-1..2//-3"
     end
 
     test "when" do

lib/mix/test/mix/tasks/xref_test.exs

@@ -63,7 +63,13 @@ defmodule Mix.Tasks.XrefTest do
       }
 
       output = [
-        %{callee: {A, :b, 1}, caller_module: B, file: "lib/b.ex", line: 3}
+        %{callee: {A, :b, 1}, caller_module: B, file: "lib/b.ex", line: 3},
+        %{
+          callee: {:elixir_quote, :shallow_validate_ast, 1},
+          caller_module: A,
+          file: "lib/a.ex",
+          line: 4
+        }
       ]
 
       assert_all_calls(files, output)