$1 inside textual local variable [was: [RFC] improve autotest syntax checks]

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> According to Eric Blake on 3/11/2008 7:07 PM:
> | The problem is that m4wrap stores the $1 literally, leaving it for the
> | definition of foo, but _m4_wrap_text is m4_define'd to contain a $1 then
> | expanded later, meaning the $1 is expanded as an (empty) argument and foo
> | is defined to the empty string.

I found a couple more places impacted by this.  In general, the idiom of 
overquoting the contents of a local variable used to store text, so that you 
can just invoke that variable rather than using defn to get at its contents, 
will mistakenly swallow anything that looks like a parameter expansion, like $1.

m4_combine is new to 2.62, so it might as well handle everything.

m4_text_wrap has always had problems with this, but as we documented that it is 
improved in 2.62, we might as well improve it all the way.

$ m4 -I ../autoconf-2.61/lib/ m4sugar/m4sugar.m4 -
m4_divert(0)m4_text_wrap([some $1 $2 long text], [-$*- ], address@hidden ], 13)
-- some $1
-- $2 long
-- text

$ m4 -I lib/ m4sugar/m4sugar.m4 -
m4_divert(0)m4_text_wrap([some $1 $2 long text], [-$*- ], address@hidden ], 13)
address@hidden some $1
-$*- $2 long
-$*- text

m4_text_wrap also had a problem with arithmetic precedence (for example, the 
expression [0xfff & 21] should wrap at column 21, not column 4095, nor expand 
[xfff] as a macro).  And it doesn't hurt that with my patch, m4_text_wrap now 
works in fewer macro expansions.

Other places used the idiom of overquoting then invoking a local variable; but 
are immune to the problem since the text that they hold will never look like a 
parameter (m4_text_box in m4sugar.m4, AT_CHECK_AT_PREP in autotest.at).

From: Eric Blake <address@hidden>
Date: Fri, 21 Mar 2008 11:07:08 -0600
Subject: [PATCH] Don't swallow $1 in textual local variables.

* lib/m4sugar/m4sugar.m4 (m4_combine): Don't use overquoting and
expansion of text arguments, as that swallows $1.
(m4_text_wrap): Likewise, by splitting out...
(_m4_text_wrap): ...new helper macro.  Also, allow arbitrary
expression for width.
* tests/m4sugar.at (m4@&address@hidden): Test this.
(m4@&address@hidden): Likewise.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog              |   11 ++++++++++
 lib/m4sugar/m4sugar.m4 |   50 +++++++++++++++++++++++------------------------
 tests/m4sugar.at       |   13 ++++++++++-
 3 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5dac387..59b39a5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2008-03-21  Eric Blake  <address@hidden>
+
+       Don't swallow $1 in textual local variables.
+       * lib/m4sugar/m4sugar.m4 (m4_combine): Don't use overquoting and
+       expansion of text arguments, as that swallows $1.
+       (m4_text_wrap): Likewise, by splitting out...
+       (_m4_text_wrap): ...new helper macro.  Also, allow arbitrary
+       expression for width.
+       * tests/m4sugar.at (m4@&address@hidden): Test this.
+       (m4@&address@hidden): Likewise.
+
 2008-03-20  Eric Blake  <address@hidden>
 
        Kill more CVS references.
diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4
index e755f98..2a780a7 100644
--- a/lib/m4sugar/m4sugar.m4
+++ b/lib/m4sugar/m4sugar.m4
@@ -1852,19 +1852,19 @@ m4_define([_m4_join],
 #   => a-1, a-2, a-3, b-1, b-2, b-3, c-1, c-2, c-3
 #
 # In order to have the correct number of SEPARATORs, we use a temporary
-# variable that redefines itself after the first use.  Note that since there
-# is no user expansion, we can avoid m4_defn overhead by overquoting the
-# second definition of m4_Separator, and by using m4_builtin.  Likewise,
-# we compute the m4_shift3 only once, rather than in each iteration of the
-# outer m4_foreach.
+# variable that redefines itself after the first use.  We use m4_builtin
+# to avoid m4_defn overhead, but must use defn rather than overquoting
+# in case PREFIX or SUFFIX contains $1.  Likewise, we compute the m4_shift3
+# only once, rather than in each iteration of the outer m4_foreach.
 m4_define([m4_combine],
 [m4_if(m4_eval([$# > 3]), [1],
-       [m4_pushdef([m4_Separator], [m4_define([m4_Separator], [[$1]])])]]dnl
+       [m4_pushdef([m4_Separator], [m4_define([m4_Separator],
+                                   m4_builtin([defn], [m4_echo]))])]]dnl
 [[m4_foreach([m4_Prefix], [$2],
             [m4_foreach([m4_Suffix], ]m4_dquote(m4_dquote(m4_shift3($@)))[,
-                        [m4_Separator[]m4_builtin([defn],
-                                       [m4_Prefix])[$3]m4_builtin([defn],
-                                                       [m4_Suffix])])])]]dnl
+                        [m4_Separator([$1])[]m4_builtin([defn],
+                                     [m4_Prefix])[$3]m4_builtin([defn],
+                                                     [m4_Suffix])])])]]dnl
 [[m4_builtin([popdef], [m4_Separator])])])
 
 
