Move match vars handling and warnings to elixir_expand

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

Author: José Valim

lib/elixir/lib/macro/env.ex

@@ -44,6 +44,8 @@ defmodule Macro.Env do
 
     * `export_vars` - a list keeping all variables to be exported in a
       construct (may be `nil`)
+    * `match_vars` - a list of variables defined in a given match (is
+      `nil` when not inside a match)
     * `prematch_vars` - a list of variables defined before a match (is
       `nil` when not inside a match)
 
@@ -64,6 +66,7 @@ defmodule Macro.Env do
   @type local :: atom | nil
 
   @opaque export_vars :: vars | nil
+  @opaque match_vars :: vars | nil
   @opaque prematch_vars :: vars | nil
 
   @type t :: %{__struct__: __MODULE__,
@@ -80,6 +83,7 @@ defmodule Macro.Env do
                context_modules: context_modules,
                vars: vars,
                export_vars: export_vars,
+               match_vars: match_vars,
                prematch_vars: prematch_vars,
                lexical_tracker: lexical_tracker}
 
@@ -99,6 +103,7 @@ defmodule Macro.Env do
       vars: [],
       lexical_tracker: nil,
       export_vars: nil,
+      match_vars: nil,
       prematch_vars: nil}
   end
 

lib/elixir/src/elixir.hrl

@@ -11,7 +11,6 @@
   caller=false,            %% when true, it means caller was invoked
   vars=#{},                %% a map of defined variables and their alias
   backup_vars=nil,         %% a copy of vars to be used on ^var
-  match_vars=nil,          %% a set of all variables defined in a particular match
   export_vars=nil,         %% a dict of all variables defined in a particular clause
   extra_guards=nil,        %% extra guards from args expansion
   counter=#{},             %% a map counting the variables defined

lib/elixir/src/elixir_clauses.erl

