[m-rev.] for review: printing error messages in order

Paul Bone pbone at csse.unimelb.edu.au
Tue Aug 4 15:20:03 AEST 2009


On Mon, Jul 27, 2009 at 03:45:14PM +1000, Zoltan Somogyi wrote:
> With this diff, I am asking for two things. First is a review by anyone.
> However, I am also asking for practical tryouts. I am now using a version
> of the compiler with this diff on my laptop, but since I don't use mmc --make,
> I am also installing it on goliath for mmc --make users to try out. This
> should provide a more thorough test than just a review.
> 
> The directory you would need to add to the front of your path to use
> this test version of the compiler is
> 
> /home/goliath/workspaces/zs/test_install/bin
> 
> The install should be done by about 6 or 7pm.

I haven't tested this.  I have reviewed the diff below.

I'm not sure context-order is always the best order for error messages.
Sometimes a syntax error can cause a clause to be removed from a
program, which can result in further errors.  Either a predicate
declaration with no definition error or an incorrectly-moded predicate
error.  In these cases I'd like to read the syntax error first.  It
might also be best not to show the user the second error since it is
caused by (or related to) the first.

If errors are given in context order the predicate declaration with no
definition error will appear before the syntax error because people
write declarations before definitions in their programs.

The Sun Java compiler will not typecheck a program if it finds any
syntax errors, this is pretty much the opposite behaviour and is also
not desirable.  It's useful to check as much of the program as possible
(and I think most people will agree).  Perhaps in the example above
where a clause contains a syntax error the definition of the predicate
can be replaced by a dummy expression so that compilation of other parts
of the module can continue and users are not shown extra errors caused
by their syntax errors.


> compiler/mercury_compile.m:
> compiler/intermod.m:
> 	Gather up error messages to print in batches.
> 
> 	In the parts of the code affected by the above, explicitly pass
> 	around a globals structure, instead of having all the predicates
> 	get it of the I/O state. Since some code (e.g. the start recompilation
> 	system) still uses the copy in the I/O state, we ensure that this copy
> 	is kept in sync with the explicitly passed around copy.
> 

Why have two places where the same information is stored?  Will this be
changed in the future?

> Index: compiler/error_util.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/error_util.m,v
> retrieving revision 1.70
> diff -u -b -r1.70 error_util.m
> --- compiler/error_util.m	21 Jul 2009 04:10:40 -0000	1.70
> +++ compiler/error_util.m	23 Jul 2009 11:20:25 -0000
> @@ -240,6 +240,11 @@
>  
>  %-----------------------------------------------------------------------------%
>  
> +:- pred maybe_write_out_errors_no_module(bool::in, globals::in,
> +    list(error_spec)::in, list(error_spec)::out, io::di, io::uo) is det.
> +

This declaration looks like it is in the interface of this module.  It should
probably have a documentation string above it.

> +%-----------------------------------------------------------------------------%
> +
>      % write_error_spec(Spec, Globals, !NumWarnings, !NumErrors, !IO):
>      % write_error_specs(Specs, Globals, !NumWarnings, !NumErrors, !IO):
>      %
> @@ -703,22 +708,35 @@
>  
>  %-----------------------------------------------------------------------------%
>  
> +maybe_write_out_errors_no_module(Verbose, Globals, !Specs, !IO) :-
> +    % maybe_write_out_errors in hlds_error_util.m is a HLDS version
> +    % of this predicate. The documentstion is in that file.
> +    (
> +        Verbose = no
> +    ;
> +        Verbose = yes,
> +        % XXX _NumErrors
> +        write_error_specs(!.Specs, Globals,
> +            0, _NumWarnings, 0, _NumErrors, !IO),
> +        !:Specs = []
> +    ).
> +

What is ment by XXX.  My attention is drawn to _NumErrors but I don't know why.

