[m-rev.] for review: fix compiler abort with invalid instance

Julien Fischer juliensf at csse.unimelb.edu.au
Mon Nov 12 17:18:09 AEDT 2007


(This is still pending a bootcheck.)

Estimated hours taken: 1
Branches: main

Fix a bug (#27) reported by Ralph.  The compiler was aborting on 
instances that contained type variables that were not wrapped in functors.
The problem is that the name mangling scheme used to mangle the names of
method wrapper predicates cannot handle instances that contain unwrapped
type variables.  The compiler already has code to check for this, but that
check was not being performed until *after* it had attempted to introduce
the method wrapper predicates.

The fix is to reorder the phases of check_typeclass.m so that we perform
the check on the types in an instance first and only run the remaining
phases if the first one succeeds.

compiler/check_typeclass.m:
 	Reorder the phases within this module so that we don't attempt
 	to introduce method wrapper predicates for invalid instances.

 	Update the documentation in this module to conform to the above
 	change.

 	Remove some unnecessary module qualification.

tests/invalid/Mercury.options:
tests/invalid/Mmakefile:
tests/invalid/instance_var_bug.{m,err_exp}:
 	Test for the above bug.

Julien.

Index: compiler/check_typeclass.m
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/compiler/check_typeclass.m,v
retrieving revision 1.115
diff -u -r1.115 check_typeclass.m
--- compiler/check_typeclass.m	22 Oct 2007 05:10:32 -0000	1.115
+++ compiler/check_typeclass.m	12 Nov 2007 06:15:39 -0000
@@ -12,7 +12,14 @@
  % This module checks conformance of instance declarations to the typeclass
  % declaration. It takes various steps to do this.
  %
-% First, in check_instance_decls/6, for every method of every instance we
+% (1) in check_instance_declaration_types/4, we check that each type
+% in the instance declaration must be either a type with no arguments,
+% or a polymorphic type whose arguments are all distinct type variables.
+% We also check that all of the types in exported instance declarations are
+% in scope here.  XXX the latter part should really be done earlier, but with
+% the current implementation this is the most convenient spot.
+%
+% (2) in check_instance_decls/6, for every method of every instance we
  % generate a new pred whose types and modes are as expected by the typeclass
  % declaration and whose body just calls the implementation provided by the
  % instance declaration.
@@ -47,7 +54,7 @@
  % using the instance constraints as assumptions.  At this point we fill in
  % the super class proofs.
  %
-% Second, in check_for_cyclic_classes/4, we check for cycles in the typeclass
+% (3) in check_for_cyclic_classes/4, we check for cycles in the typeclass
  % hierarchy.  A cycle occurs if we can start from any given typeclass
  % declaration and follow the superclass constraints on classes to reach the
  % same class that we started from.  Only the class_id needs to be repeated;
@@ -55,10 +62,10 @@
  % on class declarations only, not those on instance declarations.  While doing
  % this, we fill in the fundeps_ancestors field in the class table.
  %
-% Third, in check_for_missing_concrete_instances/4, we check that each
+% (4) in check_for_missing_concrete_instances/4, we check that each
  % abstract instance has a corresponding concrete instance.
  %
-% Fourth, in check_functional_dependencies/4, all visible instances are
+% (5) in check_functional_dependencies/4, all visible instances are
  % checked for coverage and mutual consistency with respect to any functional
  % dependencies.  This doesn't necessarily catch all cases of inconsistent
  % instances, however, since in general that cannot be done until link time.
@@ -70,14 +77,7 @@
  % generated, we should add the concrete definitions before this pass to
  % ensure that they get checked.
  %
-% Fifth, in check_instance_declaration_types/4, we check that each type
-% in the instance declaration must be either a type with no arguments,
-% or a polymorphic type whose arguments are all distinct type variables.
-% We also check that all of the types in exported instance declarations are
-% in scope here.  XXX that should really be done earlier, but with the
-% current implementation this is the most convenient spot.
-%
-% Last, in check_typeclass_constraints/4, we check typeclass constraints on
+% (6) in check_typeclass_constraints/4, we check typeclass constraints on
  % predicate and function declarations and on existentially typed data
  % constructors for ambiguity, taking into consideration the information
  % provided by functional dependencies.  We also call check_constraint_quant/5
@@ -149,41 +149,59 @@
  check_typeclasses(!ModuleInfo, !QualInfo, !Specs) :-
      module_info_get_globals(!.ModuleInfo, Globals),
      globals.lookup_bool_option(Globals, verbose, Verbose),
+
      trace [io(!IO1)] (
-        maybe_write_string(Verbose, "% Checking typeclass instances...\n",
-            !IO1)
-    ),
-    check_instance_decls(!ModuleInfo, !QualInfo, !Specs),
-
-    trace [io(!IO2)] (
-        maybe_write_string(Verbose, "% Checking for cyclic classes...\n",
-            !IO2)
-    ),
-    check_for_cyclic_classes(!ModuleInfo, !Specs),
-
-    trace [io(!IO3)] (
      maybe_write_string(Verbose,
-        "% Checking for missing concrete instances...\n", !IO3)
+        "% Checking instance declaration types...\n", !IO1)
      ),
