FYI: better diagnostics on `+=' misuses (HEAD)

[Alexandre Duret-Lutz]

>>> "Nicolas" == Nicolas Joly <address@hidden> writes:

[...]

 Nicolas> === Running test ./pluseq3.test
 Nicolas> @address@hidden = zar \
 Nicolas> FAIL: pluseq3.test
 Nicolas> === Running test ./pluseq5.test
 Nicolas> FAIL: pluseq5.test

These two failures are a consequence of my previous change to
support `+=' in conditionals.  (This also shows I didn't run the
whole test suite before commiting such a big change; what a
shame!)

With the aforementioned patch we rewrite

| if COND
|   FOO = a
| else
|   FOO = b
| endif
| FOO += c

into 

| @address@hidden = a c
| @address@hidden = b c

by simply appending `c' to all exising `FOO' definitions.

The `pluseq3.test' and `pluseq5.test' cases show this is not
always correct.


`pluseq5.test' expects Automake to fail on the following:

| if CHECK
|   INCLUDES = abc
| endif
| INCLUDES += def

The test case doesn't explain why Automake should fail, but
one can reasonably assume it's because INCLUDES has not
been defined in all conditions where `+=' apply.

Presently, our "append `def' to all existing `INCLUDES'
definitions"-rule means that Automake will simply output

| @address@hidden = abc def

and nothing in the CHECK_FALSE case since there was no
definition.

As far as `pluseq5.test' is concerned, this is wrong because
Automake doesn't error out.


`pluseq3.test' is pretty much the converse.  It makes sure
that:

| if CHECK
|   data_DATA = zar
| endif
| if CHECK
|   data_DATA += \
|  doz
| else
|   data_DATA += dog
| endif

will be output as

| @address@hidden = zar \
| @CHECK_TRUE@ doz
| @address@hidden = dog

Watch the `dog'!  Too me, this seems to contradict both
`pluseq5.test' and the existing practice.  Why should we assume
that data_DATA was initaly empty in the CHACK_FALSE case?  why
shouldn't we bail out as in `pluseq5.test'.


I think the `pluseq5.test' makes sense: `X += Y' should not be
allowed in conditions where `X' is not *always* defined.  As a
consequence `pluseq3.test' must be fixed by changing the
definition of `data_DATA' to

| if CHECK
|   data_DATA = zar
| else
|   data_DATA =
| endif


Fixing Automake for `pluseq5.test' is harder.  Automake need to
find out whether "`X' is always defined in the current
condition".

Just consider

| if COND1
| if COND2
|    A = foo1
| else
|    A = foo2
| endif
| else
|    A = foo3
| endif

In this case, Automake should know that the set of all
conditions where A is defined, namely
  - "COND1_TRUE COND2_TRUE"
  - "COND1_TRUE COND2_FALSE"
  - "COND2_FALSE"
covers all possible conditions.

I though that would be easy: there is an &invert_condition
function in Automake, you can pass it a set of conditions and I
hoped it would return the complementary set of conditions.  So
if it returns an empty list, you know your conditions cover all
cases.  Unfortunately, this functions didn't worked as I
expected.  Instead it returned these conditions:
  - "COND1_FALSE COND2_TRUE"
  - "COND1_FALSE COND2_FALSE"

This makes no sense to me.  I've looked at the places where
&invert_condition is used, and I couldn't figure what was the
point in returning some conditions which imply the input
conditions.

I guess it was a bug (unrelated to these failures, but that
needs to be corrected in order to code the fix for the failures)
so I've rewritten this function.  As far our current example is
concerned, &invert_condition now returns an empty list, thus we
know that this set of conditions cover all possible cases.

Actually we are not really interested by `all possible cases',
but by `all possible cases under a given condition' (the
condition where `+=' applies).  The function
&variable_not_always_defined_in_cond does that.
It returns a list of conditions where the variable is not
defined so that Automake can print useful diagnostics.

For instance on `pluseq5.test':

| if CHECK
| INCLUDES = abc
| endif
| INCLUDES += def

It says:

| Makefile.am:4: Cannot apply `+=' because `INCLUDES' is not defined in
| Makefile.am:4: the following conditions:
| Makefile.am:4:   CHECK_FALSE
| Makefile.am:4: Either define `INCLUDES' in these conditions, or use
| Makefile.am:4: `+=' in the same conditions as the definitions.