> Index: compiler/intermod.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/intermod.m,v
> retrieving revision 1.245
> diff -u -b -r1.245 intermod.m
> --- compiler/intermod.m	11 Jun 2009 07:00:11 -0000	1.245
> +++ compiler/intermod.m	24 Jul 2009 07:43:04 -0000
> @@ -2306,29 +2309,31 @@
>  
>      % Read in and process the optimization interfaces.
>      %
> -grab_opt_files(!Module, FoundError, !IO) :-
> +grab_opt_files(Globals, !Module, FoundError, !IO) :-
>      % Read in the .opt files for imported and ancestor modules.
> -    ModuleName = !.Module ^ module_name,
> -    Ancestors0 = !.Module ^ parent_deps,
> -    InterfaceDeps0 = !.Module ^ int_deps,
> -    ImplementationDeps0 = !.Module ^ impl_deps,
> +    ModuleName = !.Module ^ mai_module_name,
> +    Ancestors0 = !.Module ^ mai_parent_deps,
> +    InterfaceDeps0 = !.Module ^ mai_int_deps,
> +    ImplementationDeps0 = !.Module ^ mai_impl_deps,
>      OptFiles = list.sort_and_remove_dups(list.condense(
>          [Ancestors0, InterfaceDeps0, ImplementationDeps0])),
> -    globals.io_lookup_bool_option(read_opt_files_transitively, Transitive,
> -        !IO),
> +    globals.lookup_bool_option(Globals, read_opt_files_transitively,
> +        Transitive),
>      ModulesProcessed = set.insert(set.sorted_list_to_set(OptFiles),
>          ModuleName),
> -    read_optimization_interfaces(Transitive, ModuleName, OptFiles,
> -        ModulesProcessed, cord.empty, OptItemsCord, no, OptError, !IO),
> -    OptItems = cord.list(OptItemsCord),
> +    read_optimization_interfaces(Globals, Transitive, ModuleName, OptFiles,
> +        ModulesProcessed, cord.empty, OptItemsCord, [], OptSpecs, no, OptError,
> +        !IO),
>  
>      % Append the items to the current item list, using a `opt_imported'
> -    % psuedo-declaration to let make_hlds know the opt_imported stuff
> +    % pseudo-declaration to let make_hlds know the opt_imported stuff
>      % is coming.
> -    module_imports_get_items(!.Module, Items0),
> -    Items1 = Items0 ++ cord.from_list([
> -        make_pseudo_decl(md_opt_imported) | OptItems]),
> -    module_imports_set_items(Items1, !Module),
> +    % 
> +    % XXX Using this mechanism to let make_hlds know this is a bad design.
> +    OptItems = cord.list(OptItemsCord),
> +    AddedItems = [make_pseudo_decl(md_opt_imported) | OptItems],
> +    module_and_imports_add_items(cord.from_list(AddedItems), !Module),
> +    module_and_imports_add_specs(OptSpecs, !Module),

Why turn a cord into a list just to cons something to it and then turn it back
into a cord?  I recommend using the cord module's cons function.


> Index: compiler/mercury_compile.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/mercury_compile.m,v
> retrieving revision 1.498
> diff -u -b -r1.498 mercury_compile.m
> --- compiler/mercury_compile.m	21 Jul 2009 04:10:40 -0000	1.498
> +++ compiler/mercury_compile.m	27 Jul 2009 03:52:59 -0000
> @@ -1110,7 +1150,10 @@
>          ConvertToMercury = yes
>      ->
>          read_module_or_file(FileOrModule, do_not_return_timestamp,
> -            ModuleName, _, _, Items, Error, map.init, _, !IO),
> +            ModuleName, _, _, Items, Specs, Error, map.init, _, !Globals, !IO),
> +        % XXX _NumErrors
> +        write_error_specs(Specs, !.Globals, 0, _NumWarnings, 0, _NumErrors,
> +            !IO),
>          ( halt_at_module_error(HaltSyntax, Error) ->
>              true
>          ;

And here.  What does the XXX mean?

> Index: compiler/modes.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/modes.m,v
> retrieving revision 1.379
> diff -u -b -r1.379 modes.m
> --- compiler/modes.m	21 Jul 2009 02:08:49 -0000	1.379
> +++ compiler/modes.m	21 Jul 2009 10:26:34 -0000
> @@ -160,8 +160,8 @@
>      % was halted prematurely due to an error, and that we should therefore
>      % not perform determinism-checking, because we might get internal errors.
>      %
> -:- pred modecheck_module(module_info::in, module_info::out,
> -    modes_safe_to_continue::out, list(error_spec)::out) is det.
> +:- pred modecheck_module(module_info::in,
> +    {module_info, modes_safe_to_continue, list(error_spec)}::out) is det.
>  
>      % Mode-check or unique-mode-check the code of all the predicates
>      % in a module.

When reading this without the context of the changelog I don't know why a tuple
is used here.  Please add a small comment.

> Index: compiler/modules.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/modules.m,v
> retrieving revision 1.453
> diff -u -b -r1.453 modules.m
> --- compiler/modules.m	16 Jan 2009 02:31:26 -0000	1.453
> +++ compiler/modules.m	24 Jul 2009 05:57:01 -0000
> @@ -404,28 +414,36 @@
>      % these to qualify all the declarations as much as possible. Then write
>      % out the .int0 file.
>      %
> -make_private_interface(SourceFileName, SourceFileModuleName, ModuleName,
> -        MaybeTimestamp, Items0, !IO) :-
> -    grab_unqual_imported_modules(SourceFileName, SourceFileModuleName,
> -        ModuleName, Items0, Module, Error, !IO),
> +make_private_interface(Globals, SourceFileName, SourceFileModuleName,
> +        ModuleName, MaybeTimestamp, Items0, !IO) :-
> +    grab_unqual_imported_modules(Globals, SourceFileName, SourceFileModuleName,
> +        ModuleName, Items0, Module, !IO),
>  
>      % Check whether we succeeded.
> -    % XXX zs: why does this code not check for fatal_module_errors?
> -    ( Error = some_module_errors ->
> +    % XXX zs: why is fatal_module_errors with no_module_errors instead of
> +    % some_module_errors?

This sentence doesn't read well.  I don't know what you mean.

> +    module_and_imports_get_results(Module, Items1, Specs0, Error),
> +    (
> +        Error = some_module_errors,
>          module_name_to_file_name(ModuleName, ".int0", do_not_create_dirs,
>              FileName, !IO),
> +        % XXX _NumErrors

As above.  Perhaps there's a comment somewhere that describes all of these.  If
so these should point me at that comment.

> +        write_error_specs(Specs0, Globals, 0, _NumWarnings, 0, _NumErrors,
> +            !IO),
>          io.write_strings(["Error reading interface files.\n",
>              "`", FileName, "' not written.\n"], !IO)
>      ;
> +        ( Error = no_module_errors
> +        ; Error = fatal_module_errors
> +        ),
>          % Module-qualify all items.
> -        module_imports_get_items_list(Module, Items1),
> -        globals.io_get_globals(Globals, !IO),
>          module_name_to_file_name(ModuleName, ".m", do_not_create_dirs,
>              FileName, !IO),
>          module_qualify_items(Items1, Items2, map.init, _, Globals, ModuleName,
> -            yes(FileName), "", _, _, _, [], Specs),
> +            yes(FileName), "", _, _, _, Specs0, Specs),
>          (
>              Specs = [_ | _],
> +            % XXX _NumErrors

And again.

>              write_error_specs(Specs, Globals, 0, _NumWarnings, 0, _NumErrors,
>                  !IO),
>              io.write_strings(["`", FileName, "' not written.\n"], !IO)
> @@ -447,7 +465,7 @@
>              handle_mutables_in_private_interface(ModuleName, Items4, Items5),
>              list.map(make_any_instances_abstract, Items5, Items6),
>              list.reverse(Items6, Items),
> -            write_interface_file(SourceFileName, ModuleName,
> +            write_interface_file(Globals, SourceFileName, ModuleName,
>                  ".int0", MaybeTimestamp,
>                  [make_pseudo_decl(md_interface) | Items], !IO),
>              touch_interface_datestamp(ModuleName, ".date0", !IO)
> @@ -522,19 +540,25 @@
>      % to qualify all items in the interface as much as possible. Then write out
>      % the .int and .int2 files.
>      %
> -make_interface(SourceFileName, SourceFileModuleName, ModuleName,
> +make_interface(Globals, SourceFileName, SourceFileModuleName, ModuleName,
>          MaybeTimestamp, Items0, !IO) :-
>      some [!InterfaceItems] (
>          get_interface(ModuleName, yes, Items0, !:InterfaceItems),
>  
>          % Get the .int3 files for imported modules.
> -        grab_unqual_imported_modules(SourceFileName, SourceFileModuleName,
> -            ModuleName, !.InterfaceItems, Module0, Error, !IO),
> +        grab_unqual_imported_modules(Globals, SourceFileName,
> +            SourceFileModuleName, ModuleName, !.InterfaceItems, Module0, !IO),
>  
>          % Check whether we succeeded.
> -        module_imports_get_items_list(Module0, !:InterfaceItems),
> -        % XXX zs: why does this code not check for fatal_module_errors?
> -        ( Error = some_module_errors ->
> +        module_and_imports_get_results(Module0, !:InterfaceItems,
> +            Specs0, Error),
> +        % XXX zs: why is fatal_module_errors with no_module_errors instead of
> +        % some_module_errors?
> +        (
> +            Error = some_module_errors,
> +            % XXX _NumErrors

These comments again.  I now know what you mean, you're referring to the cases
of the switch arms.

> +            write_error_specs(Specs0, Globals, 0, _NumWarnings, 0, _NumErrors,
> +                !IO),
>              module_name_to_file_name(ModuleName, ".int", do_not_create_dirs,
>                  IntFileName, !IO),
>              module_name_to_file_name(ModuleName, ".int2", do_not_create_dirs,
> @@ -543,12 +567,14 @@
>                  "`", IntFileName, "' and ",
>                  "`", Int2FileName, "' not written.\n"], !IO)
>          ;
> +            ( Error = no_module_errors
> +            ; Error = fatal_module_errors
> +            ),
>              % Module-qualify all items.
> -            globals.io_get_globals(Globals, !IO),
>              module_name_to_file_name(ModuleName, ".m", do_not_create_dirs,
>                  FileName, !IO),
>              module_qualify_items(!InterfaceItems, map.init, _, Globals,
> -                ModuleName, yes(FileName), "", _, _, _, [], Specs),
> +                ModuleName, yes(FileName), "", _, _, _, Specs0, Specs),
>  
>              % We want to finish writing the interface file (and keep
>              % the exit status at zero) if we found some warnings.
> @@ -570,24 +596,25 @@
>                  strip_assertions(!InterfaceItems),
>                  strip_unnecessary_impl_defns(!InterfaceItems),
>                  check_for_clauses_in_interface(!InterfaceItems, [],
> -                    InterfaceSpecs),
> +                    InterfaceSpecs0),
>                  % XXX _NumErrors

And here.

> @@ -2184,40 +2203,40 @@
>  
>  %-----------------------------------------------------------------------------%
>  
> -generate_module_dependencies(ModuleName, !IO) :-
> +generate_module_dependencies(Globals, ModuleName, !IO) :-
>      map.init(DepsMap),
> -    generate_dependencies(output_all_dependencies, do_not_search, ModuleName,
> -        DepsMap, !IO).
> +    generate_dependencies(Globals, output_all_dependencies, do_not_search,
> +        ModuleName, DepsMap, !IO).
>  
> -generate_file_dependencies(FileName, !IO) :-
> -    build_deps_map(FileName, ModuleName, DepsMap, !IO),
> -    generate_dependencies(output_all_dependencies, do_not_search, ModuleName,
> -        DepsMap, !IO).
> +generate_file_dependencies(Globals, FileName, !IO) :-
> +    build_deps_map(Globals, FileName, ModuleName, DepsMap, !IO),
> +    generate_dependencies(Globals, output_all_dependencies, do_not_search,
> +        ModuleName, DepsMap, !IO).
>  
> -generate_module_dependency_file(ModuleName, !IO) :-
> +generate_module_dependency_file(Globals, ModuleName, !IO) :-
>      map.init(DepsMap),
> -    generate_dependencies(output_d_file_only, do_search, ModuleName,
> +    generate_dependencies(Globals, output_d_file_only, do_search, ModuleName,
>          DepsMap, !IO).
>  
> -generate_file_dependency_file(FileName, !IO) :-
> -    build_deps_map(FileName, ModuleName, DepsMap, !IO),
> -    generate_dependencies(output_d_file_only, do_search, ModuleName,
> +generate_file_dependency_file(Globals, FileName, !IO) :-
> +    build_deps_map(Globals, FileName, ModuleName, DepsMap, !IO),
> +    generate_dependencies(Globals, output_d_file_only, do_search, ModuleName,
>          DepsMap, !IO).
>  
> -:- pred build_deps_map(file_name::in, module_name::out, deps_map::out,
> -    io::di, io::uo) is det.
> +:- pred build_deps_map(globals::in, file_name::in,
> +    module_name::out, deps_map::out, io::di, io::uo) is det.
>  
> -build_deps_map(FileName, ModuleName, DepsMap, !IO) :-
> +build_deps_map(Globals, FileName, ModuleName, DepsMap, !IO) :-
>      % Read in the top-level file (to figure out its module name).
>      read_module_from_file(FileName, ".m", "Reading file", do_not_search,
> -        do_not_return_timestamp, Items, Error, ModuleName, _, !IO),
> +        do_not_return_timestamp, Items, Specs0, Error, ModuleName, _, !IO),
>      SourceFileName = FileName ++ ".m",
> -    split_into_submodules(ModuleName, Items, SubModuleList, [], Specs),
> -    globals.io_get_globals(Globals, !IO),
> +    split_into_submodules(ModuleName, Items, SubModuleList, Specs0, Specs),
> +    % XXX _NumErrors

And here.

> Index: compiler/state_var.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/state_var.m,v
> retrieving revision 1.27
> diff -u -b -r1.27 state_var.m
> --- compiler/state_var.m	28 Jul 2008 08:34:19 -0000	1.27
> +++ compiler/state_var.m	22 Jul 2009 03:10:39 -0000
> @@ -353,6 +353,7 @@
>  
>  :- import_module char.
>  :- import_module int.
> +:- import_module io.
>  :- import_module pair.
>  :- import_module string.
>  :- import_module svmap.

This is the only change in this module.  Unless this module didn't compile
without this change then this change is not required.

The rest of the diff is fine.

Thanks.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20090804/5bbbf332/attachment.sig>


More information about the reviews mailing list