@@ -1988,27 +1988,28 @@ m4_define([m4_append_uniq_w],
 # all the words are preceded by m4_Separator which is defined to empty for
 # the first word, and then ` ' (single space) for all the others.
 #
-# The algorithm overquotes m4_Prefix and m4_Prefix1 to avoid m4_defn
-# overhead, and bypasses m4_popdef overhead with m4_builtin since no user
+# The algorithm uses a helper that uses $2 through $4 directly, rather than
+# using local variables, to avoid m4_defn overhead, or expansion swallowing
+# any $.  It also bypasses m4_popdef overhead with m4_builtin since no user
 # macro expansion occurs in the meantime.  Also, the definition is written
 # with m4_do, to avoid time wasted on dnl during expansion (since this is
 # already a time-consuming macro).
 m4_define([m4_text_wrap],
+[_$0([$1], [$2], m4_if([$3], [], [[$2]], [[$3]]),
+     m4_if([$4], [], [79], [[$4]]))])
+m4_define([_m4_text_wrap],
 m4_do(dnl set up local variables, to avoid repeated calculations
-[[m4_pushdef([m4_Prefix], [[$2]])]],
-[[m4_pushdef([m4_Prefix1], m4_if([$3], [], [m4_Prefix], [[[$3]]]))]],
-[[m4_pushdef([m4_Width], m4_default([$4], 79))]],
-[[m4_pushdef([m4_Indent], m4_qlen(m4_Prefix))]],
-[[m4_pushdef([m4_Cursor], m4_qlen(m4_Prefix1))]],
+[[m4_pushdef([m4_Indent], m4_qlen([$2]))]],
+[[m4_pushdef([m4_Cursor], m4_qlen([$3]))]],
 [[m4_pushdef([m4_Separator], [m4_define([m4_Separator], [ ])])]],
 dnl expand the first prefix, then check its length vs. regular prefix
 dnl same length: nothing special
 dnl prefix1 longer: output on line by itself, and reset cursor
 dnl prefix1 shorter: pad to length of prefix, and reset cursor
-[[m4_Prefix1[]m4_cond([m4_Cursor], m4_Indent, [],
-                     [m4_eval(m4_Cursor > m4_Indent)], [1], [
-m4_Prefix[]m4_define([m4_Cursor], m4_Indent)],
-                     [m4_format([%*s], m4_max([0],
+[[[$3]m4_cond([m4_Cursor], m4_Indent, [],
+             [m4_eval(m4_Cursor > m4_Indent)], [1], [
+[$2]m4_define([m4_Cursor], m4_Indent)],
+             [m4_format([%*s], m4_max([0],
   m4_eval(m4_Indent - m4_Cursor)), [])m4_define([m4_Cursor], m4_Indent)])]],
 dnl now, for each word, compute the curser after the word is output, then
 dnl check if the cursor would exceed the wrap column
@@ -2018,19 +2019,16 @@ dnl either way, insert the word
 [[m4_foreach_w([m4_Word], [$1],
   [m4_define([m4_Cursor],
             m4_eval(m4_Cursor + m4_qlen(m4_builtin([defn], [m4_Word]))
-                    + 1))m4_if(m4_eval(m4_Cursor > m4_Width),
+                    + 1))m4_if(m4_eval(m4_Cursor > ([$4])),
       [1], [m4_define([m4_Cursor],
                      m4_eval(m4_Indent
                              + m4_qlen(m4_builtin([defn], [m4_Word])) + 1))
-m4_Prefix[]],
+[$2]],
       [m4_Separator[]])m4_builtin([defn], [m4_Word])])]],
 dnl finally, clean up the local variabls
 [[m4_builtin([popdef], [m4_Separator])]],
 [[m4_builtin([popdef], [m4_Cursor])]],
-[[m4_builtin([popdef], [m4_Indent])]],
-[[m4_builtin([popdef], [m4_Width])]],
-[[m4_builtin([popdef], [m4_Prefix1])]],
-[[m4_builtin([popdef], [m4_Prefix])]]))
+[[m4_builtin([popdef], [m4_Indent])]]))
 
 
 # m4_text_box(MESSAGE, [FRAME-CHARACTER = `-'])
diff --git a/tests/m4sugar.at b/tests/m4sugar.at
index 163dea1..244c5f6 100644
--- a/tests/m4sugar.at
+++ b/tests/m4sugar.at
@@ -2,7 +2,8 @@
 
 AT_BANNER([M4sugar.])
 
-# Copyright (C) 2000, 2001, 2002, 2005, 2006, 2007 Free Software Foundation, 
Inc.
+# Copyright (C) 2000, 2001, 2002, 2005, 2006, 2007, 2008 Free Software
+# Foundation, Inc.
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -312,7 +313,7 @@ AT_CLEANUP
 AT_SETUP([m4@&address@hidden)
 
 # m4_text_wrap is used to display the help strings.  Also, check that
-# commas are not swallowed.  This can easily happen because of
+# commas and $ are not swallowed.  This can easily happen because of
 # m4-listification.
 
 AT_DATA_M4SUGAR([script.4s],
@@ -328,6 +329,8 @@ m4_text_wrap([Short doc.], [          ], [  --too-wide], 30)
 m4_text_wrap([Super long documentation.], [          ], [  --too-wide], 30)
 
 m4_text_wrap([First, second  , third, [,quoted]])
+m4_define([xfff], [oops])
+m4_text_wrap([Some $1 $2 $3 $4 embedded dollars.], [ $* ], [ $@ ], [0xfff & 
20])
 ]])
 
 AT_DATA([expout],
@@ -346,6 +349,10 @@ AT_DATA([expout],
           documentation.
 
 First, second , third, [,quoted]
+
+ $@ Some $1 $2 $3
+ $* $4 embedded
+ $* dollars.
 ]])
 
 AT_CHECK_M4SUGAR([-o-], 0, [expout])
@@ -583,6 +590,7 @@ m4_combine([, ], [[a], [b]], [-], [])
 m4_combine([, ], [], [-], [a], [b])
 m4_combine([, ], [[]], [-], [a], [b])
 m4_combine([ a ], [[-], [+]], [a], [-], [+])
+m4_combine([$* ], [[$1], [$2]], [$#], address@hidden)
 ]],
 [[a-1, a-2, a-3, b-1, b-2, b-3, c-1, c-2, c-3
 
@@ -590,6 +598,7 @@ a-, b-
 
 -a, -b
 -a- a -a+ a +a- a +a+
address@hidden $2$#$@
 ]], [])
 
 AT_CLEANUP
-- 
1.5.4