Re: Unset “zle_bracketed_paste” .zshrc

[Daniel Shahaf <d.s@daniel.shahaf.name>]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

Peter Stephenson wrote on Thu, 06 Feb 2020 14:09 +0000:
> On Thu, 2020-02-06 at 14:32 +0100, Mikael Magnusson wrote:
> > On 2/6/20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:  
> > > 
> > > Well, it's an option.  It would result in the following interface:
> > > to prevent $zle_bracketed_paste from being defined one will have to run
> > > «unset zle_bracketed_paste» if zsh/zle has been loaded, or «zmodload -Fa
> > > zsh/zle -p:zle_bracketed_paste» otherwise.  
> > I would assume that if someone knew to run that obscure zmodload
> > command, they would know they could just do this instead,
> > zmodload zsh/zle
> > unset zle_bracketed_paste  
> 
> No, this is entirely about autoload behaviour.  "zmodload -Fa" provides
> selective autoload out of the box, which in principle fixes the issue
> with no fundamentally new features, just appropriate module exports.
> 
> However, the main reason for doing this with zmodload would be to
> provide consistency with different types of module feature, and there's
> no real call for that as people are much more used to the parameter
> interface.  So in practice adding behaviour to "unset" is probably
> easier for everyone.
> 

I've got a WIP patch for this that I'd like to share.  It fixes the
xfail ('f'-status) test; however, I'm not sure everything in it's
correct, and there are a few outstanding todo's.  It's nowhere near
being ready to be committed; I'm only posting it now for 'release early
and often' reasons.

It's attached.

Cheers,

Daniel

> It would be neater to be more consistent about the way module features
> are provided, just on the basis that if it's in a module it should use
> the module interface, else why are we pretending it's a module at all
> rather than building it into the shell?  But that can go way down a very
> long list.
> 
> pws
> 


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 6328 bytes --]

diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index ef9148d7b..43265f676 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -1114,7 +1114,8 @@ scanpmmodules(UNUSED(HashTable ht), ScanFunc func, int flags)
 	}
     for (i = 0; i < realparamtab->hsize; i++)
 	for (hn = realparamtab->nodes[i]; hn; hn = hn->next) {
-	    if ((((Param) hn)->node.flags & PM_AUTOLOAD) &&
+	    int flags = ((Param) hn)->node.flags;
+	    if ((flags & PM_AUTOLOAD) && !(flags & PM_UNSET) &&
 		!linknodebystring(done, ((Param) hn)->u.str)) {
 		pm.node.nam = ((Param) hn)->u.str;
 		addlinknode(done, pm.node.nam);
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 8c0534708..6c88a9157 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -2239,6 +2239,8 @@ setup_(UNUSED(Module m))
     bpaste[0] = ztrdup("\033[?2004h");
     bpaste[1] = ztrdup("\033[?2004l");
     /* Intended to be global, no WARNCREATEGLOBAL check. */
+    /* ### TODO: Don't set it if it's already set */
+    /* ### TODO: Make it a module parameter to let it be preemptively unset in zshrc */
     assignaparam("zle_bracketed_paste", bpaste, 0);
 
     return 0;
diff --git a/Src/module.c b/Src/module.c
index f41b82f25..7ac93ee4b 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -1018,6 +1018,7 @@ runhookdef(Hookdef h, void *d)
  * requires that either there's no parameter already present,
  * or it's a global parameter marked for autoloading.
  *
+ * Return status is zero on success, non-zero on failure.
  * The special status 2 is to indicate it didn't work but
  * -i was in use so we didn't print a warning.
  */
@@ -1030,7 +1031,8 @@ checkaddparam(const char *nam, int opt_i)
     if (!(pm = (Param) gethashnode2(paramtab, nam)))
 	return 0;
 
-    if (pm->level || !(pm->node.flags & PM_AUTOLOAD)) {
+    if (pm->level
+	    || (pm->node.flags & (PM_AUTOLOAD|PM_UNSET)) != PM_AUTOLOAD) {
 	/*
 	 * -i suppresses "it's already that way" warnings,
 	 * but not "this can't possibly work" warnings, so we print
@@ -1056,12 +1058,20 @@ checkaddparam(const char *nam, int opt_i)
 /* This adds the given parameter definition. The return value is zero on *
  * success and 1 on failure. */
 
-/**/
-int
+static int
 addparamdef(Paramdef d)
 {
     Param pm;
 
+    /* If there's already a "tombstone", honour it and don't add the parameter. */
+    /* ### TODO: verify that a tombstone can be removed by the user if desired */
+    {
+	Param pm2 = (Param) gethashnode2(paramtab, d->name);
+	if (pm2 && pm2->node.flags & PM_AUTOLOAD && pm2->node.flags & PM_UNSET) {
+	    return 0;
+	}
+    }
+
     if (checkaddparam(d->name, 0))
 	return 1;
 
@@ -1199,6 +1209,16 @@ add_autoparam(const char *module, const char *pnam, int flags)
     Param pm;
     int ret;
 
+    /* If there's already a "tombstone" parameter, we simply need to
+     * revive it. */
+    {
+	Param pm2 = (Param) gethashnode2(paramtab, pnam);
+	if (pm2 && pm2->node.flags & PM_AUTOLOAD && pm2->node.flags & PM_UNSET) {
+	    pm2->node.flags &= ~PM_UNSET;
+	    return 0;
+	}
+    }
+
     queue_signals();
     if ((ret = checkaddparam(pnam, (flags & FEAT_IGNORE)))) {
 	unqueue_signals();
diff --git a/Src/params.c b/Src/params.c
index 863b32600..9d4548720 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -527,7 +527,7 @@ getparamnode(HashTable ht, const char *nam)
 	(void)ensurefeature(mn, "p:", (pm->node.flags & PM_AUTOALL) ? NULL :
 			    nam);
 	hn = gethashnode2(ht, nam);
-	if (!hn) {
+	if (!hn || (pm->node.flags & PM_UNSET)) {
 	    /*
 	     * This used to be a warning, but surely if we allow
 	     * stuff to go ahead with the autoload stub with
@@ -3604,6 +3604,7 @@ unsetparam_pm(Param pm, int altflag, int exp)
 {
     Param oldpm, altpm;
     char *altremove;
+    int node_removed = 0; /* boolean flag */
 
     if ((pm->node.flags & PM_READONLY) && pm->level <= locallevel) {
 	zerr("read-only variable: %s", pm->node.nam);
@@ -3670,8 +3671,21 @@ unsetparam_pm(Param pm, int altflag, int exp)
 	(pm->node.flags & (PM_SPECIAL|PM_REMOVABLE)) == PM_SPECIAL)
 	return 0;
 
-    /* remove parameter node from table */
-    paramtab->removenode(paramtab, pm->node.nam);
+    /* 
+     * Remove parameter node from table.
+     * 
+     * For autoloaded parameters, we leave the Param behind with
+     * the PM_UNSET flag on, as a "tombstone" so zmodload won't
+     * try to create this parameter later.  Cf. add_autoparam()
+     * and getparamnode().
+     */
+    if (pm->node.flags & PM_AUTOLOAD)
+	/* ### TODO: audit all uses of PM_AUTOLOAD to see if they need to handle tombstones */
+	pm->node.flags |= PM_UNSET;
+    else {
+	paramtab->removenode(paramtab, pm->node.nam);
+	node_removed = 1;
+    }
 
     if (pm->old) {
 	oldpm = pm->old;
@@ -3692,7 +3706,8 @@ unsetparam_pm(Param pm, int altflag, int exp)
 	}
     }
 
-    paramtab->freenode(&pm->node); /* free parameter node */
+    if (node_removed)
+	paramtab->freenode(&pm->node); /* free parameter node */
 
     return 0;
 }
diff --git a/Test/V01zmodload.ztst b/Test/V01zmodload.ztst
index 0a7fbb651..077a252dc 100644
--- a/Test/V01zmodload.ztst
+++ b/Test/V01zmodload.ztst
@@ -353,20 +353,19 @@
    if zmodload -e zsh/parameter; then zmodload -u zsh/parameter; fi
    unset options
    zmodload zsh/parameter
-   echo \$+options
+   echo \$+options +\$options+
  "
--f:can unset a non-readonly autoloadable parameter before loading the module
->0
-# Currently prints '1'.
+0:can unset a non-readonly autoloadable parameter before loading the module
+>0 ++
 
  $ZTST_testdir/../Src/zsh -fc "
    MODULE_PATH=${(q)MODULE_PATH}
    zmodload zsh/parameter
    unset options
-   echo \$+options
+   echo \$+options +\$options+
  "
 0:can unset a non-readonly autoloadable parameter after loading the module
->0
+>0 ++
 
  $ZTST_testdir/../Src/zsh -fc "
    MODULE_PATH=${(q)MODULE_PATH}
@@ -375,7 +374,10 @@
  "
 -f:can't unset a readonly autoloadable parameter before loading the module
 *?zsh:?: read-only variable: builtins
-# Currently, the 'unset' succeeds.
+# Currently, the 'unset' succeeds.  For it to fail, the code would need to
+# know that $builtins is read-only.  However, that information only becomes
+# available after features_() is called, which happens after loading the
+# module.
 
  $ZTST_testdir/../Src/zsh -fc "
    MODULE_PATH=${(q)MODULE_PATH}