Or on some more complex input (this is part of the new
`pluseq9.test'):

| if COND1
| if COND2
|     B = aa
| else
|     B = bb
| endif
| endif
| if COND3
|   B = cc
| endif
| B += dd

you get:

| Makefile.am:19: Cannot apply `+=' because `B' is not defined in
| Makefile.am:19: the following conditions:
| Makefile.am:19:   COND3_FALSE
| Makefile.am:19:   COND1_FALSE COND2_FALSE
| Makefile.am:19:   COND1_FALSE COND2_TRUE
| Makefile.am:19: Either define `B' in these conditions, or use
| Makefile.am:19: `+=' in the same conditions as the definitions.

Nifty, isn't it?  Oh well, of course the two latter conditions
could be merged into COND1_FALSE (any taker?).


Here is the patch.  I'm checking it on HEAD now, not because I'm
confident, but because I don't think it's possible to review
such kind of changes easily.  (Of course, this time I've run the
test suite.)

2002-05-16  Alexandre Duret-Lutz  <address@hidden>

        * automake.in (conditional_true_when): Return false if $WHEN == FALSE.
        (conditional_is_redundant): Simplify.
        (conditional_implies_one_of,
        variable_not_always_defined_in_cond): New functions
        (macro_define): Reject appends if the variable is not defined in
        all conditions where `+=' applies.
        (invert_conditions): Rewrite.  Before this patch,
        invert_conditions("A_TRUE B_TRUE", "A_TRUE B_FALSE", "A_FALSE")
        would return ("A_FALSE B_TRUE", "A_FALSE B_TRUE"), which seems
        wrong (these conditions implies "A_FALSE").  Now it outputs (),
        which just means the input conditions cover all cases.
        (variable_conditions_permutations): Never output FALSE conditions.
        * tests/pluseq2.test, tests/pluseq3.test: Define data_DATA
        in the CHECK_FALSE condition to fix the test.
        * tests/pluseq5.test: Actually check the diagnostic.
        * tests/pluseq9.test: New file.
        * tests/Makefile.am (TESTS): Add pluseq9.test.

Index: automake.in
===================================================================
RCS file: /cvs/automake/automake/automake.in,v
retrieving revision 1.1297
diff -u -r1.1297 automake.in
--- automake.in 14 May 2002 14:12:17 -0000      1.1297
+++ automake.in 16 May 2002 20:32:07 -0000
@@ -5351,6 +5351,10 @@
     # Make a hash holding all the values from $WHEN.
     my %cond_vals = map { $_ => 1 } split (' ', $when);
 
+    # Nothing is true when FALSE (not even FALSE itself, but it
+    # shouldn't hurt if you decide to change that).
+    return 0 if exists $cond_vals{'FALSE'};
+
     # Check each component of $cond, which looks `COND1 COND2'.
     foreach my $comp (split (' ', $cond))
     {
@@ -5376,18 +5380,30 @@
 {
     my ($cond, @whens) = @_;
 
-    if (@whens == 0)
+    @whens = ("") if @whens == 0;
+
+    foreach my $when (@whens)
     {
-       return 1 if conditional_true_when ($cond, "");
+       return 1 if conditional_true_when ($cond, $when);
     }
-    else
+    return 0;
+}
+
+
+# $BOOLEAN
+# &conditional_implies_one_of ($COND, @CONDS)
+# -------------------------------------------
+# Returns true iff $COND implies one of the conditions in @CONDS.
+sub conditional_implies_one_of ($@)
+{
+    my ($cond, @conds) = @_;
+
+    @conds = ("") if @conds == 0;
+
+    foreach my $c (@conds)
     {
-       foreach my $when (@whens)
-       {
-           return 1 if conditional_true_when ($cond, $when);
-       }
+       return 1 if conditional_true_when ($c, $cond);
     }
-
     return 0;
 }
 
@@ -5618,6 +5634,85 @@
     return '';
 }
 
+# @MISSING_CONDS
+# variable_not_always_defined_in_cond ($VAR, $COND)
+# ---------------------------------------------
+# Check whether $VAR is always defined for condition $COND.
+# Return a list of conditions where the definition is missing.
+#
+# For instance, given
+#
+#   if COND1
+#     if COND2
+#       A = foo
+#       D = d1
+#     else
+#       A = bar
+#       D = d2
+#     endif
+#   else
+#     D = d3
+#   endif
+#   if COND3
+#     A = baz
+#     B = mumble
+#   endif
+#   C = mumble
+#
+# we should have:
+#   variable_not_always_defined_in_cond ('A', 'COND1_TRUE COND2_TRUE')
+#     => ()
+#   variable_not_always_defined_in_cond ('A', 'COND1_TRUE')
+#     => ()
+#   variable_not_always_defined_in_cond ('A', 'TRUE')
+#     => ("COND1_FALSE COND2_FALSE COND3_FALSE",
+#         "COND1_FALSE COND2_TRUE COND3_FALSE",
+#         "COND1_TRUE COND2_FALSE COND3_FALSE",
+#         "COND1_TRUE COND2_TRUE COND3_FALSE")
+#   variable_not_always_defined_in_cond ('B', 'COND1_TRUE')
+#     => ("COND3_FALSE")
+#   variable_not_always_defined_in_cond ('C', 'COND1_TRUE')
+#     => ()
+#   variable_not_always_defined_in_cond ('D', 'TRUE')
+#     => ()
+#   variable_not_always_defined_in_cond ('Z', 'TRUE')
+#     => ("TRUE")
+#
+sub variable_not_always_defined_in_cond ($$)
+{
+    my ($var, $cond) = @_;
+
+    # It's easy to answer if the variable is not defined.
+    return ("TRUE",) unless exists $var_value{$var};
+
+    # How does it work?  Let's take the second example:
+    #
+    #   variable_not_always_defined_in_cond ('A', 'COND1_TRUE')
+    #
+    # (1) First, we get the list of conditions where A is defined:
+    #
+    #   ("COND1_TRUE COND2_TRUE", "COND1_TRUE COND2_FALSE", "COND3_TRUE")
+    #
+    # (2) Then we generate the set of inverted conditions:
+    #
+    #   ("COND1_FALSE COND2_TRUE COND3_FALSE",
+    #    "COND1_FALSE COND2_FALSE COND3_FALSE")
+    #
+    # (3) Finally we remove these conditions which are not implied by
+    #     COND1_TRUE.  This yields an empty list and we are done.
+
+    my @res = ();
+    my @cond_defs = keys %{$var_value{$var}}; # (1)
+    foreach my $icond (invert_conditions (@cond_defs)) # (2)
+    {
+       prog_error ("invert_conditions returned an input condition")
+           if exists $var_value{$var}{$icond};
+
+       push @res, $icond
+           if (conditional_true_when ($cond, $icond)); # (3)
+    }
+    return @res;
+}
 
 # &macro_define($VAR, $VAR_IS_AM, $TYPE, $COND, $VALUE, $WHERE)
 # -------------------------------------------------------------
@@ -5727,7 +5822,29 @@
       # Add VALUE to all definitions of VAR.
       foreach my $vcond (keys %{$var_value{$var}})
         {
-           &macro_define ($var, $var_is_am, '+', $vcond, $value, $where);
+         # We have a bit of error detection to do here.
+         # This:
+         #   if COND1
+         #     X = Y
+         #   endif
+         #   X += Z
+         # should be rejected because X is not defined for all conditions
+         # where `+=' applies.
+         my @undef_cond = variable_not_always_defined_in_cond $var, $cond;
+         if (@undef_cond != 0)
+           {
+             file_error ($where,
+                         "Cannot apply `+=' because `$var' is not defined "
+                         . "in\nthe following conditions:\n  "
+                         . join ("\n  ", @undef_cond)
+                         . "\nEither define `$var' in these conditions,"
+                         . " or use\n`+=' in the same conditions as"
+                         . " the definitions.");
+           }
+         else
+           {
+             &macro_define ($var, $var_is_am, '+', $vcond, $value, $where);
+           }
        }
     }
   # 3. first assignment (=, :=, or +=)
