Last part of PR/347: warn about $(foo bar).

[Alexandre Duret-Lutz]

This patch teaches Automake how to warn about $(foo bar)-style
variables.  People frequently use GNU make extensions
(e.g., `$(wildcard *.dat)' or `$(addsuffix .c,foo bar)') assuming
they are portable (or assuming nothing at all :)).

Automake will thus warn about any variable name that does not
meet POSIX (address@hidden') and hint the user about a
possible use of a `GNU make extension' when the bogus name
contain a space.

I've used one of the new functions (&subvariables_in) to
simplify &variable_conditions_recursive_sub.  The change is not
stricly equivalant because &variable_conditions_recursive_sub
will now recurse in all subvariables even when they are not
separated by a space as in
  `$(foo)$(bar)'
(before this patch &variable_conditions_recursive_sub would attempt
to recurse into the `foo)$(bar' variable).

In practice this should not make any difference since this
returns only conditionals, and the other functions that retrun
*values* still split on spaces.  I don't know how hard it would
be to support non-space-separated variables in other places; at
least this is a first step.

Comments?  Fears?

PS: I'd buy any better name for subvariables_in and subvariables_of...

2002-08-21  Alexandre Duret-Lutz  <address@hidden>

        For PR automake/347:
        * automake.in (MACRO_PATTERN): Allow `.' in variable names.
        (check_typos): Rename as ...
        (check_variables): ... this. Warn about non-POSIX variable names.
        (macro_define): Move the leading-`_' check ...
        (check_variables): ... here.
        (subvariables_in, subvariables_of): New functions.
        (variable_conditions_recursive_sub): Simplify using subvariables_in.
        * tests/vars3.test: New file.
        * tests/Makefile.am (TESTS): Add vars3.test.
        * tests/colneq.test: Use -Wno-portability.
 
Index: automake.in
===================================================================
RCS file: /cvs/automake/automake/automake.in,v
retrieving revision 1.1338
diff -u -r1.1338 automake.in
--- automake.in 19 Aug 2002 22:48:38 -0000      1.1338
+++ automake.in 21 Aug 2002 19:34:00 -0000
@@ -156,7 +156,7 @@
 # Only recognize leading spaces, not leading tabs.  If we recognize
 # leading tabs here then we need to make the reader smarter, because
 # otherwise it will think rules like `foo=bar; \' are errors.
-my $MACRO_PATTERN = 'address@hidden' . "\$";
+my $MACRO_PATTERN = 'address@hidden' . "\$";
 my $ASSIGNMENT_PATTERN = '^ *([^ \t=:+]*)\s*([:+]?)=\s*(.*)' . "\$";
 # This pattern recognizes a Gnits version id and sets $1 if the
 # release is an alpha release.  We also allow a suffix which can be
@@ -1337,10 +1337,10 @@
 
 # var_SUFFIXES_trigger ($TYPE, $VALUE)
 # ------------------------------------
-# This is called automagically by define_macro() when SUFFIXES
+# This is called automagically by macro_define() when SUFFIXES
 # is defined ($TYPE eq '') or appended ($TYPE eq '+').
 # The work here needs to be performed as a side-effect of the
-# define_macro() call because SUFFIXES definitions impact
+# macro_define() call because SUFFIXES definitions impact
 # on $KNOWN_EXTENSIONS_PATTERN, and $KNOWN_EXTENSIONS_PATTERN
 # are used when parsing the input am file.
 sub var_SUFFIXES_trigger ($$)
@@ -1651,7 +1651,7 @@
     &handle_clean;
     &handle_factored_dependencies;
 
-    check_typos ();
+    check_variables ();
 
     if (! -d ($output_directory . '/' . $am_relative_dir))
     {
@@ -3394,15 +3394,17 @@
     }
 }
 
-# See if any _SOURCES variable were misspelled.  Also, make sure that
-# EXTRA_ variables don't contain configure substitutions.
-sub check_typos ()
+# Catch many errors related to variables.
+sub check_variables ()
 {
   # It is ok if the user sets this particular variable.
+  # Mark it as examined so we don't warn about it later.
   &examine_variable ('AM_LDFLAGS');
 
   foreach my $varname (keys %var_value)
     {
+      # See if any _SOURCES variable (or similar) was defined
+      # but not examined.  This often indicates a typo.
       foreach my $primary ('_SOURCES', '_LIBADD', '_LDADD', '_LDFLAGS',
                           '_DEPENDENCIES')
        {
@@ -3411,6 +3413,26 @@
            if ($varname =~ /$primary$/ && ! $content_seen{$varname}
                && ! exists $configure_vars{$varname});
        }
+
+      # NEWS-OS 4.2R complains if a Makefile variable begins with `_'.
+      msg_var ('portability', $varname,
+          "$varname: variable names starting with `_' are not portable")
+       if $varname =~ /^_/;
+
+      # Catch uses of variables whose name does not conform to POSIX.
+      foreach my $subvar (subvariables_of ($varname))
+       {
+         if ($subvar !~ /^[.a-zA-Z0-9_]*$/)
+           {
+             # If the variable name contains a space, it's likely
+             # to be a GNU make extension (such as $(addsuffix ...)).
+             # Mention this in the diagnostic.
+             my $gnuext = "";
+             $gnuext = "\n(probably a GNU make extension)" if $subvar =~ / /;
+             msg_var ('portability', $varname,
+                      "$subvar: non-POSIX variable name$gnuext");
+           }
+       }
     }
 }
 
@@ -6156,11 +6178,6 @@
   err $where, "bad characters in variable name `$var'"
     if $var !~ /$MACRO_PATTERN/o;
 
-  # NEWS-OS 4.2R complains if a Makefile variable begins with `_'.
-  msg ('portability', $where,
-       "$var: variable names starting with `_' are not portable")
-    if $var =~ /^_/;
-
   $cond ||= 'TRUE';
 
   # An Automake variable must be consistently defined with the same
@@ -6565,7 +6582,51 @@
     return 0;
 }
 
