Error on undefined variables in bitstring segments (#8598)

Binary/bitstring matching allows to dynamically define the
`size` of the binary in certain conditions:

  * if the `size` variable is defined prior to the pattern
    match:

        iex> size = 8
        iex> <<a::size(size), rest::binary>> = "hello"
        iex> a
        104

  * if the `size` variable is matched within the same
    binary/bitstring match, prior to its use:

        iex> <<name_size::size(8), name::binary-size(name_size), _rest::binary>> = <<5, "Frank the Walrus">>
        iex> name
        "Frank"

Other cases are considered illegal patterns, for example:

    {name_size, <<name::binary-size(name_size), _rest::binary>>} = {5, "Frank the Walrus"}

This commit raises a `CompileError: undefined variable ...` for such cases.

Signed-off-by: José Valim <jose.valim@plataformatec.com.br>

Author: fertapric

lib/elixir/lib/kernel/special_forms.ex

@@ -246,12 +246,17 @@ defmodule Kernel.SpecialForms do
       iex> {name, species}
       {"Frank", "Walrus"}
 
-  And the variable can be defined in the match itself:
+  And the variable can be defined in the match itself (prior to its use):
 
       iex> <<name_size::size(8), name::binary-size(name_size), " the ", species::binary>> = <<5, "Frank the Walrus">>
       iex> {name, species}
       {"Frank", "Walrus"}
 
+  However, the size cannot be defined in the match outside the binary/bitstring match:
+
+      {name_size, <<name::binary-size(name_size), _rest::binary>>} = {5, <<"Frank the Walrus">>}
+      ** (CompileError): undefined variable "name_size" in bitstring segment
+
   Failing to specify the size for the non-last causes compilation to fail:
 
       <<name::binary, " the ", species::binary>> = <<"Frank the Walrus">>

lib/elixir/src/elixir_bitstring.erl

@@ -7,7 +7,7 @@ expand(Meta, Args, E, RequireSize) ->
   case ?key(E, context) of
     match ->
       {EArgs, Alignment, EE} =
-        expand(Meta, fun elixir_expand:expand/2, Args, [], E, 0, RequireSize),
+        expand(Meta, fun elixir_expand:expand/2, Args, [], E, E, 0, RequireSize),
 
       case find_match(EArgs) of
         false ->
@@ -17,13 +17,13 @@ expand(Meta, Args, E, RequireSize) ->
       end;
     _ ->
       {EArgs, Alignment, {_EC, EV}} =
-        expand(Meta, fun elixir_expand:expand_arg/2, Args, [], {E, E}, 0, RequireSize),
+        expand(Meta, fun elixir_expand:expand_arg/2, Args, [], {E, E}, E, 0, RequireSize),
       {{'<<>>', [{alignment, Alignment} | Meta], EArgs}, EV}
   end.
 
-expand(_BitstrMeta, _Fun, [], Acc, E, Alignment, _RequireSize) ->
+expand(_BitstrMeta, _Fun, [], Acc, E, _OriginalE, Alignment, _RequireSize) ->
   {lists:reverse(Acc), Alignment, E};
-expand(BitstrMeta, Fun, [{'::', Meta, [Left, Right]} | T], Acc, E, Alignment, RequireSize) ->
+expand(BitstrMeta, Fun, [{'::', Meta, [Left, Right]} | T], Acc, E, OriginalE, Alignment, RequireSize) ->
   {ELeft, EL} = expand_expr(Meta, Left, Fun, E),
 
   %% Variables defined outside the binary can be accounted
@@ -38,7 +38,7 @@ expand(BitstrMeta, Fun, [{'::', Meta, [Left, Right]} | T], Acc, E, Alignment, Re
     end,
 
   EType = expr_type(ELeft),
-  {ERight, EAlignment, ES} = expand_specs(EType, Meta, Right, EM, RequireSize or MatchSize),
+  {ERight, EAlignment, ES} = expand_specs(EType, Meta, Right, EM, OriginalE, RequireSize or MatchSize),
 
   EE =
     case EL of
@@ -65,15 +65,15 @@ expand(BitstrMeta, Fun, [{'::', Meta, [Left, Right]} | T], Acc, E, Alignment, Re
         prepend_unless_bitstring_in_match(EType, Meta, ELeft, ERight, Acc, E)
     end,
 
-  expand(BitstrMeta, Fun, T, EAcc, EE, alignment(Alignment, EAlignment), RequireSize);
-expand(BitstrMeta, Fun, [{_, Meta, _} = H | T], Acc, E, Alignment, RequireSize) ->
+  expand(BitstrMeta, Fun, T, EAcc, EE, OriginalE, alignment(Alignment, EAlignment), RequireSize);
+expand(BitstrMeta, Fun, [{_, Meta, _} = H | T], Acc, E, OriginalE, Alignment, RequireSize) ->
   {Expr, ES} = expand_expr(Meta, H, Fun, E),
   {EAcc, EAlignment} = wrap_expr(Expr, Acc),
-  expand(BitstrMeta, Fun, T, EAcc, ES, alignment(Alignment, EAlignment), RequireSize);
-expand(Meta, Fun, [H | T], Acc, E, Alignment, RequireSize) ->
+  expand(BitstrMeta, Fun, T, EAcc, ES, OriginalE, alignment(Alignment, EAlignment), RequireSize);
+expand(Meta, Fun, [H | T], Acc, E, OriginalE, Alignment, RequireSize) ->
   {Expr, ES} = expand_expr(Meta, H, Fun, E),
   {EAcc, EAlignment} = wrap_expr(Expr, Acc),
-  expand(Meta, Fun, T, EAcc, ES, alignment(Alignment, EAlignment), RequireSize).
+  expand(Meta, Fun, T, EAcc, ES, OriginalE, alignment(Alignment, EAlignment), RequireSize).
 
 prepend_unless_bitstring_in_match(Type, Meta, Left, Right, Acc, E) ->
   Expr = {'::', Meta, [Left, Right]},
@@ -157,15 +157,15 @@ env_for_error(E) -> E.
 
 %% Expands and normalizes types of a bitstring.
 
-expand_specs(ExprType, Meta, Info, E, RequireSize) ->
+expand_specs(ExprType, Meta, Info, E, OriginalE, RequireSize) ->
   Default =
     #{size => default,
       unit => default,
       sign => default,
       type => default,
       endianness => default},
   {#{size := Size, unit := Unit, type := Type, endianness := Endianness, sign := Sign}, ES} =
-    expand_each_spec(Meta, unpack_specs(Info, []), Default, E),
+    expand_each_spec(Meta, unpack_specs(Info, []), Default, E, OriginalE),
   MergedType = type(Meta, ExprType, Type, E),
   validate_size_required(Meta, RequireSize, ExprType, MergedType, Size, ES),
   SizeAndUnit = size_and_unit(Meta, ExprType, Size, Unit, ES),
@@ -190,30 +190,30 @@ type(_, default, Type, _) ->
 type(Meta, Other, Value, E) ->
   form_error(Meta, ?key(E, file), ?MODULE, {bittype_mismatch, Value, Other, type}).
 
-expand_each_spec(Meta, [{Expr, _, Args} = H | T], Map, E) when is_atom(Expr) ->
+expand_each_spec(Meta, [{Expr, _, Args} = H | T], Map, E, OriginalE) when is_atom(Expr) ->
   case validate_spec(Expr, Args) of
     {Key, Arg} ->
       {Value, EE} = expand_spec_arg(Arg, E),
-      validate_spec_arg(Meta, Key, Value, EE),
+      validate_spec_arg(Meta, Key, Value, EE, OriginalE),
 
       case maps:get(Key, Map) of
         default -> ok;
         Value -> ok;
         Other -> form_error(Meta, ?key(E, file), ?MODULE, {bittype_mismatch, Value, Other, Key})
       end,
 
-      expand_each_spec(Meta, T, maps:put(Key, Value, Map), EE);
+      expand_each_spec(Meta, T, maps:put(Key, Value, Map), EE, OriginalE);
     none ->
       case 'Elixir.Macro':expand(H, elixir_env:linify({?line(Meta), E})) of
         H ->
           form_error(Meta, ?key(E, file), ?MODULE, {undefined_bittype, H});
         NewTypes ->
-          expand_each_spec(Meta, unpack_specs(NewTypes, []) ++ T, Map, E)
+          expand_each_spec(Meta, unpack_specs(NewTypes, []) ++ T, Map, E, OriginalE)
       end
   end;
-expand_each_spec(Meta, [Expr | _], _Map, E) ->
+expand_each_spec(Meta, [Expr | _], _Map, E, _OriginalE) ->
   form_error(Meta, ?key(E, file), ?MODULE, {undefined_bittype, Expr});
-expand_each_spec(_Meta, [], Map, E) ->
+expand_each_spec(_Meta, [], Map, E, _OriginalE) ->
   {Map, E}.
 
 unpack_specs({'-', _, [H, T]}, Acc) ->
@@ -251,17 +251,33 @@ validate_spec(_, _)          -> none.
 expand_spec_arg(Expr, E) when is_atom(Expr); is_integer(Expr) -> {Expr, E};
 expand_spec_arg(Expr, E) -> elixir_expand:expand(Expr, E).
 
-validate_spec_arg(Meta, size, Value, E) ->
+validate_spec_arg(Meta, size, Value, E, OriginalE) ->
   case Value of
-    {Var, _, Context} when is_atom(Var) and is_atom(Context) -> ok;
-    _ when is_integer(Value) -> ok;
-    _ -> form_error(Meta, ?key(E, file), ?MODULE, {bad_size_argument, Value})
+    {Var, _, Context} when is_atom(Var) and is_atom(Context) ->
+      case is_valid_spec_arg_var({Var, Context}, E, OriginalE) of
+        true -> ok;
+        false -> form_error(Meta, ?key(E, file), ?MODULE, {undefined_var_in_spec, Value})
+      end;
+    _ when is_integer(Value) ->
+      ok;
+    _ ->
+      form_error(Meta, ?key(E, file), ?MODULE, {bad_size_argument, Value})
   end;
-validate_spec_arg(Meta, unit, Value, E) when not is_integer(Value) ->
+validate_spec_arg(Meta, unit, Value, E, _OriginalE) when not is_integer(Value) ->
   form_error(Meta, ?key(E, file), ?MODULE, {bad_unit_argument, Value});
-validate_spec_arg(_Meta, _Key, _Value, _E) ->
+validate_spec_arg(_Meta, _Key, _Value, _E, _OriginalE) ->
   ok.
 
+is_valid_spec_arg_var(Var, E, #{context := match} = OriginalE) ->
+  case ?key(OriginalE, prematch_vars) of
+    #{Var := _} ->
+      true;
+    _ ->
+      maps:is_key(Var, ?key(E, current_vars)) andalso
+        not maps:is_key(Var, ?key(OriginalE, current_vars))
+  end;
+is_valid_spec_arg_var(_Var, _E, _OriginalE) -> true.
+
 validate_size_required(Meta, true, default, Type, default, E) when Type == binary; Type == bitstring ->
   form_error(Meta, ?key(E, file), ?MODULE, unsized_binary);
 validate_size_required(_, _, _, _, _, _) ->
@@ -375,4 +391,10 @@ format_error({nested_match, Expr}) ->
   Message =
     "cannot pattern match inside a bitstring "
       "that is already in match, got: ~ts",
-  io_lib:format(Message, ['Elixir.Macro':to_string(Expr)]).
+  io_lib:format(Message, ['Elixir.Macro':to_string(Expr)]);
+format_error({undefined_var_in_spec, Var}) ->
+  Message =
+    "undefined variable \"~ts\" in bitstring segment. If the size of the binary is a "
+      "variable, the variable must be defined prior to its use in the binary/bitstring match "
+      "itself, or outside the pattern match",
+  io_lib:format(Message, ['Elixir.Macro':to_string(Var)]).

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

@@ -2394,6 +2394,24 @@ defmodule Kernel.ExpansionTest do
       expand(code)
     end
 
+    assert_raise CompileError, ~r/undefined variable "foo"/, fn ->
+      code =
+        quote do
+          fn <<_::size(foo), foo::size(8)>> -> :ok end
+        end
+
+      expand(code)
+    end
+
+    assert_raise CompileError, ~r/undefined variable "foo" in bitstring segment/, fn ->
+      code =
+        quote do
+          fn foo, <<_::size(foo)>> -> :ok end
+        end
+
+      expand(code)
+    end
+
     message = ~r"size in bitstring expects an integer or a variable as argument, got: foo()"
 
     assert_raise CompileError, message, fn ->