@@ -9,13 +9,13 @@
 
 match(Fun, Expr, #{context := match} = E) ->
   Fun(Expr, E);
-match(Fun, Expr, #{context := Context, prematch_vars := nil, vars := Vars} = E) ->
-  {EExpr, EE} = Fun(Expr, E#{context := match, prematch_vars := Vars}),
-  {EExpr, EE#{context := Context, prematch_vars := nil}}.
+match(Fun, Expr, #{context := Context, match_vars := nil, prematch_vars := nil, vars := Vars} = E) ->
+  {EExpr, EE} = Fun(Expr, E#{context := match, match_vars := [], prematch_vars := Vars}),
+  {EExpr, EE#{context := Context, match_vars := nil, prematch_vars := nil}}.
 
 def({Meta, Args, Guards, Body}, E) ->
-  {EArgs, EA}   = elixir_expand:expand(Args, E#{context := match}),
-  {EGuards, EG} = guard(Guards, EA#{context := guard}),
+  {EArgs, EA}   = elixir_expand:expand(Args, E#{context := match, match_vars := []}),
+  {EGuards, EG} = guard(Guards, EA#{context := guard, match_vars := nil}),
   {EBody, _}    = elixir_expand:expand(Body, EG#{context := ?key(E, context)}),
   {Meta, EArgs, EGuards, EBody}.
 

lib/elixir/src/elixir_env.erl

@@ -19,6 +19,7 @@ new() ->
     lexical_tracker => nil,                %% holds the lexical tracker PID
     vars => [],                            %% a set of defined variables
     export_vars => nil,                    %% a set of variables to be exported in some constructs
+    match_vars => nil,                     %% a set of variables defined in the current match
     prematch_vars => nil}.                 %% a set of variables defined before the current match
 
 linify({Line, Env}) ->

lib/elixir/src/elixir_erl_clauses.erl

@@ -20,12 +20,9 @@ get_clauses(Key, Keyword, As, AllowNil) ->
 
 %% Translate matches
 
-match(Fun, Args, #elixir_erl{context=Context, match_vars=MatchVars,
-    backup_vars=BackupVars, vars=Vars} = S) when Context /= match ->
-  {Result, NewS} = match(Fun, Args, S#elixir_erl{context=match,
-                         match_vars=#{}, backup_vars=Vars}),
-  {Result, NewS#elixir_erl{context=Context,
-      match_vars=MatchVars, backup_vars=BackupVars}};
+match(Fun, Args, #elixir_erl{context=Context, backup_vars=BackupVars, vars=Vars} = S) when Context /= match ->
+  {Result, NewS} = match(Fun, Args, S#elixir_erl{context=match, backup_vars=Vars}),
+  {Result, NewS#elixir_erl{context=Context, backup_vars=BackupVars}};
 match(Fun, Args, S) -> Fun(Args, S).
 
 %% Translate clauses with args, guards and expressions

lib/elixir/src/elixir_erl_pass.erl

@@ -153,7 +153,6 @@ translate({for, Meta, [_ | _] = Args}, S) ->
 translate({'^', Meta, [{Name, VarMeta, Kind}]}, #elixir_erl{context=match, file=File} = S) when is_atom(Name), is_atom(Kind) ->
   Tuple = {Name, var_kind(VarMeta, Kind)},
   {ok, {Value, _Counter, Safe}} = maps:find(Tuple, S#elixir_erl.backup_vars),
-  elixir_erl_var:warn_underscored_var_access(VarMeta, File, Name),
   elixir_erl_var:warn_unsafe_var(VarMeta, File, Name, Safe),
 
   PAnn = ?ann(Meta),

lib/elixir/src/elixir_erl_var.erl

@@ -1,9 +1,9 @@
 %% Convenience functions used to manipulate scope and its variables.
 -module(elixir_erl_var).
--export([translate/4, build/2, context_info/1,
+-export([translate/4, build/2,
   load_binding/2, dump_binding/2,
   mergev/2, mergec/2, merge_vars/2, merge_opt_vars/2,
-  warn_unsafe_var/4, warn_underscored_var_access/3, format_error/1
+  warn_unsafe_var/4, format_error/1
 ]).
 -include("elixir.hrl").
 
@@ -13,22 +13,26 @@ translate(Meta, Name, Kind, S) when is_atom(Kind); is_integer(Kind) ->
   Ann = ?ann(Meta),
   Tuple = {Name, Kind},
   Vars = S#elixir_erl.vars,
+  BackupVars = S#elixir_erl.backup_vars,
 
-  {Current, Exists, Safe} =
+  {Current, Safe} =
     case maps:find({Name, Kind}, Vars) of
-      {ok, {VarC, _, VarS}} -> {VarC, true, VarS};
-      error -> {nil, false, true}
+      {ok, {VarC, _, VarS}} -> {VarC, VarS};
+      error -> {nil, true}
     end,
 
   case S#elixir_erl.context of
     match ->
-      MatchVars = S#elixir_erl.match_vars,
-
-      case Exists andalso maps:get(Tuple, MatchVars, false) of
-        true ->
-          warn_underscored_var_repeat(Meta, S#elixir_erl.file, Name, Kind),
+      Previous =
+        case maps:find({Name, Kind}, BackupVars) of
+          {ok, {BackupVarC, _, _}} -> BackupVarC;
+          error -> nil
+        end,
+
+      if
+        Current /= nil, Current /= Previous ->
           {{var, Ann, Current}, S};
-        false ->
+        true ->
           %% We attempt to give vars a nice name because we
           %% still use the unused vars warnings from erl_lint.
           %%
@@ -44,7 +48,6 @@ translate(Meta, Name, Kind, S) when is_atom(Kind); is_integer(Kind) ->
 
           FS = NS#elixir_erl{
             vars=maps:put(Tuple, {NewVar, Counter, true}, Vars),
-            match_vars=maps:put(Tuple, true, MatchVars),
             export_vars=case S#elixir_erl.export_vars of
               nil -> nil;
               EV  -> maps:put(Tuple, {NewVar, Counter, true}, EV)
@@ -53,8 +56,7 @@ translate(Meta, Name, Kind, S) when is_atom(Kind); is_integer(Kind) ->
 
           {{var, Ann, NewVar}, FS}
       end;
-    _  when Exists ->
-      warn_underscored_var_access(Meta, S#elixir_erl.file, Name),
+    _  when Current /= nil ->
       warn_unsafe_var(Meta, S#elixir_erl.file, Name, Safe),
       {{var, Ann, Current}, S}
   end.
@@ -69,43 +71,14 @@ build(Key, #elixir_erl{counter=Counter} = S) ->
    Cnt,
    S#elixir_erl{counter=maps:put(Key, Cnt, Counter)}}.
 
-context_info(Kind) when Kind == nil; is_integer(Kind) -> "";
-context_info(Kind) -> io_lib:format(" (context ~ts)", [elixir_aliases:inspect(Kind)]).
-
-warn_underscored_var_repeat(Meta, File, Name, Kind) ->
-  Warn = should_warn(Meta),
-  case atom_to_list(Name) of
-    "_@" ++ _ ->
-      ok; %% Automatically generated variables
-    "_" ++ _ when Warn ->
-      elixir_errors:form_warn(Meta, File, ?MODULE, {unused_match, Name, Kind});
-    _ ->
-      ok
-  end.
-
 warn_unsafe_var(Meta, File, Name, Safe) ->
-  Warn = should_warn(Meta),
-  if
-    (not Safe) and Warn ->
-      elixir_errors:form_warn(Meta, File, ?MODULE, {unsafe_var, Name});
+  case (not Safe) andalso (lists:keyfind(generated, 1, Meta) /= {generated, true}) of
     true ->
+      elixir_errors:form_warn(Meta, File, ?MODULE, {unsafe_var, Name});
+    false ->
       ok
   end.
 
-warn_underscored_var_access(Meta, File, Name) ->
-  Warn = should_warn(Meta),
-  case atom_to_list(Name) of
-    "_@" ++ _ ->
-      ok; %% Automatically generated variables
-    "_" ++ _ when Warn ->
-      elixir_errors:form_warn(Meta, File, ?MODULE, {underscore_var_access, Name});
-    _ ->
-      ok
-  end.
-
-should_warn(Meta) ->
-  lists:keyfind(generated, 1, Meta) /= {generated, true}.
-
 %% SCOPE MERGING
 
 %% Receives two scopes and return a new scope based on
@@ -193,13 +166,6 @@ dump_binding(Binding, #elixir_erl{vars=Vars}) ->
 
 %% Errors
 
-format_error({unused_match, Name, Kind}) ->
-  io_lib:format("the underscored variable \"~ts\"~ts appears more than once in a "
-                "match. This means the pattern will only match if all \"~ts\" bind "
-                "to the same value. If this is the intended behaviour, please "
-                "remove the leading underscore from the variable name, otherwise "
-                "give the variables different names", [Name, context_info(Kind), Name]);
-
 format_error({unsafe_var, Name}) ->
   io_lib:format("the variable \"~ts\" is unsafe as it has been set inside "
                 "one of: case, cond, receive, if, and, or, &&, ||. "
@@ -214,10 +180,4 @@ format_error({unsafe_var, Name}) ->
                 "        1 -> :one\n"
                 "        2 -> :two\n"
                 "      end\n\n"
-                "Unsafe variable found at:", [Name]);
-
-format_error({underscore_var_access, Name}) ->
-  io_lib:format("the underscored variable \"~ts\" is used after being set. "
-                "A leading underscore indicates that the value of the variable "
-                "should be ignored. If this is intended please rename the "
-                "variable to remove the underscore", [Name]).
+                "Unsafe variable found at:", [Name]).

lib/elixir/src/elixir_expand.erl

@@ -314,8 +314,9 @@ expand({'^', Meta, [Arg]}, #{context := match, prematch_vars := PrematchVars} =
       %% If the variable was defined, then we return the expanded ^, otherwise
       %% we raise. We cannot use the expanded env because it would contain the
       %% variable.
-      case lists:member({VarName, var_kind(VarMeta, Kind)}, PrematchVars) of
+      case lists:member({VarName, var_context(VarMeta, Kind)}, PrematchVars) of
         true ->
+          warn_underscored_var_access(VarMeta, VarName, Kind, E),
           {{'^', Meta, [Var]}, EA};
         false ->
           form_error(Meta, ?key(EA, file), ?MODULE, {unbound_variable_pin, VarName})
@@ -331,17 +332,28 @@ expand({'_', _Meta, Kind} = Var, #{context := match} = E) when is_atom(Kind) ->
 expand({'_', Meta, Kind}, E) when is_atom(Kind) ->
   form_error(Meta, ?key(E, file), ?MODULE, unbound_underscore);
 
-expand({Name, Meta, Kind} = Var, #{context := match, export_vars := Export} = E) when is_atom(Name), is_atom(Kind) ->
-  Pair      = {Name, var_kind(Meta, Kind)},
-  NewVars   = ordsets:add_element(Pair, ?key(E, vars)),
-  NewExport = case (Export /= nil) of
-    true  -> ordsets:add_element(Pair, Export);
-    false -> Export
-  end,
-  {Var, E#{vars := NewVars, export_vars := NewExport}};
+expand({Name, Meta, Kind} = Var, #{context := match} = E) when is_atom(Name), is_atom(Kind) ->
+  #{vars := Vars, match_vars := Match, export_vars := Export} = E,
+  Pair = {Name, var_context(Meta, Kind)},
+  NewVars = ordsets:add_element(Pair, Vars),
+
+  NewExport =
+    case (Export /= nil) of
+      true  -> ordsets:add_element(Pair, Export);
+      false -> Export
+    end,
+
+  NewMatch =
+    case lists:member(Pair, Match) of
+      true -> warn_underscored_var_repeat(Meta, Name, Kind, E), Match;
+      false -> [Pair | Match]
+    end,
+
+  {Var, E#{vars := NewVars, match_vars := NewMatch, export_vars := NewExport}};
 expand({Name, Meta, Kind} = Var, #{vars := Vars} = E) when is_atom(Name), is_atom(Kind) ->
-  case lists:member({Name, var_kind(Meta, Kind)}, Vars) of
+  case lists:member({Name, var_context(Meta, Kind)}, Vars) of
     true ->
+      warn_underscored_var_access(Meta, Name, Kind, E),
       {Var, E};
     false ->
       case lists:keyfind(var, 1, Meta) of
@@ -555,7 +567,7 @@ expand_args(Args, E) ->
 
 %% Match/var helpers
 
-var_kind(Meta, Kind) ->
+var_context(Meta, Kind) ->
   case lists:keyfind(counter, 1, Meta) of
     {counter, Counter} -> Counter;
     false -> Kind
@@ -814,6 +826,30 @@ assert_no_underscore_clause_in_cond(_Other, _E) ->
 
 %% Warnings
 
+warn_underscored_var_repeat(Meta, Name, Kind, E) ->
+  Warn = should_warn(Meta),
+  case atom_to_list(Name) of
+    "_" ++ _ when Warn ->
+      elixir_errors:form_warn(Meta, ?key(E, file), ?MODULE, {underscored_var_repeat, Name, Kind});
+    _ ->
+      ok
+  end.
+
+warn_underscored_var_access(Meta, Name, Kind, E) ->
+  Warn = should_warn(Meta),
+  case atom_to_list(Name) of
+    "_" ++ _ when Warn ->
+      elixir_errors:form_warn(Meta, ?key(E, file), ?MODULE, {underscored_var_access, Name, Kind});
+    _ ->
+      ok
+  end.
+
+context_info(Kind) when Kind == nil; is_integer(Kind) -> "";
+context_info(Kind) -> io_lib:format(" (context ~ts)", [elixir_aliases:inspect(Kind)]).
+
+should_warn(Meta) ->
+  lists:keyfind(generated, 1, Meta) /= {generated, true}.
+
 format_error({useless_literal, Term}) ->
   io_lib:format("code block contains unused literal ~ts "
                 "(remove the literal or assign it to _ to avoid warnings)",
@@ -866,7 +902,7 @@ format_error({undefined_var, Name, Kind}) ->
   Message =
     "expected variable \"~ts\"~ts to expand to an existing variable "
     "or be part of a match",
-  io_lib:format(Message, [Name, elixir_erl_var:context_info(Kind)]);
+  io_lib:format(Message, [Name, context_info(Kind)]);
 format_error(underscore_in_cond) ->
   "unbound variable _ inside \"cond\". If you want the last clause to always match, "
     "you probably meant to use: true ->";
@@ -918,4 +954,15 @@ format_error({options_are_not_keyword, Kind, Opts}) ->
   io_lib:format("invalid options for ~s, expected a keyword list, got: ~ts",
                 [Kind, 'Elixir.Macro':to_string(Opts)]);
 format_error({undefined_function, Name, Args}) ->
-  io_lib:format("undefined function ~ts/~B", [Name, length(Args)]).
+  io_lib:format("undefined function ~ts/~B", [Name, length(Args)]);
+format_error({underscored_var_repeat, Name, Kind}) ->
+  io_lib:format("the underscored variable \"~ts\"~ts appears more than once in a "
+                "match. This means the pattern will only match if all \"~ts\" bind "
+                "to the same value. If this is the intended behaviour, please "
+                "remove the leading underscore from the variable name, otherwise "
+                "give the variables different names", [Name, context_info(Kind), Name]);
+format_error({underscored_var_access, Name, Kind}) ->
+  io_lib:format("the underscored variable \"~ts\"~ts is used after being set. "
+                "A leading underscore indicates that the value of the variable "
+                "should be ignored. If this is intended please rename the "
+                "variable to remove the underscore", [Name, context_info(Kind)]).