[m-rev.] for review: disable_warnings scope
Paul Bone
paul at bone.id.au
Mon Jan 9 17:08:39 AEDT 2017
On Mon, Jan 09, 2017 at 12:46:32AM +1100, Zoltan Somogyi wrote:
> This is the scope several of us discussed on friday.
>
> For review by Paul. I would particularly like to know why warnings
> about recursive calls that are not TAIL recursive are generated in
> two separate places, and whether there is any particular reason why
> we can't delete the version in ml_tailcalls.m, and make mark_tail_calls.m
> do the job in all grades. I also added some XXXs that I would like his
> opinion on.
The LLDS and MLDS backends implement tailcalls differently. Once TCO has
taken place, or at least tail calls are identified (LLDS), _then_ warnings
are generated. The warnings that need to be generated depends on which
things became tail calls which is different depending on the backend (eg:
MLDS doesn't support mutual recursion (I'm working on it)). Also because
these bakends perform TCO at different stages, the warnings are generated at
different stages.
If I remember completely.
LLDS:
Tail calls are identified in an HLDS stage,
Warnings are generated in (the same?) HLDS stage.
TCO is performed during HLDS->LLDS conversion.
MLDS:
HLDS->MLDS conversion occurs.
Tail calls are identified _and_ optimised in one MLDS stage.
Warnings are generated in (the same?) MLDS stage.
If we wanted to unify these parts of the compiler we'd probably need to
re-implement most of the MLDS tail call code. If we decide want to do
that, it should probably be done at the same time as my MLDS mutual
recursion work.
Another option is to find whatever common code (should) exist between them
and make that shared but otherwise keep them separate. Then we will need to
pass the ignore warnings scope information through to the MLDS backend.
Depending on how most of the MLDS backend is written that could also be a
fair amount of work. We may also find that we need to do this anyway to
suppress other warnings generated within the MLDS backend.
This second option is tempting because it keeps these separate things
separate. Different backends have very different needs, even though both
need to do TCO. This is my "gut feeling" preference, but I'm beginning to
think that if we can handle both backends in mark_tail_calls.m (and generate
warnings there) then actually do TCO for each backend separately that may be
best. Provided that we make the logic in mark_tail_calls very clear with
respect to which backends support which types of TCO. This is currently my
favorite option.
Apart from this issue, which we can resolve later, I only had minor comments
about this change. Thanks.
> I would welcome reviews of the new construct's syntax, and its
> documentation, by others as well.
>
> Zoltan.
> diff --git a/NEWS b/NEWS
> index 9f48d8e..a5b3e9f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -114,6 +114,16 @@ Changes to the Mercury language:
> the compiler now generates an error if the goal inside the scope
> is not a switch on the named variable.
>
> +* We have added a new kind of scope to the language for disabling warnings
> + within the scope. A goal such as
> +
> + disable_warnings [singleton_vars] (
> + Goal
> + )
> +
> + is equivalent to Goal, with the exception that the compiler will not generate
> + warnings about singleton variables inside Goal.
> +
> * We have added an extension to include external files
> in pragma foreign_decl and pragma foreign_code declarations.
Regarding syntax/naming I would consider calling it "suppress_warnings".
I don't think it's either better or worse, just something to consider.
> diff --git a/compiler/constraint.m b/compiler/constraint.m
> index 15b8020..ec89dea 100644
> --- a/compiler/constraint.m
> +++ b/compiler/constraint.m
> @@ -165,6 +165,17 @@ propagate_conj_sub_goal_2(Constraints, Goal0, FinalGoals, !Info) :-
> ;
> GoalExpr = scope(Reason, SubGoal0),
> (
> + Reason = disable_warnings(_, _),
> + % NOTE: This assumes that compiler passes that generate the
> + % warnings that could be disabled by this scope have all been run
> + % BEFORE program transformations such as constraint propagation.
> + % If they haven't been, then the transformations can hide warnings
> + % about code by moving them into these scopes, or can caused them
> + % to be generated when the user does not want them by moving
> + % the warned-about code out of such scopes.
> + propagate_goal(Constraints, SubGoal0, SubGoal, !Info),
> + FinalGoals = [hlds_goal(scope(Reason, SubGoal), GoalInfo)]
> + ;
> ( Reason = exist_quant(_)
> ; Reason = from_ground_term(_, from_ground_term_deconstruct)
> ; Reason = from_ground_term(_, from_ground_term_other)
That will probably be a problem. Both backends process their tailcalls
after this stage.
> diff --git a/compiler/mark_tail_calls.m b/compiler/mark_tail_calls.m
> index 9c2287f..0732121 100644
> --- a/compiler/mark_tail_calls.m
> +++ b/compiler/mark_tail_calls.m
> @@ -608,12 +620,12 @@ at_tail_branch(A, B) = R :-
>
> %-----------------------------------------------------------------------------%
>
> -:- pred maybe_report_nontailcall(at_tail::in, mark_tail_calls_info::in,
> - sym_name::in, arity::in, proc_id::in, prog_context::in,
> - list(error_spec)::out) is det.
> +:- pred maybe_report_nontail_recursive_call(at_tail::in,
> + mark_tail_calls_info::in, sym_name::in, arity::in, proc_id::in,
> + prog_context::in, list(error_spec)::in, list(error_spec)::out) is det.
>
> -maybe_report_nontailcall(AtTail, Info, SymName, Arity, ProcId,
> - Context, Specs) :-
> +maybe_report_nontail_recursive_call(AtTail, Info, SymName, Arity, ProcId,
> + Context, !Specs) :-
> (
> ( AtTail = at_tail(_)
> ; AtTail = not_at_tail_have_not_seen_reccall
> @@ -621,15 +633,18 @@ maybe_report_nontailcall(AtTail, Info, SymName, Arity, ProcId,
> PredInfo = Info ^ mtc_pred_info,
> WarnTailCalls = Info ^ mtc_warn_tail_calls,
> MaybeRequireTailRecursion = Info ^ mtc_maybe_require_tailrec,
> + % XXX I (zs) don't understand the reason why the code below
> + % looks like it does. It should be enough to have a SINGLE field in
> + % Info that says whether we should call report_nontail_recursive_call,
> + % and if so, with what severity.
At the time I didn't consider that it looked bad or anything. Now that I
look at it I can see that it is needed to support the tail recursion warning
proposal we created about this time last year. We decided that tail
recursion warning pragmas should be able to say whether mutual recursion was
permitted or self-recursion only. Therefore whether a warning is given will
depend upon the call site. So MaybeRequireTailRecursion needs to tell us
which callees we should create a warning for. Here's the original code
where you can see it.
MaybeRequireTailRecursion = Info ^ mtc_maybe_require_tailrec,
(
MaybeRequireTailRecursion = no,
(
WarnTailCalls = warn_tail_calls,
report_nontailcall(PredInfo, SymName, Arity, ProcId,
Context, we_warning, Specs)
;
WarnTailCalls = do_not_warn_tail_calls,
Specs = []
)
;
MaybeRequireTailRecursion = yes(RequireTailrecInfo),
(
RequireTailrecInfo = enable_tailrec_warnings(WarnOrError,
HERE---> _Type, _),
% TODO: Check recursion type to implement support for
% mutual vs self recursion checking.
report_nontailcall(PredInfo, SymName, Arity, ProcId,
Context, WarnOrError, Specs)
;
RequireTailrecInfo = suppress_tailrec_warnings(_),
Specs = []
)
)
That being said this switch could be simplified if we introduce a new type. I
don't mind.
> diff --git a/compiler/parse_goal.m b/compiler/parse_goal.m
> index dda6c51..c15bd3e 100644
> --- a/compiler/parse_goal.m
> +++ b/compiler/parse_goal.m
> @@ -246,6 +248,48 @@ parse_non_call_goal(Functor, Args, Context, ContextPieces, MaybeGoal,
> MaybeGoal = error1([Spec])
> )
> ;
> + ( Functor = "disable_warning"
> + ; Functor = "disable_warnings"
> + ),
> + ( if Args = [WarningsTerm, SubGoalTerm] then
> + varset.coerce(!.VarSet, GenericVarSet),
> + parse_warnings(GenericVarSet, WarningsTerm, Functor,
> + ContextPieces, 1, MaybeWarningsList),
> + parse_goal(SubGoalTerm, ContextPieces, MaybeSubGoal, !VarSet),
> + ( if
> + MaybeWarningsList = ok1(WarningsList0),
> + MaybeSubGoal = ok1(SubGoal)
> + then
> + % XXX Should we generate a warning if the name of a warning
> + % to disable is listed more than once?
It might be indicative of some other error if it shows that the developer
meant to type one thing but typed another. Other than that and poor style
I can't imagine how this represents a problem in the program.
> + list.sort_and_remove_dups(WarningsList0, WarningsList),
> + (
> + WarningsList = [HeadWarning | TailWarnings],
> + Goal = disable_warnings_expr(Context,
> + HeadWarning, TailWarnings, SubGoal),
> + MaybeGoal = ok1(Goal)
> + ;
> + WarningsList = [],
> + Pieces = cord.list(ContextPieces) ++
> + [lower_case_next_if_not_first, words("Error:"),
> + words("a"), fixed(Functor), words("scope"),
> + words("should list at least one warning."), nl],
s/should/must
> + Spec = error_spec(severity_error, phase_term_to_parse_tree,
> + [simple_msg(get_term_context(WarningsTerm),
> + [always(Pieces)])]),
> + MaybeGoal = error1([Spec])
> + )
> + else
> + Specs = get_any_errors1(MaybeWarningsList) ++
> + get_any_errors1(MaybeSubGoal),
> + MaybeGoal = error1(Specs)
> + )
> + else
> + Spec = should_have_one_x_one_goal_prefix(ContextPieces, Context,
> + "a list of warnings to disable", Functor),
> + MaybeGoal = error1([Spec])
> + )
> + ;
> ( Functor = "not" % Negation (NU-Prolog syntax).
> ; Functor = "\\+" % Negation (Prolog syntax).
> ),
> diff --git a/doc/reference_manual.texi b/doc/reference_manual.texi
> index 3ae254b..024be43 100644
> --- a/doc/reference_manual.texi
> +++ b/doc/reference_manual.texi
> @@ -526,6 +526,8 @@ when xfx 900
> all fxy 950
> arbitrary fxy 950
> atomic fxy 950
> +disable_warning fxy 950
> +disable_warnings fxy 950
> promise_equivalent_solutions fxy 950
> promise_equivalent_solution_sets fxy 950
> promise_exclusive fy 950
> @@ -995,6 +997,40 @@ in a @code{require_switch_arms_semidet} scope,
> even though it would not be ok
> to have a det goal in a @code{require_semidet} scope.
>
> + at item @code{disable_warnings [@var{Warning}] @var{Goal}}
> + at itemx @code{disable_warning [@var{Warning}] @var{Goal}}
> +The Mercury compiler can generate warnings
> +about several kinds of constructs that whose legal Mercury semantics
> +is likely to differ from the semantics intended by the programmer.
> +While such warnings are useful most of the time,
> +they are a distraction in cases where the programmer's intention
> + at emph{does} match the legal semantics.
> +Programmers can disable all warnings of a particular kind for an entire module
> +by compiling that module with the appropriate compiler option,
> +but in many cases this is not a good idea,
> +since some of the warnings it disables may @emph{not} have been mistaken.
> +This is what these goals are for.
> +The goal @code{disable_warnings [@var{Warning}] @var{Goal}}
> +is equivalent to @code{@var{Goal}} in all respects, with one exception:
> +the Mercury compiler will not generate warnings of any of the categories
> +whose names appear in @code{[@var{Warning}]}.
> +
> +At the moment, the Mercury compiler supports the disabling of
> +the following warning category:
> + at table @asis
> + at item @code{singleton_vars}
> +Disable the generation of singleton variable warnings.
> + at c @item @code{non_tail_recursive_calls}
> + at c Disable the generation of warnings about recursive calls
> + at c that are not @emph{tail}-recursive calls.
> + at end table
> +
> +The keyword starting this scope may be written
> +either as @code{disable_warnings} or as @code{disable_warning}.
> +This is intended to make the code read more naturally
> +regardless of whether the list contains the name of
> +more than one disabled warning category.
> +
I don't think this is necessary but don't feel strongly about it. I would
be happy to have the plural one even if I disabled only a single warning.
Thanks.
--
Paul Bone
http://paul.bone.id.au
More information about the reviews
mailing list