[m-rev.] for review: warn about unsigned comparisons against zero that are tautologies

[Julien Fischer]

Hi Zoltan,

On Sat, 20 Oct 2018, Zoltan Somogyi wrote:

>
>
> On Sat, 20 Oct 2018 05:53:00 +0000 (UTC), Julien Fischer <jfischer at opturion.com> wrote:
>> +maybe_generate_warning_for_useless_comparison(
>> +        PredInfo, InstMap, Args, GoalInfo, !Info) :-
>> +    pred_info_is_pred_or_func(PredInfo) = PredOrFunc,
>> +    ModuleSymName = pred_info_module(PredInfo),
>> +    ( if
>> +        simplify_do_warn_simple_code(!.Info),
>> +        is_std_lib_module_name(ModuleSymName, ModuleName),
>> +        PredOrFunc = pf_predicate,
>> +        Args = [ArgA, ArgB]
>> +    then
>
> I have two minor issues with this diff. The first is how you test whether
> warn_simple_code is enabled. *Either* it should be tested in the
> condition of this if-then-else, *or* it should be tested in the conditional
> error message components and severity, but not both, since one is enough.
> And if it is tested in this condition, it should be the *last* test: if you are
> gonna fail, you wanna fail fast.

Done.

>> +is_useless_unsigned_comparison(ModuleName, PredName, ArgA, ArgB,
>> +        Pieces) :-
>> +    (
>> +        PredName = ">=",
>> +        arg_is_unsigned_zero(ModuleName, ArgB, ZeroStr),
>> +        Pieces = [words("cannot fail."), nl,
>> +            words("Comparison of"), words(ModuleName),
>> +            words(">="), words(ZeroStr), words("is always true.")]
>
> The other issue is the wording of these messages.
> I think something along the lines of "All uint8 values are >= 0u8"
> would be clearer.

I think the second part of the error message should reflect the specific
operator that was used and whether zero was the first or second operand.
I've reworded things as per the relative diff below.

> Other than that, the diff is fine.
>
> Note that I think Peter is also working on this module, so one of you
> will probably get a conflict :-(

Since I've primarily added code I hope not!  In any case, Peter can you
let me know if this will cause problems for you.  If so, I will hold
of pushing it.

Julien.

diff --git a/compiler/simplify_goal_call.m b/compiler/simplify_goal_call.m
index adf1e9d..5dcc7bd 100644
--- a/compiler/simplify_goal_call.m
+++ b/compiler/simplify_goal_call.m
@@ -614,10 +614,10 @@ maybe_generate_warning_for_useless_comparison(
      pred_info_is_pred_or_func(PredInfo) = PredOrFunc,
      ModuleSymName = pred_info_module(PredInfo),
      ( if
-        simplify_do_warn_simple_code(!.Info),
          is_std_lib_module_name(ModuleSymName, ModuleName),
          PredOrFunc = pf_predicate,
-        Args = [ArgA, ArgB]
+        Args = [ArgA, ArgB],
+        simplify_do_warn_simple_code(!.Info)
      then
          pred_info_get_name(PredInfo, PredName),
          instmap_lookup_var(InstMap, ArgA, InstA),
@@ -634,9 +634,7 @@ maybe_generate_warning_for_useless_comparison(
              Msg = simple_msg(GoalContext,
                  [option_is_set(warn_simple_code, yes,
                      [always(Pieces)])]),
-            Severity = severity_conditional(warn_simple_code, yes,
-                severity_warning, no),
-            Spec = error_spec(Severity,
+            Spec = error_spec(severity_warning,
                  phase_simplify(report_in_any_mode), [Msg]),
              simplify_info_add_message(Spec, !Info)
          else
@@ -655,26 +653,26 @@ is_useless_unsigned_comparison(ModuleName, PredName, ArgA, ArgB,
          PredName = ">=",
          arg_is_unsigned_zero(ModuleName, ArgB, ZeroStr),
          Pieces = [words("cannot fail."), nl,
-            words("Comparison of"), words(ModuleName),
-            words(">="), words(ZeroStr), words("is always true.")]
+            words("All"), words(ModuleName), words("values are"),
+            words(">="), words(ZeroStr), suffix(".")]
      ;
          PredName = "=<",
          arg_is_unsigned_zero(ModuleName, ArgA, ZeroStr),
          Pieces = [words("cannot fail."), nl,
-            words("Comparison of"), words(ZeroStr), words("=<"),
-            words(ModuleName), words("is always true.")]
+            words(ZeroStr), words("=<"), words("all"),
+            words(ModuleName), words("values.")]
      ;
          PredName = "<",
          arg_is_unsigned_zero(ModuleName, ArgB, ZeroStr),
          Pieces = [words("cannot succeed."), nl,
-            words("Comparison of"), words(ModuleName),words("<"),
-            words(ZeroStr), words("is always false.")]
+            words("There are no"), words(ModuleName),
+            words("values <"), words(ZeroStr), suffix(".")]
      ;
          PredName = ">",
          arg_is_unsigned_zero(ModuleName, ArgA, ZeroStr),
          Pieces = [words("cannot succeed."), nl,
-            words("Comparison of"), words(ZeroStr), words(">"),
-            words(ModuleName), words("is always false.")]
+            words(ZeroStr), words("> no"), words(ModuleName),
+            words("values.")]
      ).

  :- pred arg_is_unsigned_zero(string::in, mer_inst::in, string::out) is semidet.
diff --git a/tests/warnings/unsigned_zero_cmp.exp b/tests/warnings/unsigned_zero_cmp.exp
index 3d964e5..0b7e8e5 100644
--- a/tests/warnings/unsigned_zero_cmp.exp
+++ b/tests/warnings/unsigned_zero_cmp.exp
@@ -1,46 +1,46 @@
  unsigned_zero_cmp.m:028: Warning: call to predicate `uint.<'/2 cannot succeed.
-unsigned_zero_cmp.m:028:   Comparison of uint < 0u is always false.
+unsigned_zero_cmp.m:028:   There are no uint values < 0u.
  unsigned_zero_cmp.m:029: Warning: call to predicate `uint.>'/2 cannot succeed.
-unsigned_zero_cmp.m:029:   Comparison of 0u > uint is always false.
+unsigned_zero_cmp.m:029:   0u > no uint values.
  unsigned_zero_cmp.m:030: Warning: call to predicate `uint.=<'/2 cannot fail.
-unsigned_zero_cmp.m:030:   Comparison of 0u =< uint is always true.
+unsigned_zero_cmp.m:030:   0u =< all uint values.
  unsigned_zero_cmp.m:031: Warning: call to predicate `uint.>='/2 cannot fail.
-unsigned_zero_cmp.m:031:   Comparison of uint >= 0u is always true.
+unsigned_zero_cmp.m:031:   All uint values are >= 0u.
  unsigned_zero_cmp.m:034: Warning: call to predicate `uint8.<'/2 cannot succeed.
-unsigned_zero_cmp.m:034:   Comparison of uint8 < 0u8 is always false.
+unsigned_zero_cmp.m:034:   There are no uint8 values < 0u8.
  unsigned_zero_cmp.m:035: Warning: call to predicate `uint8.>'/2 cannot succeed.
-unsigned_zero_cmp.m:035:   Comparison of 0u8 > uint8 is always false.
+unsigned_zero_cmp.m:035:   0u8 > no uint8 values.
  unsigned_zero_cmp.m:036: Warning: call to predicate `uint8.=<'/2 cannot fail.
-unsigned_zero_cmp.m:036:   Comparison of 0u8 =< uint8 is always true.
+unsigned_zero_cmp.m:036:   0u8 =< all uint8 values.
  unsigned_zero_cmp.m:037: Warning: call to predicate `uint8.>='/2 cannot fail.
-unsigned_zero_cmp.m:037:   Comparison of uint8 >= 0u8 is always true.
+unsigned_zero_cmp.m:037:   All uint8 values are >= 0u8.
  unsigned_zero_cmp.m:040: Warning: call to predicate `uint16.<'/2 cannot
  unsigned_zero_cmp.m:040:   succeed.
-unsigned_zero_cmp.m:040:   Comparison of uint16 < 0u16 is always false.
+unsigned_zero_cmp.m:040:   There are no uint16 values < 0u16.
  unsigned_zero_cmp.m:041: Warning: call to predicate `uint16.>'/2 cannot
  unsigned_zero_cmp.m:041:   succeed.
-unsigned_zero_cmp.m:041:   Comparison of 0u16 > uint16 is always false.
+unsigned_zero_cmp.m:041:   0u16 > no uint16 values.
  unsigned_zero_cmp.m:042: Warning: call to predicate `uint16.=<'/2 cannot fail.
-unsigned_zero_cmp.m:042:   Comparison of 0u16 =< uint16 is always true.
+unsigned_zero_cmp.m:042:   0u16 =< all uint16 values.
  unsigned_zero_cmp.m:043: Warning: call to predicate `uint16.>='/2 cannot fail.
-unsigned_zero_cmp.m:043:   Comparison of uint16 >= 0u16 is always true.
+unsigned_zero_cmp.m:043:   All uint16 values are >= 0u16.
  unsigned_zero_cmp.m:046: Warning: call to predicate `uint32.<'/2 cannot
  unsigned_zero_cmp.m:046:   succeed.
-unsigned_zero_cmp.m:046:   Comparison of uint32 < 0u32 is always false.
+unsigned_zero_cmp.m:046:   There are no uint32 values < 0u32.
  unsigned_zero_cmp.m:047: Warning: call to predicate `uint32.>'/2 cannot
  unsigned_zero_cmp.m:047:   succeed.
-unsigned_zero_cmp.m:047:   Comparison of 0u32 > uint32 is always false.
+unsigned_zero_cmp.m:047:   0u32 > no uint32 values.
  unsigned_zero_cmp.m:048: Warning: call to predicate `uint32.=<'/2 cannot fail.
-unsigned_zero_cmp.m:048:   Comparison of 0u32 =< uint32 is always true.
+unsigned_zero_cmp.m:048:   0u32 =< all uint32 values.
  unsigned_zero_cmp.m:049: Warning: call to predicate `uint32.>='/2 cannot fail.
-unsigned_zero_cmp.m:049:   Comparison of uint32 >= 0u32 is always true.
+unsigned_zero_cmp.m:049:   All uint32 values are >= 0u32.
  unsigned_zero_cmp.m:052: Warning: call to predicate `uint64.<'/2 cannot
  unsigned_zero_cmp.m:052:   succeed.
-unsigned_zero_cmp.m:052:   Comparison of uint64 < 0u64 is always false.
+unsigned_zero_cmp.m:052:   There are no uint64 values < 0u64.
  unsigned_zero_cmp.m:053: Warning: call to predicate `uint64.>'/2 cannot
  unsigned_zero_cmp.m:053:   succeed.
-unsigned_zero_cmp.m:053:   Comparison of 0u64 > uint64 is always false.
+unsigned_zero_cmp.m:053:   0u64 > no uint64 values.
  unsigned_zero_cmp.m:054: Warning: call to predicate `uint64.=<'/2 cannot fail.
-unsigned_zero_cmp.m:054:   Comparison of 0u64 =< uint64 is always true.
+unsigned_zero_cmp.m:054:   0u64 =< all uint64 values.
  unsigned_zero_cmp.m:055: Warning: call to predicate `uint64.>='/2 cannot fail.
-unsigned_zero_cmp.m:055:   Comparison of uint64 >= 0u64 is always true.
+unsigned_zero_cmp.m:055:   All uint64 values are >= 0u64.