[MLton] Implementing warnExnMatch

[Vesa Karvonen]

I've made a bit of progress on implementing `warnExnMatch' and I have a few
questions. The code in this message is preliminary and incomplete.

It was easy to add the `warnExnMatch' annotation:

<--- cvs diff -u --->
Index: mlton/control/control-flags.sig
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/control/control-flags.sig,v
retrieving revision 1.4
diff -u -r1.4 control-flags.sig
--- mlton/control/control-flags.sig     23 Jul 2005 11:55:39 -0000      1.4
+++ mlton/control/control-flags.sig     25 Jul 2005 15:22:13 -0000
@@ -72,6 +72,7 @@
            (* in (e1; e2), require e1: unit. *)
            val sequenceUnit: (bool,bool) t
            val warnMatch: (bool,bool) t
+           val warnExnMatch: (bool,bool) t
            val warnUnused: (bool,bool) t

            val current: ('args, 'st) t -> 'st
Index: mlton/control/control-flags.sml
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/control/control-flags.sml,v
retrieving revision 1.5
diff -u -r1.5 control-flags.sml
--- mlton/control/control-flags.sml     23 Jul 2005 11:55:39 -0000      1.5
+++ mlton/control/control-flags.sml     25 Jul 2005 15:22:14 -0000
@@ -334,6 +334,8 @@
            makeBool ({name = "sequenceUnit", default = false, expert = false}, ac)
         val (warnMatch, ac) =
            makeBool ({name = "warnMatch", default = true, expert = false}, ac)
+        val (warnExnMatch, ac) =
+           makeBool ({name = "warnExnMatch", default = true, expert = false}, ac)
         val (warnUnused, ac) =
            makeBool ({name = "warnUnused", default = false, expert = false}, ac)
         val {parseId, parseIdAndArgs, withDef, snapshot} = ac
<--- cvs diff -u --->

Should we also provide a separate command line option (`-warn-exn-match')?
I'm quite happy with just the annotation, which can be specified on the
command line through `-default-ann'.

Figuring out where to put the code that actually disables the warning and
how to test whether a type is the same type as `exn' took me much longer.

One source of confusion was that the `warnMatch' warning is actually
generated by the Defunctorize pass, but the Elaborate pass determines
whether a warning should be generated for a specific declaration or
expression.

After finding a proper spot to silence the warning, I first tried to find
an `equals' method on types. Then I briefly though about using unification,
but it seemed wrong. Finally, I saw that Tycons can be compared for equality.

I then modifed elaborate as follows (to see if it works):

<--- cvs diff -u --->
Index: mlton/elaborate/elaborate-core.fun
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/elaborate/elaborate-core.fun,v
retrieving revision 1.153
diff -u -r1.153 elaborate-core.fun
--- mlton/elaborate/elaborate-core.fun  23 Jul 2005 11:55:40 -0000      1.153
+++ mlton/elaborate/elaborate-core.fun  25 Jul 2005 15:22:14 -0000
@@ -17,6 +17,7 @@
    val allowRebindEquals = fn () => current allowRebindEquals
    val sequenceUnit = fn () => current sequenceUnit
    val warnMatch = fn () => current warnMatch
+   val warnExnMatch = fn () => current warnExnMatch
 end

 local
@@ -2911,7 +2912,12 @@
                           region = region,
                           rules = rules,
                           test = Cexp.var (arg, argType),
-                          warnMatch = warnMatch ()}
+                          warnMatch = if (case Type.deConOpt argType of
+                                            NONE => false
+                                          | SOME (c,_) => Tycon.equals (c, Tycon.exn))
+                                         then warnMatch () andalso warnExnMatch ()
+                                      else
+                                         warnMatch ()}
         in
           {arg = arg,
            argType = argType,
<--- cvs diff -u --->

Is the above the correct way test for the `exn' type?

Of course, the test is probably best factored into an auxiliary function.
Actually, there seems to be some room for abstraction / refactoring. If
you look for `val isBool' in `elaborate-core.fun', you'll find out that it
is defined three times identically.

One question I have in mind is the proper behavior of `warnExnMatch' in
conjunction with `warnMatch'. One reasonable behavior would be that
`warnExnMatch' can only be used to turn warnings off, but never to turn
them on:

         warnMatch   t t f f
      warnExnMatch   t f t f
    --------------------------
       warn on exn   t f f f
                         *

Another alternative would be to allow warnExnMatch to also turn warnings
on:

         warnMatch   t t f f
      warnExnMatch   t f t f
    --------------------------
       warn on exn   t f t f
                         *

Which should be preferred?

Another question is which match contexts should be subject to
`warnExnMatch'? I see 5 places in `elaborate-core.fun' that refer to
`warnMatch ()' and seem to correspond to (in order from top to bottom):

  1.  fun declaration (Adec.Fun / fun ...)
  2a. val rec declaration (Adec.Val / val rec ... = fn ...)
  2b. val declaration (Adec.Val / val ...)
  3.  case expression (Aexp.Case / case ...)
  4.  fn expression (elabMatchFn / Aexp.Fn / fn ...)
     [elabMatchFn is also called on Aexp.Handle]

Is the above list correct?

Should all of the contexts be subject to `warnExnMatch' or would it be
desirable to limit `warnExnMatch' to only certain kinds of matches? My
code will probably only use case 3, but I think that it would be more
logical to apply `warnExnMatch' to all except perhaps case 2b.

-Vesa Karvonen