[m-rev.] for review: make illegal mutable attributes unrepresentable

[Julien Fischer]

On Tue, 25 May 2021, Zoltan Somogyi wrote:

> For review by anyone.

> Make illegal mutable attributes unrepresentable.
> 
> compiler/prog_item.m:
>     Change the representation of mutable_var_attributes,
>     in two different ways.
>
>     The first is replacing a list of elements that each say
>     "in this language, use this name", which allowed duplicate entries
>     for a language, with a map from languages to names, which does not.
>
>     The second is replacing of a set of four boolean-equivalent flags,
>     many of whose 16 possible combinations are invalid, with
>     a structured type that allows the representation of only
>     the valid combinations.
>
>     To allow code working on attributes to exploit this defense against
>     illegal attribute sets, export the new representation. (Fully taking
>     advantage of this will require moving the code that creates the
>     declarations and definitions of each aux predicate next to each other.)
> 
> compiler/parse_mutable.m:
>     Previously, the code that parsed mutable declarations checked for
>     *some* invalid combinations of attributes, but not *all*. Since
>     invalid combinations are no longer representable, the new parsing code
>     does check for a reject all invalid combinations.
>
>     Diagnose repeated attributes, as well as conflicting attributes.
>
>     Improve error messages, even when not related to this change,
>     e.g. by quoting echoed user code.
> 
> compiler/add_mutable_aux_preds.m:
>     Delete the code that used to check for illegal combinations of mutable
>     values, since these cannot happen anymore.
>
>     Conform to the change above.
>
>     Delete an unneeded field from the parameters. The old ImplLang and Lang
>     fields contained exactly the same information, so keep only Lang.
> 
> compiler/prog_mutable.m:
>     Conform to the change above.
> 
> tests/invalid/bad_mutable.m:
>     Add some more bad mutable declarations to diagnose.
>
>     Add an XXX pointing out an error for which we generate *two* error
>     messages (one in add_mutable_aux_preds.m, and one during module
>     qualification).
> 
> tests/invalid/bad_mutable.err_exp:
>     Expect the updated error messages for both the old and the new
>     bad mutable declarations.
> 
> tests/invalid/bad_finalise.err_exp:
>     Expect quoting of echoed user code.


> diff --git a/compiler/parse_mutable.m b/compiler/parse_mutable.m
> index 948989c40..650c01e9f 100644
> --- a/compiler/parse_mutable.m
> +++ b/compiler/parse_mutable.m

...


> @@ -360,38 +352,221 @@ parse_mutable_attrs(VarSet, MutAttrsTerm, MaybeMutAttrs) :-

...

> +:- pred check_attribute_fit(varset::in,
> +    map(foreign_language, string)::in,
> +    maybe(pair(term, mutable_trailed))::in,
> +    maybe(pair(term, unit))::in,
> +    maybe(pair(term, unit))::in,
> +    maybe(pair(term, unit))::in,
> +    maybe1(mutable_var_attributes)::out) is det.
> +
> +check_attribute_fit(VarSet, OnlyLangMap, MaybeTrailed, MaybeConst, MaybeIO,
> +        MaybeLocal, MaybeMutAttrs) :-
> +    some [!Specs] (
> +        !:Specs = [],
> +        (
> +            MaybeConst = no,
> +            (
> +                MaybeIO = no,
> +                IO = mutable_dont_attach_to_io_state
> +            ;
> +                MaybeIO = yes(_ - unit),
> +                IO = mutable_attach_to_io_state
> +            ),
> +            (
> +                MaybeLocal = yes(LocalTerm - unit),
> +                Local = mutable_is_thread_local, % implicitly mutable_untrailed
> +                (
> +                    MaybeTrailed = yes(_TrailTerm - mutable_untrailed)
> +                ;
> +                    MaybeTrailed = yes(TrailTerm - mutable_trailed),
> +                    report_conflicting_attributes(VarSet, LocalTerm, TrailTerm,
> +                        !Specs)
> +                    % Local is wrong, but will not be used due to !:Specs.
> +                ;
> +                    MaybeTrailed = no
> +                    % This code lets a "thread_local" attribute
> +                    % implicitly set the "untrailed" attribute.
> +                    % XXX Should we require "untrailed" to be set explicitly?

Yes, I think so.

The diff is otherwise fine.

Julien.