Re: preparation for expand-before-require warning

[Eric Blake]

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

> There are a couple 
> more problems in m4sh itself (for example, _AS_SHELL_SANITIZE expands 
> _AS_PATH_SEPARATOR_PREPARE then requires it via _AS_PATH_WALK), but I'm still 
> trying to come up with the cleanest fixes for those.

It turns out that the idiom:

m4_defun([a],[A])
m4_defun([b],[m4_require([a])B])
m4_defun([c],[a
b])

is rather common (autoconf, automake, and gnulib all triggered it, and gnulib 
in multiple places), but is also harmless (you can't get out-of-order expansion 
from an ac_require until you have _nested_ ac_require).  So I reworked my patch 
to recognize this case and avoid treating it as a false positive, which means I 
no longer have to worry about _AS_SHELL_SANITIZE doing a expand-before-require 
of _AS_PATH_SEPARATOR_PREPARE.  Instead, we can focus directly on the true 
expand-before-indirect-require cases where there really is out-of-order 
expansion (gnulib has some of those still, but not autoconf or automake).  
Here's the current state of my two patches; the second one implements Bruno's 
idea that any time we issue the expand-before-require message, we can also 
avoid the out-of-order expansion by merely doing a duplicate expansion.  Or, 
put another way, the canonical example:

m4_defun([a],[A])
m4_defun([b],[m4_require([a])B])
m4_defun([c],[m4_require([b])C])
m4_defun([d],[D
a
c])

now results in a warning (because a was expanded twice, where it earlier 
autoconf only expanded it once), and the following _corrected_ output:

A
B
D
A
C


>From 525797c1fc204bba2c83928b0e06239ea5c9d3ff Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 20 Jan 2009 14:03:59 -0700
Subject: [PATCH] Warn if macro is provided before indirectly required.

* lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros
provided since last outermost defun.
(_m4_defun_pro_outer): Empty the set.
(m4_require): Check the set, in order to warn.
(_m4_require_call): Distinguish between direct and indirect
requires.
* tests/m4sugar.at (m4@&address@hidden: nested): Remove xfail, and add
test case for direct requires.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog              |   12 ++++++++++
 lib/m4sugar/m4sugar.m4 |   14 ++++++++---
 tests/m4sugar.at       |   57 +++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6023598..c348e6b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,17 @@
 2009-01-20  Eric Blake  <address@hidden>
 
+       Warn if macro is provided before indirectly required.
+       * lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros
+       provided since last outermost defun.
+       (_m4_defun_pro_outer): Empty the set.
+       (m4_require): Check the set, in order to warn.
+       (_m4_require_call): Distinguish between direct and indirect
+       requires.
+       * tests/m4sugar.at (m4@&address@hidden: nested): Remove xfail, and add
+       test case for direct requires.
+
+2009-01-20  Eric Blake  <address@hidden>
+
        Clean up some bugs caught by preliminary dependency validation.
        * lib/autoconf/headers.m4 (AC_DIR_HEADER): Don't invoke
        AC_HEADER_DIRENT, since AC_FUNC_CLOSEDIR_VOID requires it.
diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4
index 3b28512..04ddc1b 100644
--- a/lib/m4sugar/m4sugar.m4
+++ b/lib/m4sugar/m4sugar.m4
@@ -1772,6 +1772,7 @@ m4_define([_m4_defun_pro],
 [m4_expansion_stack_push([$1])m4_pushdef([_m4_expanding($1)])])
 
 m4_define([_m4_defun_pro_outer],
+[m4_set_delete([_m4_provide])]dnl
 [m4_pushdef([_m4_divert_dump], m4_divnum)m4_divert_push([GROW])])
 
 # _m4_defun_epi(MACRO-NAME)
