_make-expandVars() broken? (was Re: Makefile completion with wildcard targets)

_make-expandVars() broken? (was Re: Makefile completion with wildcard targets)

[Jun T.]

Continued form zsh-users (20348, 20349, 20352).

Currently _make recognizes the target by the the following
pattern (line 87 of _make):

[[:alnum:]][^$TAB:=]#:[^=]*

i.e., the first character must be an alnum. This means if a target
start with a meta character, for example,

*.c: dependencies

then *.c is not recognized as a target. If we are going to add -Q
to compadd, then it would be better to use a pattern something like

[[*?[:alnum:]$][^$TAB:=%]#:[^=]*

But when I tried this pattern I noticed that the function
_make-expandVars() was not working at all.

# I've looked into the function but couldn't understand what
# the variables $front, $ret and $tmp were supposed to do.

I think this function must be fixed irrespective of what pattern
to use at line 87, and add -Q to compadd or not.

I tried to rewrite the function as in the patch below.
It seems to work at least with the zsh's top-level Makefile
and several of my own small Makefiles, but may be far from
complete. Please improve if someone has a spare time.

In _make-expandVars() below, the input is divided into $front$rest,
where in $front all the make variables have already been replaced
with their values, while in $rest there remains one or more variables. 

I also changed the $TARGETS from an associative to an ordinary
array, because only its keys were used.


diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index c14a34c..64c91cd 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -4,58 +4,57 @@
 # are used in those targets and their dependencies.
 
 _make-expandVars() {
-  local open close var val front ret tmp=$1
+  local open close var val front='' rest=$1
 
-  front=${tmp%%\$*}
-  case $tmp in
-    (\(*) # Variable of the form $(foobar)
-    open='('
-    close=')'
-    ;;
-
-    ({*) # ${foobar}
-    open='{'
-    close='}'
-    ;;
-
-    ([[:alpha:]]*) # $foobar. This is exactly $(f)oobar.
-    open=''
-    close=''
-    var=${(s::)var[1]}
-    ;;
-
-    (\$*) # Escaped $.
-    print -- "${front}\$$(_make-expandVars ${tmp#\$})"
-    return
-    ;;
-
-    (*) # Nothing left to substitute.
-    print -- $tmp
-    return
-    ;;
-  esac
+  while [[ $rest = (#b)[^$]#($)* ]]; do
+    front=$front${rest[1,$mbegin[1]-1]}
+    rest=${rest[$mbegin[1],-1]}
 
-  if [[ -n $open ]]
-  then
-    var=${tmp#$open}
-    var=${var%%$close*}
-  fi
+    case $rest[2] in
+      ($)	    # '$$'. may not appear in target and variable's value
+	front=$front\$	    # or $front\$\$ ?
+	rest=${rest[3,-1]}
+	continue
+	;;
+      (\()	    # Variable of the form $(foobar)
+	open='('
+	close=')'
+	;;
+      ({)	    # ${foobar}
+	open='{'
+	close='}'
+	;;
+      ([[:alpha:]]) # $foobar. This is exactly $(f)oobar.
+	open=''
+	close=''
+	var=$rest[2]
+	;;
+      (*)	    # bad parameter name
+	print -- $front$rest
+	return
+	;;
+    esac
 
-  case $var in
-    ([[:alnum:]_]#)
-    val=${VARIABLES[$var]}
-    ret=${ret//\$$open$var$close/$val}
-    ;;
+    if [[ -n $open ]]; then
+      if [[ $rest = \$$open(#b)([[:alnum:]_]##)(#B)$close* ]]; then
+	var=$match
+      else  # unmatched () or {}, or bad parameter name
+	print -- $front$rest
+	return
+      fi
+    fi
 
-    (*)
-    # Improper variable name. No replacement.
-    # I'm not sure if this is desired behavior.
-    front+="\$$open$var$close"
-    ret=${ret/\$$open$var$close/}
-    ;;
-  esac
+    if [[ -n ${VARIABLES[(i)$var]} ]]; then
+      rest=${rest//\$$open$var$close/${VARIABLES[$var]}}
+    else
+      # if $var is not defined, just leave it as is.
+      # (or better to replace by a null string?)
+      front=$front\$$open$var$close
+      rest=${rest#\$$open$var$close}
+    fi
+  done
 
-  print -- "${front}$(_make-expandVars ${ret})"
+  print -- ${front}${rest}
 }
 
 _make-parseMakefile () {
@@ -84,16 +83,9 @@ _make-parseMakefile () {
 
       # TARGET: dependencies
       # TARGET1 TARGET2 TARGET3: dependencies
-      ([[:alnum:]][^$TAB:=]#:[^=]*)
-      input=$(_make-expandVars $input)
-      target=${input%%:*}
-      dep=${input#*:}
-      dep=${(z)dep}
-      dep="$dep"
-      for tmp in ${(z)target}
-      do
-        TARGETS[$tmp]=$dep
-      done
+      ([[*?[:alnum:]$][^$TAB:=%]#:[^=]*)
+      target=$(_make-expandVars ${input%%:*})
+      TARGETS+=( ${(z)target} )
       ;;
 
       # Include another makefile
@@ -150,7 +142,8 @@ _make() {
   local prev="$words[CURRENT-1]" file expl tmp is_gnu dir incl match
   local context state state_descr line
   local -a option_specs
-  local -A TARGETS VARIABLES opt_args
+  local -A VARIABLES opt_args
+  local -aU TARGETS
   local ret=1
 
   _pick_variant -r is_gnu gnu=GNU unix -v -f
@@ -275,7 +268,7 @@ _make() {
       while _tags
       do
         _requested targets expl 'make targets' \
-          compadd -- ${(k)TARGETS} && ret=0
+          compadd -Q -- ${TARGETS} && ret=0
         _requested variables expl 'make variables' \
           compadd -S '=' -- ${(k)VARIABLES} && ret=0
       done

Re: _make-expandVars() broken? (was Re: Makefile completion with wildcard targets)

[Bart Schaefer]

On Jul 28, 10:20pm, Jun T. wrote:
}
} I think this function must be fixed irrespective of what pattern
} to use at line 87, and add -Q to compadd or not.

I agree.  I see you've replaced recursive calls with a loop, that's
probably a good thing.

Re: _make-expandVars() broken? (was Re: Makefile completion with wildcard targets)

[Jun T.]

Here is a slightly updated _make.

(1)(2)(3) were already in the previous patch.
(4)(5) are new.

(1) fix the broken make-expandVars()
(2) allow * ? and $ as the first character of the target.
(3) add an option -Q to compadd for targets.

(4) If 'VAR=VAL' is on the current command line,

% make VAR=VAL <TAB>

then use it in make-expandVars(). Also do not offer VAR for
completion if VAR=VAL is already on the command line (compadd -F).

(5) If the variable is not defined in Makefile nor on the command
line, then look it up in the environment.
The option --environment-overrides is respected.

It doesn't support $(SRCS:.c=.o) etc., but if a user need these
then he/she can set call-command style to true to use the
output from 'make -nsp'.

Any comment?

# The reason that make-expandVars() has been broken for a long time
# may be that most of the users are setting call-command style?


diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index c14a34c..48befa7 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -4,58 +4,68 @@
 # are used in those targets and their dependencies.
 
 _make-expandVars() {
-  local open close var val front ret tmp=$1
+  local open close var val front='' rest=$1
 
-  front=${tmp%%\$*}
-  case $tmp in
-    (\(*) # Variable of the form $(foobar)
-    open='('
-    close=')'
-    ;;
-
-    ({*) # ${foobar}
-    open='{'
-    close='}'
-    ;;
-
-    ([[:alpha:]]*) # $foobar. This is exactly $(f)oobar.
-    open=''
-    close=''
-    var=${(s::)var[1]}
-    ;;
-
-    (\$*) # Escaped $.
-    print -- "${front}\$$(_make-expandVars ${tmp#\$})"
-    return
-    ;;
+  while [[ $rest == (#b)[^$]#($)* ]]; do
+    front=$front${rest[1,$mbegin[1]-1]}
+    rest=${rest[$mbegin[1],-1]}
 
-    (*) # Nothing left to substitute.
-    print -- $tmp
-    return
-    ;;
-  esac
-
-  if [[ -n $open ]]
-  then
-    var=${tmp#$open}
-    var=${var%%$close*}
-  fi
+    case $rest[2] in
+      ($)	    # '$$'. may not appear in target and variable's value
+	front=$front\$\$
+	rest=${rest[3,-1]}
+	continue
+	;;
+      (\()	    # Variable of the form $(foobar)
+	open='('
+	close=')'
+	;;
+      ({)	    # ${foobar}
+	open='{'
+	close='}'
+	;;
+      ([[:alpha:]]) # $foobar. This is exactly $(f)oobar.
+	open=''
+	close=''
+	var=$rest[2]
+	;;
+      (*)	    # bad parameter name
+	print -- $front$rest
+	return 1
+	;;
+    esac
 
-  case $var in
-    ([[:alnum:]_]#)
-    val=${VARIABLES[$var]}
-    ret=${ret//\$$open$var$close/$val}
-    ;;
+    if [[ -n $open ]]; then
+      if [[ $rest == \$$open(#b)([[:alnum:]_]##)(#B)$close* ]]; then
+	var=$match
+      else  # unmatched () or {}, or bad parameter name
+	print -- $front$rest
+	return 1
+      fi
+    fi
 
-    (*)
-    # Improper variable name. No replacement.
-    # I'm not sure if this is desired behavior.
-    front+="\$$open$var$close"
-    ret=${ret/\$$open$var$close/}
-    ;;
-  esac
+    val=''
+    if [[ -n ${VAR_ARGS[(i)$var]} ]]; then
+      val=${VAR_ARGS[$var]}
+    else
+      if [[ -n $opt_args[(I)(-e|--environment-overrides)] ]]; then
+	if [[ $parameters[$var] == scalar-export* ]]; then
+	  val=${(P)var}
+	elif [[ -n ${VARIABLES[(i)$var]} ]]; then
+	  val=${VARIABLES[$var]}
+	fi
+      else
+	if [[ -n ${VARIABLES[(i)$var]} ]]; then
+	  val=${VARIABLES[$var]}
+	elif [[ $parameters[$var] == scalar-export* ]]; then
+	  val=${(P)var}
+	fi
+      fi
+    fi
+    rest=${rest//\$$open$var$close/$val}
+  done
 
-  print -- "${front}$(_make-expandVars ${ret})"
+  print -- ${front}${rest}
 }
 
 _make-parseMakefile () {
@@ -84,16 +94,9 @@ _make-parseMakefile () {
 
       # TARGET: dependencies
       # TARGET1 TARGET2 TARGET3: dependencies
-      ([[:alnum:]][^$TAB:=]#:[^=]*)
-      input=$(_make-expandVars $input)
-      target=${input%%:*}
-      dep=${input#*:}
-      dep=${(z)dep}
-      dep="$dep"
-      for tmp in ${(z)target}
-      do
-        TARGETS[$tmp]=$dep
-      done
+      ([[*?[:alnum:]$][^$TAB:=%]#:[^=]*)
+      target=$(_make-expandVars ${input%%:*})
+      TARGETS+=( ${(z)target} )
       ;;
 
       # Include another makefile
@@ -150,9 +153,18 @@ _make() {
   local prev="$words[CURRENT-1]" file expl tmp is_gnu dir incl match
   local context state state_descr line
   local -a option_specs
-  local -A TARGETS VARIABLES opt_args
+  local -A VARIABLES VAR_ARGS opt_args
+  local -aU TARGETS keys
   local ret=1
 
+  # VAR=VAL on the current command line
+  for tmp in $words; do
+    if [[ $tmp == (#b)([[:alnum:]_]##)=(*) ]]; then
+      VAR_ARGS[${tmp[$mbegin[1],$mend[1]]}]=${(e)tmp[$mbegin[2],$mend[2]]}
+    fi
+  done
+  keys=( ${(k)VAR_ARGS} ) # to be used in 'compadd -F keys'
+
   _pick_variant -r is_gnu gnu=GNU unix -v -f
 
   if [[ $is_gnu == gnu ]]
@@ -275,9 +287,9 @@ _make() {
       while _tags
       do
         _requested targets expl 'make targets' \
-          compadd -- ${(k)TARGETS} && ret=0
+          compadd -Q -- $TARGETS && ret=0
         _requested variables expl 'make variables' \
-          compadd -S '=' -- ${(k)VARIABLES} && ret=0
+          compadd -S '=' -F keys -- ${(k)VARIABLES} && ret=0
       done
     fi
   esac

Re: _make-expandVars() broken? (was Re: Makefile completion with wildcard targets)

[Bart Schaefer]

On Jul 31,  1:26am, Jun T. wrote:
}
} Any comment?

This all sounds good to me, thanks!

} # The reason that make-expandVars() has been broken for a long time
} # may be that most of the users are setting call-command style?

Or just not completing targets that depend on variable expansions, I
suppose.