Skip to content

Commit

Permalink
Always warn about exported variables from constructs without clauses
Browse files Browse the repository at this point in the history 
There is never a good reason for placing variable bindings inside the
arguments of an expression.
  • Loading branch information
@richcarl
richcarl committed Dec 2, 2024
1 parent c0b3be8 commit da363aa
Showing 1 changed file with 71 additions and 26 deletions.
97 changes: 71 additions & 26 deletions lib/stdlib/src/erl_lint.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,13 @@ format_error_1({unbound_var,V,GuessV}) ->
format_error_1({unsafe_var,V,{What,Where}}) ->
{~"variable ~w unsafe in ~w ~s",
[V,What,format_where(Where)]};
format_error_1({exported_subexpr_var,V,{What,Where}}) ->
{~"""
variable ~w exported from ~w ~s.
Exporting bindings from subexpressions is deprecated and
may be removed in a future version of Erlang/OTP.
You should move the binding of ~w before the ~w.
""", [V,What,format_where(Where),V,What]};
format_error_1({exported_var,V,{What,Where}}) ->
{~"variable ~w exported from ~w ~s",
[V,What,format_where(Where)]};
Expand Down Expand Up @@ -2679,21 +2686,27 @@ expr({atom,Anno,A}, _Vt, St) ->
{[],keyword_warning(Anno, A, St)};
expr({string,_Anno,_S}, _Vt, St) -> {[],St};
expr({nil,_Anno}, _Vt, St) -> {[],St};
expr({cons,_Anno,H,T}, Vt, St) ->
expr_list([H,T], Vt, St);
expr({cons,Anno,H,T}, Vt, St) ->
vtupd_export_expr_list({list, Anno}, [H, T], Vt, St);
expr({lc,_Anno,E,Qs}, Vt, St) ->
handle_comprehension(E, Qs, Vt, St);
expr({bc,_Anno,E,Qs}, Vt, St) ->
handle_comprehension(E, Qs, Vt, St);
expr({mc,_Anno,E,Qs}, Vt, St) ->
handle_comprehension(E, Qs, Vt, St);
expr({tuple,_Anno,Es}, Vt, St) ->
expr_list(Es, Vt, St);
expr({map,_Anno,Es}, Vt, St) ->
map_fields(Es, Vt, check_assoc_fields(Es, St), fun expr_list/3);
expr({tuple,Anno,Es}, Vt, St) ->
vtupd_export_expr_list({tuple, Anno}, Es, Vt, St);
expr({map,Anno,Es}, Vt, St) ->
map_fields(Es, Vt, check_assoc_fields(Es, St),
fun(Es0, Vt0, St0) ->
vtupd_export_expr_list({map, Anno}, Es0, Vt0, St0)
end);
expr({map,Anno,Src,Es}, Vt, St) ->
{Svt,St1} = expr(Src, Vt, St),
{Fvt,St2} = map_fields(Es, Vt, St1, fun expr_list/3),
{Svt,St1} = vtupd_export_expr_list({map, Anno}, [Src], Vt, St),
{Fvt,St2} = map_fields(Es, Vt, St1,
fun(Es0, Vt0, St0) ->
vtupd_export_expr_list({map, Anno}, Es0, Vt0, St0)
end),
{vtupdate(Svt, Fvt), warn_if_literal_update(Anno, Src, St2)};
expr({record_index,Anno,Name,Field}, _Vt, St) ->
check_record(Anno, Name, St,
Expand All @@ -2720,11 +2733,13 @@ expr({record,Anno,Rec,Name,Upds}, Vt, St0) ->
no -> {vtmerge(Rvt, Usvt), warn_if_literal_update(Anno, Rec, St2)};
WildAnno -> {[],add_error(WildAnno, {wildcard_in_update,Name}, St2)}
end;
expr({bin,_Anno,Fs}, Vt, St) ->
expr_bin(Fs, Vt, St, fun expr/3);
expr({block,_Anno,Es}, Vt, St) ->
expr({bin,Anno,Fs}, Vt, St) ->
{Vt1, St1} = expr_bin(Fs, Vt, St, fun expr/3),
{vtupd_export({binary, Anno}, Vt1, Vt), St1};
expr({block,Anno,Es}, Vt, St) ->
%% Unfold block into a sequence.
exprs(Es, Vt, St);
{Vt1, St1} = exprs(Es, Vt, St),
{vtupd_export({'begin', Anno}, Vt1, Vt), St1};
expr({'if',Anno,Cs}, Vt, St) ->
icrt_clauses(Cs, {'if',Anno}, Vt, St);
expr({'case',Anno,E,Cs}, Vt, St0) ->
Expand Down Expand Up @@ -2786,7 +2801,7 @@ expr({call,Anno,{remote,_Ar,{atom,_Am,M},{atom,Af,F}},As}, Vt, St0) ->
St2 = check_remote_function(Anno, M, F, As, St1),
St3 = check_module_name(M, Anno, St2),
St4 = check_unexported_function(Anno, M, F, length(As), St3),
expr_list(As, Vt, St4);
vtupd_export_expr_list({call, Anno}, As, Vt, St4);
expr({call,Anno,{remote,_Ar,M,F},As}, Vt, St0) ->
St1 = keyword_warning(Anno, M, St0),
St2 = keyword_warning(Anno, F, St1),
Expand All @@ -2796,14 +2811,14 @@ expr({call,Anno,{remote,_Ar,M,F},As}, Vt, St0) ->
_ ->
St2
end,
expr_list([M,F|As], Vt, St3);
vtupd_export_expr_list({call, Anno}, [M, F | As], Vt, St3);
expr({call,Anno,{atom,Aa,F},As}, Vt, St0) ->
St1 = keyword_warning(Aa, F, St0),
{Asvt,St2} = expr_list(As, Vt, St1),
{Asvt,St2} = vtupd_export_expr_list({call, Anno}, As, Vt, St1),
{Asvt, check_call(Anno, F, As, Aa, St2)};
expr({call,Anno,F,As}, Vt, St0) ->
St = warn_invalid_call(Anno,F,St0),
expr_list([F|As], Vt, St); %They see the same variables
vtupd_export_expr_list({call, Anno}, [F | As], Vt, St); %They see the same variables
expr({'try',Anno,Es,Scs,Ccs,As}, Vt, St0) ->
%% The only exports we allow are from the try expressions to the
%% success clauses.
Expand Down Expand Up @@ -2846,19 +2861,19 @@ expr({'maybe',MaybeAnno,Es,{'else',ElseAnno,Cs}}, Vt, St) ->
Cvt2 = vtmerge(Cvt0, Cvt1),
{vtmerge(Evt2, Cvt2),St2};
%% No comparison or boolean operators yet.
expr({op,_Anno,_Op,A}, Vt, St) ->
expr(A, Vt, St);
expr({op,Anno,Op,A}, Vt, St) ->
vtupd_export_expr_list({Op, Anno}, [A], Vt, St);
expr({op,Anno,Op,L,R}, Vt, St0) when Op =:= 'orelse'; Op =:= 'andalso' ->
{Evt1,St1} = expr(L, Vt, St0),
{Evt1, St1} = vtupd_export_expr_list({Op, Anno}, [L], Vt, St0),
Vt1 = vtupdate(Evt1, Vt),
{Evt2,St2} = expr(R, Vt1, St1),
Evt3 = vtupd_unsafe({Op, Anno}, Evt2, Vt1),
{vtmerge(Evt1, Evt3),St2};
expr({op,_Anno,EqOp,L,R}, Vt, St0) when EqOp =:= '=:='; EqOp =:= '=/=' ->
expr({op,Anno,EqOp,L,R}, Vt, St0) when EqOp =:= '=:='; EqOp =:= '=/=' ->
St = expr_check_match_zero(R, expr_check_match_zero(L, St0)),
expr_list([L,R], Vt, St); %They see the same variables
expr({op,_Anno,_Op,L,R}, Vt, St) ->
expr_list([L,R], Vt, St); %They see the same variables
vtupd_export_expr_list({EqOp, Anno}, [L, R], Vt, St); %They see the same variables
expr({op,Anno,Op,L,R}, Vt, St) ->
vtupd_export_expr_list({Op, Anno}, [L, R], Vt, St); %They see the same variables
%% The following are not allowed to occur anywhere!
expr({remote,_Anno,M,_F}, _Vt, St) ->
{[],add_error(erl_parse:first_anno(M), illegal_expr, St)};
Expand Down Expand Up @@ -2948,6 +2963,12 @@ expr_list(Es, Vt, St0) ->
vtmerge_pat(Evt, Esvt, St2)
end, {[], St0}, Es).