@@ -1935,8 +1936,11 @@ m4_define([m4_require],
 [m4_if(_m4_divert_dump, [],
   [m4_fatal([$0($1): cannot be used outside of an ]dnl
 m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]])['d macro])])]dnl
-[m4_provide_if([$1], [],
-              [_m4_require_call([$1], [$2], _m4_divert_dump)])])
+[m4_provide_if([$1], [m4_ifdef([_m4_require],
+  [m4_set_contains([_m4_provide], [$1],
+    [m4_warn([syntax], [$0: `$1' was expanded before it was required])])])],
+  [_m4_require_call([$1], [$2], _m4_divert_dump)m4_set_remove(
+    [_m4_provide], [$1])])])
 
 
 # _m4_require_call(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK],
@@ -1949,12 +1953,13 @@ m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]])
['d macro])])]dnl
 # by avoiding dnl and other overhead on the common path.
 m4_define([_m4_require_call],
 [m4_pushdef([_m4_divert_grow], m4_decr(_m4_divert_grow))]dnl
+[m4_pushdef([_m4_require])]dnl
 [m4_divert_push(_m4_divert_grow)]dnl
 [m4_if([$2], [], [$1], [$2])
 m4_provide_if([$1], [], [m4_warn([syntax],
   [$1 is m4_require'd but not m4_defun'd])])]dnl
 [_m4_divert_raw($3)_m4_undivert(_m4_divert_grow)]dnl
-[m4_divert_pop(_m4_divert_grow)_m4_popdef([_m4_divert_grow])])
+[m4_divert_pop(_m4_divert_grow)_m4_popdef([_m4_divert_grow], [_m4_require])])
 
 
 # _m4_divert_grow
@@ -1976,7 +1981,8 @@ m4_define([m4_expand_once],
 # m4_provide(MACRO-NAME)
 # ----------------------
 m4_define([m4_provide],
-[m4_define([m4_provide($1)])])
+[m4_ifdef([m4_provide($1)], [],
+[m4_set_add([_m4_provide], [$1], [m4_define([m4_provide($1)])])])])
 
 
 # m4_provide_if(MACRO-NAME, IF-PROVIDED, IF-NOT-PROVIDED)
diff --git a/tests/m4sugar.at b/tests/m4sugar.at
index e822ffb..a6739cf 100644
--- a/tests/m4sugar.at
+++ b/tests/m4sugar.at
@@ -507,7 +507,7 @@ d
 post
 ]])
 
-dnl Direct invocation, top level
+dnl Direct invocation, nested requires, top level
 AT_CHECK_M4SUGAR_TEXT([[dnl
 m4_defun([a], [[a]])dnl
 m4_defun([b], [[b]m4_require([a])])dnl
@@ -528,10 +528,10 @@ c
 post
 ]])
 
-dnl Direct invocation, nested.  This is an example of expansion before
-dnl requirement, such that b occurs before its prerequisite a.  This
-dnl indicates a bug in the macros (but not in autoconf), so we should
-dnl be emitting a warning.
+dnl Direct invocation, nested requires, nested defun.  This is an example
+dnl of expansion before requirement, such that b occurs before its
+dnl prerequisite a.  This indicates a bug in the macros (but not in
+dnl autoconf), so we should be emitting a warning.
 AT_CHECK_M4SUGAR_TEXT([[dnl
 m4_defun([a], [[a]])dnl
 m4_defun([b], [[b]m4_require([a])])dnl
@@ -552,9 +552,50 @@ c
 a
 c
 post
-]], [stderr])
-AT_XFAIL_IF([:])
-AT_CHECK([test -s stderr])
+]],
+[[script.4s:14: warning: m4@&address@hidden: `a' was expanded before it was 
required
+script.4s:5: b is expanded from...
+script.4s:6: c is expanded from...
+script.4s:7: outer is expanded from...
+script.4s:14: the top level
+]])
+
+dnl Direct invocation, expand-before-require but no nested require.  As this
+dnl is common in real life, but does not result in out-of-order expansion,
+dnl we silently permit this.
+AT_CHECK_M4SUGAR_TEXT([[dnl
+m4_defun([a], [[a]])dnl
+m4_defun([b], [[b]m4_require([a])])dnl
+m4_defun([c], [[c]])dnl
+m4_defun([d], [[d]m4_require([c])])dnl
+pre1
+a
+b
+a
+b
+post1
+m4_defun([outer],
+[pre2
+c
+d
+c
+d
+post2])dnl
+outer
+]],
+[[pre1
+a
+b
+a
+b
+post1
+pre2
+c
+d
+c
+d
+post2
+]])
 
 AT_CLEANUP
 
-- 
1.6.0.4


>From 08fbde77efa94c161aa91325fabeef50474e2bc9 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 20 Jan 2009 14:22:41 -0700
Subject: [PATCH] Fix out-of-order expansion with expand-before-require.

* lib/m4sugar/m4sugar.m4 (m4_require): Redundantly expand a
required macro when issuing expand-before-require warning.
* tests/m4sugar.at (m4@&address@hidden: nested): Adjust test.
Suggested by Bruno Haible.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog              |    6 ++++++
 lib/m4sugar/m4sugar.m4 |    8 ++++----
 tests/m4sugar.at       |    3 ++-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c348e6b..0d9549e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2009-01-20  Eric Blake  <address@hidden>
 
+       Fix out-of-order expansion with expand-before-require.
+       * lib/m4sugar/m4sugar.m4 (m4_require): Redundantly expand a
+       required macro when issuing expand-before-require warning.
+       * tests/m4sugar.at (m4@&address@hidden: nested): Adjust test.
+       Suggested by Bruno Haible.
+
        Warn if macro is provided before indirectly required.
        * lib/m4sugar/m4sugar.m4 (m4_provide): Track the set of all macros
        provided since last outermost defun.
diff --git a/lib/m4sugar/m4sugar.m4 b/lib/m4sugar/m4sugar.m4
index 04ddc1b..57b4e57 100644
--- a/lib/m4sugar/m4sugar.m4
+++ b/lib/m4sugar/m4sugar.m4
@@ -1937,10 +1937,10 @@ m4_define([m4_require],
   [m4_fatal([$0($1): cannot be used outside of an ]dnl
 m4_if([$0], [m4_require], [[m4_defun]], [[AC_DEFUN]])['d macro])])]dnl
 [m4_provide_if([$1], [m4_ifdef([_m4_require],
-  [m4_set_contains([_m4_provide], [$1],
-    [m4_warn([syntax], [$0: `$1' was expanded before it was required])])])],
-  [_m4_require_call([$1], [$2], _m4_divert_dump)m4_set_remove(
-    [_m4_provide], [$1])])])
+  [m4_set_contains([_m4_provide], [$1], [m4_warn([syntax],
+      [$0: `$1' was expanded before it was required])_m4_require_call],
+    [m4_ignore])], [m4_ignore])], [_m4_require_call])([$1], [$2],
+  _m4_divert_dump)m4_set_remove([_m4_provide], [$1])])
 
 
 # _m4_require_call(NAME-TO-CHECK, [BODY-TO-EXPAND = NAME-TO-CHECK],
diff --git a/tests/m4sugar.at b/tests/m4sugar.at
index a6739cf..613a3c9 100644
--- a/tests/m4sugar.at
+++ b/tests/m4sugar.at
@@ -545,7 +545,8 @@ c
 post])dnl
 outer
 ]],
-[[b
+[[a
+b
 pre
 a
 c
-- 
1.6.0.4