+# @LIST
+# &subvariables_in ($TEXT)
+# ------------------------
+# Return the list of subvariable names occuring in $TEXT.
+# Note that unlike some other functions, $TEXT is not split
+# on spaces before we check for subvariables.
+sub subvariables_in ($)
+{
+  my ($text) = @_;
+  my @result = ();
+
+  # Strip comments.
+  $text =~ s/#.*$//;
+
+  # Record each use of ${stuff} or $(stuff) that do not follow a $.
+  while ($text =~ /(?<!\$)\$(?:\{([^\}]*)\}|\(([^\)]*)\))/g)
+    {
+      my $var = $1 || $2;
+      # The occurent may look like $(string1[:subst1=[subst2]]) but
+      # we want only `string1'.
+      $var =~ s/:[^:=]*=[^=]*$//;
+      push @result, $var;
+    }
+
+  return @result;
+}
+
+# @LIST
+# &subvariables_of ($var)
+# -----------------------
+# Return the list of subvariables of $VAR, for all conditions.
+# Note that unlike some other functions, the value of $VAR is not split
+# on spaces before we check for subvariables.
+sub subvariables_of ($)
+{
+  my ($var) = @_;
+  my @result = ();
+
+  foreach my $cond (keys %{$var_value{$var}})
+    {
+      push @result, subvariables_in $var_value{$var}{$cond};
+    }
 
+  return @result;
+}
 
 # &variable_conditions_recursive_sub ($VAR, $PARENT)
 # -------------------------------------------------------