@@ -5933,6 +6050,7 @@
     %vars_scanned = ();
 
     my @new_conds = variable_conditions_recursive_sub ($var, '');
+
     # Now we want to return all permutations of the subvariable
     # conditions.
     my %allconds = ();
@@ -6131,25 +6249,49 @@
 # are never true for any of the input conditionals, and when taken
 # together with the input conditionals cover all possible cases.
 #
-# For example: invert_conditions("A_TRUE B_TRUE", "A_FALSE B_FALSE") will
-# return ("A_FALSE B_TRUE", "A_TRUE B_FALSE")
+# For example:
+#   invert_conditions("A_TRUE B_TRUE", "A_FALSE B_FALSE")
+#     => ("A_FALSE B_TRUE", "A_TRUE B_FALSE")
+#
+#   invert_conditions("A_TRUE B_TRUE", "A_TRUE B_FALSE", "A_FALSE")
+#     => ()
 sub invert_conditions
 {
     my (@conds) = @_;
 
     my @notconds = ();
-    foreach my $cond (@conds)
+
+    # Generate all permutation for all inputs.
+    my @perm =
+       map { variable_conditions_permutations (split(' ', $_)); } @conds;
+    # Remove redundant conditions.
+    @perm = variable_conditions_reduce @perm;
+
+    # Now remove all conditions which imply one of the input conditions.
+    foreach my $perm (@perm)
     {
-       foreach my $perm (variable_conditions_permutations (split(' ', $cond)))
-       {
-           push @notconds, $perm
-                   if ! conditional_is_redundant ($perm, @conds);
-       }
+       push @notconds, $perm
+           if ! conditional_implies_one_of ($perm, @conds);
     }
-    return variable_conditions_reduce (@notconds);
+    return @notconds;
 }
 
 # Return a list of permutations of a conditional string.