%% as expr_list but mark new vars as exported

vtupd_export_expr_list(Where, Es, Vt, St) ->
{Evt, St1} = expr_list(Es, Vt, St),
{vtupd_export(Where, Evt, Vt), St1}.

record_expr(Anno, Rec, Vt, St0) ->
St1 = warn_invalid_record(Anno, Rec, St0),
expr(Rec, Vt, St1).
Expand Down Expand Up @@ -4349,12 +4370,18 @@ do_expr_var(V, Anno, Vt, St) ->
{[{V,{bound,used,As}}],
add_error(Anno, {unsafe_var,V,In}, St)};
{ok,{{export,From},_Usage,As}} ->
case is_warn_enabled(export_vars, St) of
case exported_subexpr_var(From) of
true ->
{[{V,{bound,used,As}}],
add_warning(Anno, {exported_var,V,From}, St)};
add_warning(Anno, {exported_subexpr_var,V,From}, St)};
false ->
{[{V,{{export,From},used,As}}],St}
case is_warn_enabled(export_vars, St) of
true ->
{[{V,{bound,used,As}}],
add_warning(Anno, {exported_var,V,From}, St)};
false ->
{[{V,{{export,From},used,As}}],St}
end
end;
{ok,{stacktrace,_Usage,As}} ->
{[{V,{bound,used,As}}],
Expand All @@ -4377,6 +4404,14 @@ exported_var(Anno, V, From, St) ->
false -> St
end.

%% warn about exporting from non-block subexpressions
exported_subexpr_var({'if',_}) -> false;
exported_subexpr_var({'case',_}) -> false;
exported_subexpr_var({'receive',_}) -> false;
exported_subexpr_var({'try',_}) -> false;
exported_subexpr_var({'begin',_}) -> false;
exported_subexpr_var(_) -> true.

shadow_vars(Vt, Vt0, In, St0) ->
case is_warn_enabled(shadow_vars, St0) of
true ->
Expand Down Expand Up @@ -4442,6 +4477,16 @@ vtunsafe({Tag,Anno}, Uvt, Vt) ->
vtupd_unsafe(Where, NewVt, OldVt) ->
vtupdate(vtunsafe(Where, NewVt, OldVt), NewVt).

%% vtexport(From, UpdVarTable, VarTable) -> ExpVarTable.
%% Return all new variables in UpdVarTable as exported.

vtexport({Tag, FileLine}, Uvt, Vt) ->
Line = erl_anno:location(FileLine),
[{V, {{export, {Tag, Line}}, U, Ls}} || {V, {_, U, Ls}} <- vtnew(Uvt, Vt)].

vtupd_export(Where, NewVt, OldVt) ->
vtupdate(vtexport(Where, NewVt, OldVt), NewVt).

%% vtmerge(VarTable, VarTable) -> VarTable.
%% Merge two variables tables generating a new vartable. Give priority to
%% errors then warnings.
Expand Down

0 comments on commit da363aa

Please sign in to comment.