[m-rev.] for review: do occurs checks on the parse tree

[Zoltan Somogyi]

On Wed, 2 Oct 2019 17:18:49 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> The two cases you detected could have been avoided if we knew that
> worst_purity/2 was a function.  Is gathering together all the visible
> function names and arities really going to be that expensive.

No, it wouldn't be. However, suppressing messages for occurs check
violations when there is a reference to the name of an executable function
between the two occurrences of a variable would, as you say, do the
wrong thing if the name is overloaded and the actual reference turns
out to be to the data constructor.

> (We
> could delay doing so until a situation like the above is detected.)

The code that unravels unifications needs either the set of function sym_names
and arities passed down to it, or it needs to pass back up some info that we can use
to decide whether each warning should be suppressed, based on the contents
of the set, once it is gathered. The logic of the latter solution would be significantly
more complex.

Gathering the function SNAs up front looks much simpler, and should be cheap enough.

> (Basing the decision on whether to emit the warning on whether a
> symbol does not correspond to a visible function is obviously not
> perfect but it should suffice for most cases except where some
> fairly inadvisable overloading has taken place -- and that's likely
> to cause its own problems.)

Such code may be the code most in need of this warning :-(

> OTOH, since it's only two instances out of the entire Mercury system
> I'm inclined to simply say issuing warnings for such ok code is fine.

Will do.
 
> (Actually, the test that got the warning in library/term_to_xml.m looks
> a bit questionable anyway, we iterate over the string once to check it
> is alphabetic, then again to convert into lower case, and then again to
> compare it with the orgiginal; that test could just be
> string.all_match(is_lower, Head)).

Yes, that looked strange to me as well. I also don't know why it computes
Head via split; shouldn't it be done via first_char?

I don't know utf corner cases well enough to fix that code, but I expect you do.

> > Warn about occurs check violations in parse trees.
> > 
> > Until now, we have generated warnings about occurs check violations
> > in the HLDS. Since the HLDS is in superhomogeneous form, this means
> > we warn for X = f(X), but not for Xs = [X1, X2 | Xs]. The reason is
> > that the latter expands to Xs = [X1 | Xs1], Xs1 = [X2 | Xs],
> > and neither of those two unifications fails the occurs check.
> 
> > compiler/superhomogeneous.m:
> >     While expanding terms into superhomogeneous form, keep track
> >     of which variables have been unified with a term *or an ancestor
> >     of that term*. In the case above, we know that Xs1 is *part of* Xs,
> >     so unifying it with [X2 | Xs] is an occurs check violation.
> >
> >     Generate a warning for such violations. This is only a warning,
> >     because in the parse tree, function symbols can be not just
> >     data constructors but also function applications, and we can't
> >     say which is which. While X = f(X) is a real problem if f is
> >     a data constructor, it may be perfectly ok code (a test for a
> >     fixpoint) if f is a function.
> > 
> > compiler/modecheck_unify.m:
> >     Disable the HLDS version of this warning, to avoid warning
> >     about the problem twice. (This diff leaves the infrastructure
> >     supporting the HLDS warning remains in place, in case we want
> >     to enable it again.)
> > 
> > compiler/prog_data.m:
> > library/term_to_xml.m:
> >     Change code to avoid the new warning.
> > 
> > tests/invalid/ambiguous_overloading_error.err_exp:
> > tests/invalid/max_error_line_width.err_exp:
> > tests/invalid/occurs.err_exp:
> > tests/warnings/unify_x_f_x.exp:
> >     Update these expected outputs to expect the new warning,
> >     and (in some cases) not to expect its HLDS version.
> 
> There should be a command line option for disabling this warning.

And a corresponding name for disable_warnings scopes.

What should its name be? --warn-occurs-check? --warn-suspected-occurs-check?
Something else?

Zoltan.