@@ -6587,50 +6648,41 @@
     # Examine every condition under which $VAR is defined.
     foreach my $vcond (keys %{$var_value{$var}})
     {
-       push (@this_conds, $vcond);
+      push (@this_conds, $vcond);
 
-       # If $VAR references some other variable, then compute the
-       # conditions for that subvariable.
-       my @subvar_conds = ();
-       foreach (split (' ', $var_value{$var}{$vcond}))
+      # If $VAR references some other variable, then compute the
+      # conditions for that subvariable.
+      my @subvar_conds = ();
+      foreach my $varname (subvariables_in $var_value{$var}{$vcond})
        {
-           # If a comment seen, just leave.
-           last if /^#/;
-
-           # Handle variable substitutions.
-           if (/^\$\{(.*)\}$/ || /^\$\((.*)\)$/)
+         if ($varname =~ /$SUBST_REF_PATTERN/o)
            {
-               my $varname = $1;
-               if ($varname =~ /$SUBST_REF_PATTERN/o)
-               {
-                   $varname = $1;
-               }
-
+             $varname = $1;
+           }
 
-               # Here we compute all the conditions under which the
-               # subvariable is defined.  Then we go through and add
-               # $VCOND to each.
-               my @svc = variable_conditions_recursive_sub ($varname, $var);
-               foreach my $item (@svc)
-               {
-                   my $val = conditional_string ($vcond, split (' ', $item));
-                   $val ||= 'TRUE';
-                   push (@subvar_conds, $val);
-               }
+         # Here we compute all the conditions under which the
+         # subvariable is defined.  Then we go through and add
+         # $VCOND to each.
+         my @svc = variable_conditions_recursive_sub ($varname, $var);
+         foreach my $item (@svc)
+           {
+             my $val = conditional_string ($vcond, split (' ', $item));
+             $val ||= 'TRUE';
+             push (@subvar_conds, $val);
            }
        }
 
-       # If there are no conditional subvariables, then we want to
-       # return this condition.  Otherwise, we want to return the
-       # permutations of the subvariables, taking into account the
-       # conditions of $VAR.
-       if (! @subvar_conds)
+      # If there are no conditional subvariables, then we want to
+      # return this condition.  Otherwise, we want to return the
+      # permutations of the subvariables, taking into account the
+      # conditions of $VAR.
+      if (! @subvar_conds)
        {
-           push (@new_conds, $vcond);
+         push (@new_conds, $vcond);
        }
-       else
+      else
        {
-           push (@new_conds, variable_conditions_reduce (@subvar_conds));
+         push (@new_conds, variable_conditions_reduce (@subvar_conds));
        }
     }
 
Index: tests/Makefile.am
===================================================================
RCS file: /cvs/automake/automake/tests/Makefile.am,v
retrieving revision 1.429
diff -u -r1.429 Makefile.am
--- tests/Makefile.am   19 Aug 2002 22:48:39 -0000      1.429
+++ tests/Makefile.am   21 Aug 2002 19:34:02 -0000
@@ -392,6 +392,7 @@
 unused.test \
 vars.test \
 vars2.test \
+vars3.test \
 vartar.test \
 version.test \
 version2.test \
Index: tests/colneq.test
===================================================================
RCS file: /cvs/automake/automake/tests/colneq.test,v
retrieving revision 1.2
diff -u -r1.2 colneq.test
--- tests/colneq.test   20 Oct 2001 11:17:16 -0000      1.2
+++ tests/colneq.test   21 Aug 2002 19:34:02 -0000
@@ -5,6 +5,7 @@
 . $srcdir/defs || exit 1
 
 cat > Makefile.am << 'END'
+AUTOMAKE_OPTIONS = -Wno-portability
 ICONS := $(wildcard *.xbm)
 data_DATA = $(ICONS)
 END
Index: tests/vars3.test
===================================================================
RCS file: tests/vars3.test
diff -N tests/vars3.test
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/vars3.test    21 Aug 2002 19:34:02 -0000
@@ -0,0 +1,46 @@
+#! /bin/sh
+
+# Check that Automake warns about variables containing spaces
+# and other non-POSIX characters.
+
+. $srcdir/defs || exit 1
+
+set -e
+
+cat >Makefile.am <<'EOF'
+A1 = $(shell echo *)
+B2 = $$(not an error)
+C3 = $$(this is)$${ok too}
+D4 = $(nextvariableisbad)$(addsuffix .a, $(A))
+E5 = "$(bad boy)"
+F6 = $(this:is= ok)
+G7 = ${three errors}${on this} $(long line)
+EOF
+
+$ACLOCAL
+# Make sure this warning is print in the `portability' category.
+$AUTOMAKE --warnings=no-error,none,portability 2>stderr
+cat stderr
+
+# Lines number are printed in error message.  We use them to grep errors.
+
+# No error expected for these lines.
+grep 2 stderr && exit 1
+grep 3 stderr && exit 1
+grep 6 stderr && exit 1
+
+# The other lines are bogus.
+grep 1 stderr
+grep 4 stderr
+grep 5 stderr
+grep 7 stderr
+
+# No check some individual values.
+grep 'shell echo' stderr
+grep 'nextvariableisbad' stderr && exit 1
+grep 'addsuffix' stderr
+grep 'bad boy' stderr
+grep 'ok' stderr && exit 1
+grep 'three errors' stderr
+grep 'on this' stderr
+grep 'long line' stderr

-- 
Alexandre Duret-Lutz