Fix a bug in Dialyzer related to call-site analysis

Dialyzer's "dataflow" module is using information from the callgraph to
determine which functions may be called at a particular call-site. Unfortunately
this information can include functions that are certainly not among the possible
choices. We don't want to emit warnings in such cases, so a "reasonable"
compromise is to stay silent if there are many possible funs and at least one of
them can succeed.

Bug reported by Dan Gudmundsson, test shrunk down by Magnus Lång.

5 files changed, 82 insertions, 11 deletions

diff --git a/lib/dialyzer/src/dialyzer_dataflow.erl b/lib/dialyzer/src/dialyzer_dataflow.erl
index 5ab0c39c04..3349b12932 100644
--- a/lib/dialyzer/src/dialyzer_dataflow.erl
+++ b/lib/dialyzer/src/dialyzer_dataflow.erl
@@ -430,17 +430,35 @@ handle_apply(Tree, Map, State) ->
 
 handle_apply_or_call(FunInfoList, Args, ArgTypes, Map, Tree, State) ->
   None = t_none(),
+  %% Call-site analysis may be inaccurate and consider more funs than those that
+  %% are actually possible. If all of them are incorrect, then warnings can be
+  %% emitted. If at least one fun is ok, however, then no warning is emitted,
+  %% just in case the bad ones are not really possible. The last argument is
+  %% used for this, with the following encoding:
+  %%   Initial value: {none, []}
+  %%   First fun checked: {one, <List of warns>}
+  %%   More funs checked: {many, <List of warns>}
+  %% A '{one, []}' can only become '{many, []}'.
+  %% If at any point an fun does not add warnings, then the list is also
+  %% replaced with an empty list.
   handle_apply_or_call(FunInfoList, Args, ArgTypes, Map, Tree, State,
-		       [None || _ <- ArgTypes], None, false).
+		       [None || _ <- ArgTypes], None, false, {none, []}).
 
 handle_apply_or_call([{local, external}|Left], Args, ArgTypes, Map, Tree, State,
-		     _AccArgTypes, _AccRet, _HadExternal) ->
+		     _AccArgTypes, _AccRet, _HadExternal, Warns) ->
+  {HowMany, _} = Warns,
+  NewHowMany =
+    case HowMany of
+      none -> one;
+      _ -> many
+    end,
+  NewWarns = {NewHowMany, []},      
   handle_apply_or_call(Left, Args, ArgTypes, Map, Tree, State,
-		       ArgTypes, t_any(), true);
+		       ArgTypes, t_any(), true, NewWarns);
 handle_apply_or_call([{TypeOfApply, {Fun, Sig, Contr, LocalRet}}|Left],
 		     Args, ArgTypes, Map, Tree,
                      #state{opaques = Opaques} = State,
-                     AccArgTypes, AccRet, HadExternal) ->
+                     AccArgTypes, AccRet, HadExternal, Warns) ->
   Any = t_any(),
   AnyArgs = [Any || _ <- Args],
   GenSig = {AnyArgs, fun(_) -> t_any() end},
@@ -586,16 +604,32 @@ handle_apply_or_call([{TypeOfApply, {Fun, Sig, Contr, LocalRet}}|Left],
     end,
   NewAccRet = t_sup(AccRet, TotalRet),
   ?debug("NewAccRet: ~s\n", [t_to_string(NewAccRet)]),
+  {NewWarnings, State4} = state__remove_added_warnings(State, State3),
+  {HowMany, OldWarnings} = Warns,
+  NewWarns =
+    case HowMany of
+      none -> {one, NewWarnings};
+      _ ->
+        case OldWarnings =:= [] of
+          true -> {many, []};
+          false ->
+            case NewWarnings =:= [] of
+              true -> {many, []};
+              false -> {many, NewWarnings ++ OldWarnings}
+            end
+        end
+    end,
   handle_apply_or_call(Left, Args, ArgTypes, Map, Tree,
-		       State3, NewAccArgTypes, NewAccRet, HadExternal);
+		       State4, NewAccArgTypes, NewAccRet, HadExternal, NewWarns);
 handle_apply_or_call([], Args, _ArgTypes, Map, _Tree, State,
-		     AccArgTypes, AccRet, HadExternal) ->
+		     AccArgTypes, AccRet, HadExternal, {_, Warnings}) ->
+  State1 = state__add_warnings(Warnings, State),
   case HadExternal of
     false ->
       NewMap = enter_type_lists(Args, AccArgTypes, Map),
