[m-rev.] for review: {err,warn,info}_spec
Julien Fischer
jfischer at opturion.com
Fri Jul 17 15:19:00 AEST 2026
On Fri, 17 Jul 2026 at 10:32, Zoltan Somogyi <zoltan.somogyi at runbox.com>
wrote:
>
> The diff is huge and boring.
Yes.
The only parts I am seeking feedback on
> are the parts marked by "XXX DIAG_SPEC" and a comment. The parts
> marked by "XXX DIAG_SPEC" without a new comment are reminders
> to myself about minor changes to do after this diff is committed.
> Use {err,warn,info}_spec as subtypes of diag_spec ...
...
> compiler/superhomogeneous_lambda.m:
> Return one (or more) of {err,warn,info}_specs instead of
> diag_specs. Perform the same replacements in data structures.
> (In most modules, diag_specs are replaced by *only* err_specs.)
>
> In modules where the diag_specs have all been replaced
> by the same subtype, keep using Specs as (part of) the names
> of the variables containing them. In modules that use
> more than one of the new subtypes, use ErrSpecs, WarnSpecs
> and/or InfoSpecs.
>
> Consistenly put err_specs before warn_specs, and both
> before info_specs.
s/Consistenly/Consistently/
...
> diff --git a/compiler/add_pragma_util.m b/compiler/add_pragma_util.m
> index 2d1a1f336..c61b80635 100644
> --- a/compiler/add_pragma_util.m
> +++ b/compiler/add_pragma_util.m
...
> @@ -295,10 +292,13 @@ look_up_pragma_pf_sym_arity(ModuleInfo,
IsFullyQualified, FailHandling,
> Specs = []
> ;
> InformIgnored = yes,
> - Severity =
> - severity_informational(inform_ignored_pragma_errors),
> - Spec = report_ambiguous_pred_or_func(Severity,
PragmaName,
> - Context, PredOrFunc, SymName, UserArity),
> + % XXX DIAG_SPEC
> + % This Spec used to have this severity:
> + % Severity =
> + % severity_informational(inform_ignored_pragma_errors),
> + % Should it stay that way?
> + Spec = report_ambiguous_pred_or_func(PragmaName, Context,
> + PredOrFunc, SymName, UserArity),
> Specs = [Spec]
Strictly speaking, unrecognised pragmas should be ignored. Practically,
since
there is only one Mercury implementation, it's probably better to treat them
as errors.
...
> diff --git a/compiler/check_module_interface.m
b/compiler/check_module_interface.m
> index 500110a36..b765b69e2 100644
> --- a/compiler/check_module_interface.m
> +++ b/compiler/check_module_interface.m
> @@ -30,7 +30,7 @@
> % report a warning.
> %
> :- pred check_module_interface_for_no_exports(globals::in,
> - parse_tree_module_src::in, list(diag_spec)::out) is det.
> + parse_tree_module_src::in, list(warn_spec)::out) is det.
>
>
%---------------------------------------------------------------------------%
>
%---------------------------------------------------------------------------%
> @@ -63,7 +63,7 @@ check_module_interface_for_no_exports(Globals,
ParseTreeModuleSrc, Specs) :-
> _IntFIMs, _ImpFIMs, _IntSelfFIMLangs, _ImpSelfFIMLangs,
>
> TypeCtorCheckedMap, InstCtorCheckedMap, ModeCtorCheckedMap,
> - TypeSpecs, InstModeSpecs,
> + TypeErrSpecs, TypeWarnSpecs, InstModeErrSpecs,
InstModeWarnSpecs,
>
> IntTypeClasses, IntInstances, IntPredDecls, IntModeDecls,
> IntDeclPragmas, IntDeclMarkers, IntPromises, _IntBadClauses,
> @@ -133,8 +133,15 @@ check_module_interface_for_no_exports(Globals,
ParseTreeModuleSrc, Specs) :-
> % a "no exports" warning or not. This means that the
warning
> % is not really needed *now*; it can be generated later,
> % once the complained-about invalid definitions are fixed.
> - TypeSpecs = [],
> - InstModeSpecs = [],
> + TypeErrSpecs = [],
> + InstModeErrSpecs = [],
> +
> + % XXX DIAG_SPECS
> + % These two conditions preserve existing behavior, from a
time
> + % where the parse tree stored co-mingled diag_specs. but
> + % I (zs) don't think it should be preserved.
I don't think it's worth preserving the old behaviour here.
...
> diff --git a/compiler/grab_modules.m b/compiler/grab_modules.m
> index b65725ec7..442c9c946 100644
> --- a/compiler/grab_modules.m
> +++ b/compiler/grab_modules.m
...
> @@ -725,10 +724,11 @@ grab_trans_opt_files(ProgressStream, Globals,
TransOptModuleNames,
>
> read_trans_opt_files(ProgressStream, Globals, TransOptModuleNames,
> cord.init, ParseTreeTransOptsCord,
> - [], NonBlockingSpecs, [], BlockingSpecs, !IO),
> + [], BlockErrSpecs, [], WarnSpecs, !IO),
> list.foldl(aug_compilation_unit_add_trans_opt,
> cord.list(ParseTreeTransOptsCord), !AugCompUnit),
> - module_baggage_add_nonfatal_specs(NonBlockingSpecs, !Baggage),
> + % XXX DIAG_SPECS We used to call module_baggage_add_nonfatal_specs.
> + module_baggage_add_warnings(WarnSpecs, !Baggage),
Looking at the nonfatal errors we have rme_unexpected_term_in_int_or_opt,
which seems relevant to reading .trans_opt files.
> % XXX why ignore any existing errors?
> !Baggage ^ mb_errors := init_read_module_errors,
>
...
> diff --git a/compiler/make_hlds_passes.m b/compiler/make_hlds_passes.m
> index bee01e019..67bc39850 100644
> --- a/compiler/make_hlds_passes.m
> +++ b/compiler/make_hlds_passes.m
...
> @@ -698,7 +707,9 @@ maybe_warn_include_and_non_include(Globals,
ParseTreeModuleSrc, Specs) :-
> words("Moving those entities to other modules can reduce"),
> words("the number of modules needing to be recompiled"),
> words("after a change."), nl],
> - Spec = spec($pred, severity_error, phase_pt2h, Context, Pieces),
> + % XXX DIAG_SPECS The severity used to be error.
> + Severity = severity_warning(warn_include_and_non_include),
> + Spec = spec($pred, Severity, phase_pt2h, Context, Pieces),
> Specs = [Spec]
> else
> Specs = []
The old behaviour was surely a mistake. The message says that this is a
warning, as does the name of this predicate.
> diff --git a/compiler/maybe_error.m b/compiler/maybe_error.m
> index 540e72d35..f62eb3f76 100644
> --- a/compiler/maybe_error.m
> +++ b/compiler/maybe_error.m
...
> @@ -106,36 +111,79 @@
>
> %---------------------%
>
> -:- func get_any_errors1(maybe1(T1)) = list(diag_spec).
> -:- func get_any_errors2(maybe2(T1, T2)) = list(diag_spec).
> -:- func get_any_errors3(maybe3(T1, T2, T3)) = list(diag_spec).
> -:- func get_any_errors4(maybe4(T1, T2, T3, T4)) = list(diag_spec).
> -:- func get_any_errors5(maybe5(T1, T2, T3, T4, T5)) = list(diag_spec).
> -:- func get_any_errors6(maybe6(T1, T2, T3, T4, T5, T6)) =
list(diag_spec).
> + % This type, which is used mostly by code that parses goals,
> + % allows us to record both
> + % - the presence of at least one error, and
> + % - the presence of zero or more warnings.
> + % The reason why we use tuples is that any readable function symbol
> + % we could use would make the lines that construct errors too long.
> +:- type err_warn_error == {one_or_more(err_spec), list(warn_spec)}.
>
> -:- func get_any_errors1el(maybe1el(T1)) = list(diag_spec).
> -:- func get_any_errors2el(maybe2el(T1, T2)) = list(diag_spec).
> -:- func get_any_errors3el(maybe3el(T1, T2, T3)) = list(diag_spec).
> -:- func get_any_errors4el(maybe4el(T1, T2, T3, T4)) = list(diag_spec).
> -:- func get_any_errors5el(maybe5el(T1, T2, T3, T4, T5)) =
list(diag_spec).
> -:- func get_any_errors6el(maybe6el(T1, T2, T3, T4, T5, T6)) =
list(diag_spec).
> +% XXX DIAG_SPECS
> +% The numbers in these names mirror the number of arguments of the okN
> +% function symbol, but they do not match the type constructor's arity,
> +% due to the warn_specs being implicit. Which is more important?
I would say the latter.
> +:- type parse_result2(T) ==
> + maybe2(T, list(warn_spec), err_warn_error).
> +:- type parse_result3(T1, T2) ==
> + maybe3(T1, T2, list(warn_spec), err_warn_error).
> +:- type parse_result4(T1, T2, T3) ==
> + maybe4(T1, T2, T3, list(warn_spec), err_warn_error).
>
...
> diff --git a/compiler/mercury_compile_args.m
b/compiler/mercury_compile_args.m
> index 8e53f54f1..a3c0bd2f6 100644
> --- a/compiler/mercury_compile_args.m
> +++ b/compiler/mercury_compile_args.m
...
> @@ -286,34 +288,37 @@ process_options_std(ProgressStream,
DefaultOptionTable, CmdLineArgs,
> DefaultOptionTable, ArgsOptionTable, cord.init, _UserData, !IO),
> (
> MaybeError = yes(OptionError),
> - Specs = report_option_error(OptionError),
> - Result = opr_failure(Specs)
> + OoMErrSpecs = report_option_error(OptionError),
> + Result = opr_failure(OoMErrSpecs, [])
> ;
> MaybeError = no,
> read_options_files_named_in_options_file_option(ProgressStream,
> ArgsOptionTable, EnvOptFileVariables0,
> - OptFileNonUndefSpecs, OptFileUndefSpecs, !IO),
> + OptFileErrSpecs, OptFileUndefWarnSpecs0, !IO),
> getopt.lookup_bool_option(ArgsOptionTable,
> warn_undefined_options_variables, WarnUndef),
> (
> WarnUndef = no,
> - OptFileSpecs = OptFileNonUndefSpecs
> + OptFileUndefWarnSpecs = []
> ;
> WarnUndef = yes,
> - OptFileSpecs = OptFileNonUndefSpecs ++ OptFileUndefSpecs
> + OptFileUndefWarnSpecs = OptFileUndefWarnSpecs0
> ),
> io.environment.get_environment_var_map(EnvVarMap, !IO),
> + % XXX DIAG_SPEC we used to include OptFileUndefWarnSpecs in this
Not sure about that one; are they written out somewhere else?
...
> diff --git a/compiler/parse_dcg_goal.m b/compiler/parse_dcg_goal.m
> index 70e3d7ed4..87e769306 100644
> --- a/compiler/parse_dcg_goal.m
> +++ b/compiler/parse_dcg_goal.m
...
> @@ -505,7 +509,7 @@ parse_dcg_goal_some_all(GoalKind, ArgTerms, Context,
ContextPieces,
> % to warrant a small amount of code target language code duplication.
> %
> :- pred parse_dcg_goal_conj(dcg_goal_kind, list(term),
> - prog_context, cord(format_piece), maybe2(goal, list(warning_spec)),
> + prog_context, cord(format_piece), parse_result2(goal),
> prog_varset, prog_varset, counter, counter, prog_var, prog_var).
> :- mode parse_dcg_goal_conj(in(dcg_goal_kind_conj), in, in, in, out,
> in, out, in, out, in, out) is det.
> @@ -518,10 +522,13 @@ parse_dcg_goal_conj(GoalKind, ArgTerms, Context,
ContextPieces,
> string_dcg_goal_kind(Functor, GoalKind),
> ( if ArgTerms = [TermA, TermB] then
> parse_dcg_goal_conjunction(Functor, TermA, TermB, Context,
> - ContextPieces, cord.init, ConjunctsCord, [], Warnings, [],
Errors,
> - !VarSet, !Counter, !DCGVar),
> + ContextPieces, cord.init, ConjunctsCord,
> + [], ErrSpecs, [], WarnSpecs, !VarSet, !Counter, !DCGVar),
> (
> - Errors = [],
> + ErrSpecs = [],
> + % XXX DIAG_SPEC In the past, we used to get here only if
> + % parse_dcg_goal_conjunction returned no diag_specs of
> + % *any* severity.
> Conjuncts = cord.list(ConjunctsCord),
The old behaviour seems wrong. parse_goal is called by
parse_dcg_goal_conjuction
and that can produce warnings.
...
> :- pred parse_dcg_goal_semicolon(list(term)::in,
> prog_context::in, cord(format_piece)::in,
> - maybe2(goal, list(warning_spec))::out,
> + parse_result2(goal)::out,
> prog_varset::in, prog_varset::out, counter::in, counter::out,
> prog_var::in, prog_var::out) is det.
> :- pragma inline(pred(parse_dcg_goal_semicolon/10)).
> @@ -611,10 +618,13 @@ parse_dcg_goal_semicolon(ArgTerms, Context,
ContextPieces,
> InitDCGVar = !.DCGVar,
> parse_dcg_goal_disjunction(InitDCGVar, ContextPieces,
> SubGoalTermA, SubGoalTermB, no, MaybeFirstDiffDCGVar,
> - cord.init, DisjunctsDCGVarsCord, [], Warnings, [],
ErrorSpecs,
> - !VarSet, !Counter),
> + cord.init, DisjunctsDCGVarsCord,
> + [], ErrSpecs, [], WarnSpecs, !VarSet, !Counter),
> (
> - ErrorSpecs = [],
> + ErrSpecs = [],
> + % XXX DIAG_SPEC In the past, we used to get here only if
> + % parse_dcg_goal_disjunction returned no diag_specs of
> + % *any* severity.
Ditto for this one.
...
> diff --git a/compiler/parse_goal.m b/compiler/parse_goal.m
> index c40c607a7..5f84f9ddf 100644
> --- a/compiler/parse_goal.m
> +++ b/compiler/parse_goal.m
...
> @@ -768,10 +772,13 @@ parse_goal_conj(GoalKind, ArgTerms, Context,
ContextPieces,
> string_goal_kind(Functor, GoalKind),
> ( if ArgTerms = [SubGoalTermA, SubGoalTermB] then
> parse_goal_conjunction(Functor, SubGoalTermA, SubGoalTermB,
> - ContextPieces, cord.init, ConjunctsCord, [], WarningSpecs,
> - [], ErrorSpecs, !VarSet),
> + ContextPieces, cord.init, ConjunctsCord,
> + [], ErrSpecs, [], WarnSpecs, !VarSet),
> (
> - ErrorSpecs = [],
> + ErrSpecs = [],
> + % XXX DIAG_SPEC In the past, we used to get here only if
> + % parse_goal_conjunction returned no diag_specs of
> + % *any* severity.
> Conjuncts = cord.list(ConjunctsCord),
As in the above DCG cases, the old behaviour seems wrong.
...
> @@ -852,28 +859,36 @@ parse_goal_semicolon(ArgTerms, Context,
ContextPieces, MaybeGoal, !VarSet) :-
> parse_goal(ElseGoalTerm, ContextPieces, MaybeElseGoal,
!VarSet),
> ( if
> MaybeCondGoal =
> - ok4(Vars, StateVars, CondGoal, CondWarningSpecs),
> - MaybeThenGoal = ok2(ThenGoal, ThenWarningSpecs),
> - MaybeElseGoal = ok2(ElseGoal, ElseWarningSpecs)
> + ok4(Vars, StateVars, CondGoal, CondWarnSpecs),
> + MaybeThenGoal = ok2(ThenGoal, ThenWarnSpecs),
> + MaybeElseGoal = ok2(ElseGoal, ElseWarnSpecs)
> then
> Goal = if_then_else_expr(Context, Vars, StateVars,
> CondGoal, ThenGoal, ElseGoal),
> - WarningSpecs = CondWarningSpecs ++
> - ThenWarningSpecs ++ ElseWarningSpecs,
> - MaybeGoal = ok2(Goal, WarningSpecs)
> + WarnSpecs = CondWarnSpecs ++
> + ThenWarnSpecs ++ ElseWarnSpecs,
> + MaybeGoal = ok2(Goal, WarnSpecs)
> else
> - Specs = get_any_errors_warnings4(MaybeCondGoal) ++
> - get_any_errors_warnings2(MaybeThenGoal) ++
> - get_any_errors_warnings2(MaybeElseGoal),
> - det_list_to_one_or_more(Specs, OoMSpecs),
> - MaybeGoal = error2(OoMSpecs)
> + get_all_errors_warnings4(MaybeCondGoal,
> + CondErrSpecs, CondWarnSpecs),
> + get_all_errors_warnings2(MaybeThenGoal,
> + ThenErrSpecs, ThenWarnSpecs),
> + get_all_errors_warnings2(MaybeElseGoal,
> + ElseErrSpecs, ElseWarnSpecs),
> + ErrSpecs = CondErrSpecs ++ ThenErrSpecs ++ ElseErrSpecs,
> + WarnSpecs = CondWarnSpecs ++ ThenWarnSpecs ++
ElseWarnSpecs,
> + det_list_to_one_or_more(ErrSpecs, OoMErrSpecs),
> + MaybeGoal = error2({OoMErrSpecs, WarnSpecs})
> )
> else
> parse_goal_disjunction(SubGoalTermA, SubGoalTermB,
ContextPieces,
> - cord.init, DisjunctsCord, [], WarningSpecs, [],
ErrorSpecs,
> + cord.init, DisjunctsCord, [], ErrSpecs, [], WarnSpecs,
> !VarSet),
> (
> - ErrorSpecs = [],
> + ErrSpecs = [],
> + % XXX DIAG_SPEC In the past, we used to get here only if
> + % parse_goal_disjunction returned no diag_specs of
> + % *any* severity.
> Disjuncts = cord.list(DisjunctsCord),
Ditto here.
...
> diff --git a/compiler/parse_item.m b/compiler/parse_item.m
> index 18e2ca3da..dc74b1d23 100644
> --- a/compiler/parse_item.m
> +++ b/compiler/parse_item.m
...
> @@ -1583,13 +1594,15 @@ read_term_to_iom_result(ModuleName, FileName,
ReadTermResult, ReadIOMResult,
> ReadTermResult = term(VarSet, Term),
> counter.allocate(SeqNum, !SeqNumCounter),
> parse_item_or_marker(ModuleName, VarSet, Term,
item_seq_num(SeqNum),
> - MaybeItemOrMarker),
> + MaybeItemOrMarker, [], WarnSpecs),
> (
> MaybeItemOrMarker = ok1(ItemOrMarker),
> + % XXX DIAG_SPEC This ignores WarnSpecs.
> ReadIOMResult = read_iom_ok(VarSet, Term, ItemOrMarker)
> ;
> - MaybeItemOrMarker = error1(Specs),
> - ReadIOMResult = read_iom_parse_item_errors(VarSet, Term,
Specs)
> + MaybeItemOrMarker = error1(OoMErrSpecs),
> + ReadIOMResult = read_iom_parse_item_errors(VarSet, Term,
> + OoMErrSpecs, WarnSpecs)
> )
> ).
>
...
> diff --git a/compiler/read_modules.m b/compiler/read_modules.m
> index 24482f774..dc2c19124 100644
> --- a/compiler/read_modules.m
> +++ b/compiler/read_modules.m
> @@ -1487,7 +1487,10 @@ record_and_report_missing_timestamp(Globals,
FileName, IOError,
> words("Smart recompilation will not work."), nl],
> Severity = severity_warning(warn_smart_recompilation),
> Spec = no_ctxt_spec($pred, Severity, phase_read_files, Pieces),
> - add_nonfatal_error(rme_cannot_find_modify_time, Spec, !Errors)
> + % XXX DIAG_SPEC This used to be:
> + % add_nonfatal_error(rme_cannot_find_modify_time, Spec,
!Errors)
> + % but this is a warning, not an error.
> + add_warning(Spec, !Errors)
> ;
> Warn = no
> ).
I don't know about that one. I would leave it marked with an XXX and whoever
undertakes the work need to complete smart recompilation can deal with it.
Julien.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20260717/2255d179/attachment-0001.html>
More information about the reviews
mailing list