-    check_for_missing_concrete_instances(!ModuleInfo, !Specs),
+    check_instance_declaration_types(!ModuleInfo, !Specs),
+ 
+    % If we encounter any errors while checking that the types in an
+    % instance declaration are valid then don't attempt the remaining
+    % passes.  Pass 2 cannot be run since the name mangling scheme we
+    % use to generate the names of the method wrapper predicates may 
+    % abort if the types in an instance are not valid, e.g. if an
+    % instance head contains a type variable that is not wrapped inside
+    % a functor.  Most of the other passes also depend upon information
+    % that is calculated during pass 2.
+    %
+    % XXX it would be better to just remove the invalid instances at this
+    % point and then continue on with the valid instances.
+    %
+    (
+        !.Specs = [],
+        trace [io(!IO2)] (
+            maybe_write_string(Verbose,
+                "% Checking typeclass instances...\n", !IO2)
+        ),
+        check_instance_decls(!ModuleInfo, !QualInfo, !Specs),
+ 
+        trace [io(!IO3)] (
+            maybe_write_string(Verbose,
+                "% Checking for cyclic classes...\n", !IO3)
+        ),
+        check_for_cyclic_classes(!ModuleInfo, !Specs),

-    trace [io(!IO4)] (
-    maybe_write_string(Verbose,
-        "% Checking functional dependencies on instances...\n", !IO4)
-    ),
-    check_functional_dependencies(!ModuleInfo, !Specs),
+        trace [io(!IO4)] (
+            maybe_write_string(Verbose,
+                "% Checking for missing concrete instances...\n", !IO4)
+        ),
+        check_for_missing_concrete_instances(!ModuleInfo, !Specs),

-    trace [io(!IO5)] (
-    maybe_write_string(Verbose,
-        "% Checking instance declaration types...\n", !IO5)
-    ),
-    check_instance_declaration_types(!ModuleInfo, !Specs),
+        trace [io(!IO5)] (
+            maybe_write_string(Verbose,
+                "% Checking functional dependencies on instances...\n", !IO5)
+        ),
+        check_functional_dependencies(!ModuleInfo, !Specs),

-    trace [io(!IO6)] (
-    maybe_write_string(Verbose, "% Checking typeclass constraints...\n",
-        !IO6)
-    ),
-    check_typeclass_constraints(!ModuleInfo, !Specs).
+        trace [io(!IO6)] (
+            maybe_write_string(Verbose,
+                "% Checking typeclass constraints...\n", !IO6)
+        ),
+        check_typeclass_constraints(!ModuleInfo, !Specs)
+    ;
+        !.Specs = [_ | _]
+    ).

  %---------------------------------------------------------------------------%