-      {State, NewMap, AccRet};
+      {State1, NewMap, AccRet};
     true ->
-      {had_external, State}
+      {had_external, State1}
   end.
 
 apply_fail_reason(FailedSig, FailedBif, FailedContract) ->
@@ -3038,6 +3072,14 @@ state__add_warning(#state{warnings = Warnings, warning_mode = true} = State,
       end
   end.
 
+state__remove_added_warnings(OldState, NewState) ->
+  #state{warnings = OldWarnings} = OldState,
+  #state{warnings = NewWarnings} = NewState,
+  {NewWarnings -- OldWarnings, NewState#state{warnings = OldWarnings}}.
+
+state__add_warnings(Warns, #state{warnings = Warnings} = State) ->
+  State#state{warnings = Warns ++ Warnings}.
+
 -spec state__set_curr_fun(curr_fun(), state()) -> state().
 
 state__set_curr_fun(undefined, State) ->
diff --git a/lib/dialyzer/test/small_SUITE_data/results/higher_order_discrepancy b/lib/dialyzer/test/small_SUITE_data/results/higher_order_discrepancy
index 7ce440a60d..11b9ecade6 100644
--- a/lib/dialyzer/test/small_SUITE_data/results/higher_order_discrepancy
+++ b/lib/dialyzer/test/small_SUITE_data/results/higher_order_discrepancy
@@ -1,4 +1,3 @@
 
-higher_order_discrepancy.erl:11: The call higher_order_discrepancy:g('foo') will never return since it differs in the 1st argument from the success typing arguments: ('bar')
-higher_order_discrepancy.erl:14: Function g/1 has no local return
-higher_order_discrepancy.erl:14: The pattern 'bar' can never match the type 'foo'
+higher_order_discrepancy.erl:19: Function g/1 has no local return
+higher_order_discrepancy.erl:19: The pattern 'bar' can never match the type 'foo'
diff --git a/lib/dialyzer/test/small_SUITE_data/src/higher_order_discrepancy.erl b/lib/dialyzer/test/small_SUITE_data/src/higher_order_discrepancy.erl
index ff5ee6bac4..f9547d4929 100644
--- a/lib/dialyzer/test/small_SUITE_data/src/higher_order_discrepancy.erl
+++ b/lib/dialyzer/test/small_SUITE_data/src/higher_order_discrepancy.erl
@@ -1,3 +1,8 @@
+%% With the patch introduced to avoid false warnings in
+%% user_SUITE_data/src/wpc_hlines.erl we can unfortunately no longer precisely
+%% catch problems like this one... The refinement procedure is still enough to
+%% keep some of the details, nevertheless.
+
 -module(higher_order_discrepancy).
 
 -export([test/1]).
diff --git a/lib/dialyzer/test/user_SUITE_data/results/wpc_hlines b/lib/dialyzer/test/user_SUITE_data/results/wpc_hlines
new file mode 100644
index 0000000000..d6e3f29ab9
--- /dev/null
+++ b/lib/dialyzer/test/user_SUITE_data/results/wpc_hlines
@@ -0,0 +1,3 @@
+
+wpc_hlines.erl:22: Function bad/1 has no local return
+wpc_hlines.erl:22: The pattern 'false' can never match the type 'true'
diff --git a/lib/dialyzer/test/user_SUITE_data/src/wpc_hlines.erl b/lib/dialyzer/test/user_SUITE_data/src/wpc_hlines.erl
new file mode 100644
index 0000000000..8c205a8247
--- /dev/null
+++ b/lib/dialyzer/test/user_SUITE_data/src/wpc_hlines.erl
@@ -0,0 +1,22 @@
+%% Bug reported by Dan Gudmundsson, test shrunk down by Magnus Lång.
+
+%% The problem is that dialyzer_dep generates edges from the fun 
+%% application to both of the functions, and then during the warning pass 
+%% dialyzer_dataflow:handle_apply_or_call generates warnings for any such 
+%% edge that won't return.
+
+%% Since dialyzer_dep is currently supposed to overapproximate rather than
+%% underapproximate, the fix was to modify handle_apply_or_call to not generate
+%% warnings if some of the possible funs can succeed.
+
+-module(wpc_hlines).
+
+-export([do_export/0]).
+
+do_export() ->
+   {Proj, _} =  % The culprit seems to be putting the funs in a tuple
+     {fun good/1, fun bad/1},
+   Proj(true).
+
+good(_) -> ok.
+bad(false) -> ok.