[m-rev.] diff: double the typecheck ambiguity warning limit

[Zoltan Somogyi]

On Fri, 7 Nov 2014 22:31:13 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
> The point I was trying to get across is
> that IMO, the existing warning limit is too low.

And the points I was trying to get across are that (a) IF that
is so, then it will STILL be too low after doubling, and (b) that
due to the exponential time complexity of the current
typechecking algorithm, the two notions "I expect the compiler
to handle this level of ambiguity with no problem" and "the
compiler's actual behavior with this level of ambiguity is to
thrash the machine to death by allocating far more memory
than the machine has" actually overlap, so there is NO level
of ambiguity you can pick that is guaranteed to (a) not
annoy users by giving warnings, and (b) not annoy users
by thrashing their machine. Short of moving to the constraint
based type checker, you HAVE to pick an annoyance to
inflict on users. I happen to think annoyance (a) is the
lesser of the two evils we have to choose among.
 
> Here is the predicate that motivated this change:
> 
> :- pred do_query_dt_entries_3(location_table::in,
>      matrix::in, matrix::in,
>      string::in, string::in, location_index::in, location_index::in,
>      bool::in, bool::out, io::di, io::uo) is det.
> 
> do_query_dt_entries_3(LocnTable, DistanceMatrix, TimeMatrix,
>          QueryLocnA, QueryLocnB, QueryIndexA, QueryIndexB,
>          !IsFirst, !IO) :-
>      KeyAB = {QueryIndexA, QueryIndexB},
>      KeyBA = {QueryIndexB, QueryIndexA},
>      TimeAB = lookup(TimeMatrix, KeyAB),

My first reaction was that the last line should be

map.lookup(TimeMatrix, KeyAB, TimeAB)

My second was that all three lines should be replaced with

map.lookup(TimeMatrix, QueryIndexA, TimeSubMatrixA),
map.lookup(TimeSubMatrixA, QueryIndexB, TimeAB)

since this two-stage map should make lookups faster.
With the existing design, the comparisons done by map.lookup
will first differ on the first part of the tuple, and then agree
(corresponding to the two map.lookups in the second design).
For the first set of comparisons, the second design avoids
going through the tuple wrapper; for the second set of
comparisons, it avoid the redundant checking of
QueryIndexA.

A second advantage would be the total size of the maps
should be smaller, since each 234 tree node avoids storing
both the tuple wrapper and whichever index is not being
compared. There will be some more nodes, but not enough
to counteract this effect.

> Obviously, I am well aware that there are bunch of things I can do in
> order to avoid the above ambiguities and hence the warnings.
> At what point should I have to start doing those things though?
> (My answer is not in the case of something like the above predicate.)

Why not?

As a reader of the above code, *I* would want to know whether the
lookups are in maps or multi_maps, and I shouldn't have to duplicate
the compiler's work to find out. (BTW, I would also module qualify
the calls to format, to avoid the io.format/string.format ambiguity
for humans.)

> As to how much the limit should be increased by, the original limit of
> 50 appears to have been fairly arbitrary, 100 was equally arbitrary on
> my part.  I would note that --typecheck-ambiguity-error-limit is set to
> 3000, so perhaps the warning limit should be increased by quite a bit
> more, say 1000?

1000 is less than two doublings away from 3000. If 3000 leads to
unacceptable compiler performance (which probably has also changed
in the intervening time), then that means users get the warning
too late.

The warning limit should be set based on when the compiler's
performance in type checking starts to be annoying, since the point
of the warning is to tell users what they can do to AVOID the annoyance.
That requires experiment. Do you want to do some?

Zoltan.