+# (But never output FALSE conditions, they are useless.)
+#
+# Examples:
+#   variable_conditions_permutations ("FOO_FALSE", "BAR_TRUE")
+#     => ("FOO_FALSE BAR_FALSE",
+#         "FOO_FALSE BAR_TRUE",
+#         "FOO_TRUE BAR_FALSE",
+#         "FOO_TRUE BAR_TRUE")
+#   variable_conditions_permutations ("FOO_FALSE", "TRUE")
+#     => ("FOO_FALSE TRUE",
+#         "FOO_TRUE TRUE")
+#   variable_conditions_permutations ("TRUE")
+#     => ("TRUE")
+#   variable_conditions_permutations ("FALSE")
+#     => ("TRUE")
 sub variable_conditions_permutations
 {
     my (@comps) = @_;
@@ -6163,13 +6305,13 @@
     my @ret;
     foreach my $sub (variable_conditions_permutations (@comps))
     {
-       push (@ret, "$comp $sub");
-       push (@ret, "$neg $sub");
+       push (@ret, "$comp $sub") if $comp ne 'FALSE';
+       push (@ret, "$neg $sub") if $neg ne 'FALSE';
     }
     if (! @ret)
     {
-       push (@ret, $comp);
-       push (@ret, $neg);
+       push (@ret, $comp) if $comp ne 'FALSE';
+       push (@ret, $neg) if $neg ne 'FALSE';
     }
     return @ret;
 }
Index: tests/Makefile.am
===================================================================
RCS file: /cvs/automake/automake/tests/Makefile.am,v
retrieving revision 1.397
diff -u -r1.397 Makefile.am
--- tests/Makefile.am   14 May 2002 14:12:21 -0000      1.397
+++ tests/Makefile.am   16 May 2002 20:32:07 -0000
@@ -254,6 +254,7 @@
 pluseq6.test \
 pluseq7.test \
 pluseq8.test \
+pluseq9.test \
 postproc.test \
 ppf77.test \
 pr2.test \
Index: tests/Makefile.in
===================================================================
RCS file: /cvs/automake/automake/tests/Makefile.in,v
retrieving revision 1.514
diff -u -r1.514 Makefile.in
--- tests/Makefile.in   14 May 2002 14:12:21 -0000      1.514
+++ tests/Makefile.in   16 May 2002 20:32:07 -0000
@@ -338,6 +338,7 @@
 pluseq6.test \
 pluseq7.test \
 pluseq8.test \
+pluseq9.test \
 postproc.test \
 ppf77.test \
 pr2.test \
