[MLton] Implementing warnExnMatch

[Matthew Fluet]

> 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:
 
As expected.

> 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'.

No need for a separate comand line option.  We've ditched the original 
-warn-match option in favor of the -default-ann usage.

> 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.

Yes.  That is a minor point of confusion.  It also leads to a slightly bad 
behavior in that an un-instantiated functor with redundant/inexhaustive 
matches will not give warning messaegs (and a multiply instantiated 
functor will give multiple warning messages).  At some point in time, it 
would be wise to migrate MatchCompile into the Elaborate pass, but that is 
not necessary for implementing warnExnMatch.

But, that notwithstanding, the correct place to set the warnMatch will be
in the Elaborate pass.

> 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.

You don't want to use unification, because that will raise an error when 
the types do not unify or it will force an otherwise polymorphic type to 
be equal to an exception type.  It will be appropriate to use the 
underlying tycon equality.

> @@ -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?

It is correct, but as you say, it would probably best be factored into an 
auxilary function:

<--- cvs diff -u --->
Index: mlton/ast/prim-tycons.fun
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/ast/prim-tycons.fun,v
retrieving revision 1.29
diff -u -r1.29 prim-tycons.fun
--- mlton/ast/prim-tycons.fun	19 Jun 2005 21:33:41 -0000	1.29
+++ mlton/ast/prim-tycons.fun	25 Jul 2005 17:28:04 -0000
@@ -31,6 +31,8 @@
 datatype z = datatype Kind.t
 datatype z = datatype AdmitsEquality.t
 
+val isExn = fn c => equals (c, exn)
+
 local
    fun 'a make (prefix: string,
 		all: 'a list,
Index: mlton/ast/prim-tycons.sig
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/ast/prim-tycons.sig,v
retrieving revision 1.16
diff -u -r1.16 prim-tycons.sig
--- mlton/ast/prim-tycons.sig	14 Jan 2005 01:23:36 -0000	1.16
+++ mlton/ast/prim-tycons.sig	25 Jul 2005 17:28:04 -0000
@@ -46,6 +46,7 @@
       val ints: (tycon * IntSize.t) vector
       val intInf: tycon
       val isCharX: tycon -> bool
+      val isExn: tycon -> bool
       val isIntX: tycon -> bool
       val isRealX: tycon -> bool
       val isWordX: tycon -> bool
Index: mlton/elaborate/type-env.fun
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/elaborate/type-env.fun,v
retrieving revision 1.56
diff -u -r1.56 type-env.fun
--- mlton/elaborate/type-env.fun	19 Jun 2005 21:33:58 -0000	1.56
+++ mlton/elaborate/type-env.fun	25 Jul 2005 17:28:04 -0000
@@ -774,6 +774,11 @@
 	  | Overload Overload.Char => true
  	  | _ => false
 
+      fun isExn t =
+	 case toType t of
+	    Con (c, _) => Tycon.isExn c
+	  | _ => false
+
       fun isInt t =
 	 case toType t of
 	    Con (c, _) => Tycon.isIntX c
Index: mlton/elaborate/type-env.sig
===================================================================
RCS file: /cvsroot/mlton/mlton/mlton/elaborate/type-env.sig,v
retrieving revision 1.29
diff -u -r1.29 type-env.sig
--- mlton/elaborate/type-env.sig	20 May 2005 16:34:27 -0000	1.29
+++ mlton/elaborate/type-env.sig	25 Jul 2005 17:28:04 -0000
@@ -38,6 +38,7 @@
 			  replaceSynonyms: bool,
 			  var: Tyvar.t -> 'a} -> 'a
 	    val isCharX: t -> bool
+	    val isExn: t -> bool
 	    val isInt: t -> bool
 	    val isUnit: t -> bool
 	    val layout: t -> Layout.t
<--- cvs diff -u --->

BTW, it is useful to note that Type.isExn will return false if the type is 
a type variable being used for inference.  However, this is o.k.  Since we 
will always be checking the type of the scruitinee in a pattern match, 
either the constructors in the pattern will be manifestly exception 
constructors, or they will not.  Hence, the type of the scruitinee will 
always be an exception in the cases where we want to turn the match 
warning off. 

> 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.

Fair enough, you might push it back as to PrimTycon and TypeEnv.Type as in 
the patch above.

> 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?

I prefer the former.  I think this boils down logicaly to:

  warnMatch () 
  andalso (not (Type.isExn argType) 
           orelse warnExnMatch ())


> 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?

I believe so.

> 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.

I think they should apply to all of them.  I'm somewhat ambivalent about 
2b, but uniformity is probably better than one exceptional (no pun 
intended) case.