@@ -235,7 +253,7 @@
          !:Specs = [Spec | !.Specs],
          InstanceDefns = InstanceDefns0
      ;
-        solutions.solutions(
+        solutions(
              ( pred(PredId::out) is nondet :-
                  list.member(ClassProc, ClassInterface),
                  ClassProc = hlds_class_proc(PredId, _)
@@ -431,7 +449,7 @@
          !InstanceCheckInfo, !Specs) :-
      !.InstanceCheckInfo = instance_check_info(InstanceDefn0,
          OrderedMethods0, ModuleInfo0, QualInfo0),
-    solutions.solutions((pred(ProcId::out) is nondet :-
+    solutions((pred(ProcId::out) is nondet :-
              list.member(ClassProc, ClassInterface),
              ClassProc = hlds_class_proc(PredId, ProcId)
          ), ProcIds),
Index: tests/invalid/Mercury.options
===================================================================
RCS file: /home/mercury/mercury1/repository/tests/invalid/Mercury.options,v
retrieving revision 1.26
diff -u -r1.26 Mercury.options
--- tests/invalid/Mercury.options	25 Oct 2007 05:05:13 -0000	1.26
+++ tests/invalid/Mercury.options	12 Nov 2007 06:15:39 -0000
@@ -51,6 +51,7 @@
  MCFLAGS-invalid_mllibs =	--no-errorcheck-only --no-verbose-make \
  				--options-file Mercury.options.invalid \
  				--make invalid_mllibs
+MCFLAGS-instance_var_bug =      --verbose-error-messages
  MCFLAGS-loopcheck =		--warn-inferred-erroneous \
  				--verbose-error-messages
  MCFLAGS-method_impl =		--no-intermodule-optimization \
Index: tests/invalid/Mmakefile
===================================================================
RCS file: /home/mercury/mercury1/repository/tests/invalid/Mmakefile,v
retrieving revision 1.225
diff -u -r1.225 Mmakefile
--- tests/invalid/Mmakefile	25 Oct 2007 05:05:14 -0000	1.225
+++ tests/invalid/Mmakefile	12 Nov 2007 06:15:39 -0000
@@ -113,6 +113,7 @@
  	instance_bug \
  	instance_dup_var \
  	instance_no_type \
+	instance_var_bug \
  	invalid_event \
  	invalid_export_detism \
  	invalid_import_detism \
Index: tests/invalid/instance_var_bug.err_exp
===================================================================
RCS file: tests/invalid/instance_var_bug.err_exp
diff -N tests/invalid/instance_var_bug.err_exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ tests/invalid/instance_var_bug.err_exp	12 Nov 2007 06:15:39 -0000
@@ -0,0 +1,4 @@
+instance_var_bug.m:014: In instance declaration for `instance_var_bug.tc(V)':
+instance_var_bug.m:014:   the first arg is a type variable
+instance_var_bug.m:014:   types in instance declarations must be functors with
+instance_var_bug.m:014:   distinct variables as arguments
Index: tests/invalid/instance_var_bug.m
===================================================================
RCS file: tests/invalid/instance_var_bug.m
diff -N tests/invalid/instance_var_bug.m
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ tests/invalid/instance_var_bug.m	12 Nov 2007 06:15:39 -0000
@@ -0,0 +1,14 @@
+% This program cause the following assertion failure in rotd-2007-11-11:
+%
+% Software Error: hlds_code_util.m: Unexpected: type_to_string: invalid type
+%
+% The problem was that the name mangling scheme used for the names of method 
+% wrapper predicates cannot handle type variables.  The compiler should
+% have been reporting the instance tc(V) as an error anyway, but it wasn't
+% doing that check until *after* it had attempted to add the method wrapper.
+%
+:- module instance_var_bug.
+:- interface.
+:- typeclass tc(T) where [ pred p(T::in) is semidet ].
+:- implementation.
+:- instance tc(V) where [ p(_) :- semidet_true ].

--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------



More information about the reviews mailing list