Index: tests/pluseq2.test
===================================================================
RCS file: /cvs/automake/automake/tests/pluseq2.test,v
retrieving revision 1.3
diff -u -r1.3 pluseq2.test
--- tests/pluseq2.test  20 Oct 2001 11:17:17 -0000      1.3
+++ tests/pluseq2.test  16 May 2002 20:32:08 -0000
@@ -10,6 +10,8 @@
 
 if CHECK
 data_DATA = zar
+else
+data_DATA =
 endif
 
 if CHECK
Index: tests/pluseq3.test
===================================================================
RCS file: /cvs/automake/automake/tests/pluseq3.test,v
retrieving revision 1.4
diff -u -r1.4 pluseq3.test
--- tests/pluseq3.test  20 Oct 2001 11:17:17 -0000      1.4
+++ tests/pluseq3.test  16 May 2002 20:32:08 -0000
@@ -10,6 +10,8 @@
 
 if CHECK
 data_DATA = zar
+else
+data_DATA =
 endif
 
 if CHECK
Index: tests/pluseq5.test
===================================================================
RCS file: /cvs/automake/automake/tests/pluseq5.test,v
retrieving revision 1.2
diff -u -r1.2 pluseq5.test
--- tests/pluseq5.test  20 Oct 2001 11:17:17 -0000      1.2
+++ tests/pluseq5.test  16 May 2002 20:32:08 -0000
@@ -14,5 +14,21 @@
 END
 
 $ACLOCAL || exit 1
-$AUTOMAKE || exit 0
-exit 1
+$AUTOMAKE 2>stderr && exit 1
+
+cat stderr # for debugging
+
+# We expect the following diagnostic:
+#
+# Makefile.am:4: Cannot apply `+=' because `INCLUDES' is not defined in
+# Makefile.am:4: the following conditions:
+# Makefile.am:4:   CHECK_FALSE
+# Makefile.am:4: Either define `INCLUDES' in these conditions, or use
+# Makefile.am:4: `+=' in the same conditions as the definitions.
+
+# Is CHECK_FALSE mentioned?
+grep ':.*CHECK_FALSE$' stderr || exit 1
+# Is there only one missing condition?
+test `grep ':  ' stderr | wc -l` = 1 || exit 1
+
+:
Index: tests/pluseq9.test
===================================================================
RCS file: tests/pluseq9.test
diff -N tests/pluseq9.test
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/pluseq9.test  16 May 2002 20:32:08 -0000
@@ -0,0 +1,60 @@
+#! /bin/sh
+
+# Test the += diagnostics.
+
+. $srcdir/defs || exit 1
+
+cat >>configure.in <<EOF
+AM_CONDITIONAL(COND1, true)
+AM_CONDITIONAL(COND2, true)
+AM_CONDITIONAL(COND3, true)
+EOF
+
+cat > Makefile.am << 'END'
+if COND1
+if COND2
+    A = a
+    B = aa
+else
+    A = b
+    B = bb
+endif
+  A += c
+else
+  A = d
+endif
+A += e
+
+if COND3
+  A += f
+  B = cc
+endif
+B += dd
+END
+
+$ACLOCAL || exit 1
+$AUTOMAKE 2>stderr && exit 1
+
+cat stderr # for debugging
+
+# We expect the following diagnostic:
+#
+# Makefile.am:19: Cannot apply `+=' because `B' is not defined in
+# Makefile.am:19: the following conditions:
+# Makefile.am:19:   COND3_FALSE
+# Makefile.am:19:   COND1_FALSE COND2_FALSE
+# Makefile.am:19:   COND1_FALSE COND2_TRUE
+# Makefile.am:19: Either define `B' in these conditions, or use
+# Makefile.am:19: `+=' in the same conditions as the definitions.
+#
+# It would be nice if Automake could print only COND3_FALSE and
+# COND1_FALSE (merging the last two conditions), so we'll support
+# this case in the check too.
+
+# Are COND3_FALSE and COND1_FALSE mentioned?
+grep ':.*COND3_FALSE$' stderr || exit 1
+grep ':.*COND1_FALSE' stderr || exit 1
+# Make sure there are no more than three missing conditions.
+test `grep ':  ' stderr | wc -l` -le 3 || exit 1
+
+:

-- 
Alexandre Duret-Lutz