Errors on module feature [auto]loading (Re: zsh 5.0.2-test-1 is available)

Errors on module feature [auto]loading (Re: zsh 5.0.2-test-1 is available)

[Bart Schaefer]

[> workers]

On Nov 9, 10:32pm, Peter Stephenson wrote:
}
} Suppose we load builtin bar from zsh/foo at some point, doesn't matter
} how, then run "zmodload -a bar zsh/foo". We find that a builtin bar
} exists. We can then check that zsh/foo is already loaded, and is
} already providing builtin bar.

And conversely we can tell that a builtin bar already exists that is not
provided by zsh/foo ?  This is the part that I thought for some reason
was difficult.

Where the analogy with autoloaded functions breaks down is that autoload
for functions cannot be told which file the function must come from.  It
searches $fpath and takes the first one it finds.  The autoload builtin
makes the assumption (rightly or wrongly) that $fpath is stable and that
if a function has already been loaded, it is the same one that would be
found if $fpath were searched again.

Loading of modules *themselves* with "zmodload zsh/foo" can make a similar
assumption about $module_path so the analogy holds there.

Explicit builtin autoloads with "zmodload -a zsh/foo bar" have no basis
for making that same assumption.  We've provided a way to say "bar must
come from zsh/foo, even if zsh/bar would be found first by a $module_path
search".  Therefore, if we *can't* guarantee that bar came from zsh/foo,
we should not suppress the error message.

} furthermore, given that you can check by this means the effect really
} *is* redundant rather than a conflict, it seems to me there is no
} harmful side effect of any sort.

I'm OK with this given the precondition.  Do all the rules about warning
on circular dependency, etc., apply?  If I try to explicitly autoload
from zsh/foo a feature that would implicitly be provided by zsh/bar,
when if ever would an error occur?

Aside:  Should we update the manual to say that the right way to test for
module existence is "zmodload -F module" (with no feature arguments)?  This
checks that the module could be loaded without enabling anything, whereas
"zmodload [-i] module" enables all the default features.

-- 
Barton E. Schaefer

Re: Errors on module feature [auto]loading (Re: zsh 5.0.2-test-1 is available)

[Peter Stephenson]

On Sun, 10 Nov 2013 12:03:54 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> [> workers]
> 
> On Nov 9, 10:32pm, Peter Stephenson wrote:
> }
> } Suppose we load builtin bar from zsh/foo at some point, doesn't matter
> } how, then run "zmodload -a bar zsh/foo". We find that a builtin bar
> } exists. We can then check that zsh/foo is already loaded, and is
> } already providing builtin bar.
> 
> And conversely we can tell that a builtin bar already exists that is not
> provided by zsh/foo ?  This is the part that I thought for some reason
> was difficult.

We can tell a builtin is loaded easily enough --- this is the test
that's currently reporting the error if the builtin is already provided
by anything at all.  The new trick is that we then look to see if
zsh/foo is loaded already, and if so whether it's providing that
builtin, which we can do e.g. by querying the features.  (That's
probably the best way without straining the standard module interface, I
think.)  If this says that b:bar is present and enabled, we know that
zsh/foo is providing it, because zsh/foo wouldn't have said it was
enabled otherwise, it would have reported an error instead.  If it isn't
enabled as a feature from zsh/foo, then something else is providing the
builtin; we don't care what, this is an error.

This is generic to any sort of feature, so exactly the same applies to
the other types of feature.

> I'm OK with this given the precondition.  Do all the rules about warning
> on circular dependency, etc., apply?  If I try to explicitly autoload
> from zsh/foo a feature that would implicitly be provided by zsh/bar,
> when if ever would an error occur?

Hmm... I don't know how this works at the moment, but it's not really
relevant because zsh/bar won't show the builtin as a feature --- unless
a dependent module is providing a conflicting builtin, but that's a
configuration problem, not a user problem.  The method doesn't care
about dependencies, it cares that you specified the right module, which
I think is reasonable --- the documentation tells you.
 
> Aside:  Should we update the manual to say that the right way to test for
> module existence is "zmodload -F module" (with no feature arguments)?  This
> checks that the module could be loaded without enabling anything, whereas
> "zmodload [-i] module" enables all the default features.

It doesn't just test for it, it does in fact load it, but without
enabling any of the features.  If you didn't want anything in the module
at that point you'd need to unload it again, but the combined operation
should be free of side effects.  That might be worth mentioning.

pws

PATCH: autoloading already loaded features

[Peter Stephenson]

On Sat, 9 Nov 2013 22:32:50 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> For autoloads from modules, which may be builtins, conditions, math
> functions or parameters, we can actually do a bit better, with a little
> more work but nothing like to the same extent (no additional loading is
> required).  Suppose we load builtin bar from zsh/foo at some point,
> doesn't matter how, then run "zmodload -a bar zsh/foo".  We find that a
> builtin bar exists.  We can then check that zsh/foo is already loaded,
> and is already providing builtin bar.

Gah.  This turns out to be completely trivial.  We already check if the
module is loaded, so we can ensure that it does in fact provide that
feature.  We just need to check at that point if it *is* providing that
feature.  Then we can be absolutely confident that simply telling the
user everything is OK is the right thing to do: they're already getting
the feature they've just asked for.  If it's not yet providing the
feature, marking it for autoload works unless there's a clash (it's a
little pointless since we could just enable it immediately but that's a
bit more work).

diff --git a/Src/module.c b/Src/module.c
index 5cc595c..c567b3c 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -3419,12 +3419,15 @@ autofeatures(const char *cmdnam, const char *module, char **features,
     int ret = 0, subret;
     Module defm, m;
     char **modfeatures = NULL;
+    int *modenables = NULL;
     if (module) {
 	defm = (Module)find_module(module,
 				   FINDMOD_ALIASP|FINDMOD_CREATE, NULL);
 	if ((defm->node.flags & MOD_LINKED) ? defm->u.linked :
-	    defm->u.handle)
+	    defm->u.handle) {
 	    (void)features_module(defm, &modfeatures);
+	    (void)enables_module(defm, &modenables);
+	}
     } else
 	defm = NULL;
 
@@ -3544,6 +3547,16 @@ autofeatures(const char *cmdnam, const char *module, char **features,
 		    ret = 1;
 		    continue;
 		}
+		/*
+		 * If the feature is already provided by the module, there's
+		 * nothing more to do.
+		 */
+		if (modenables[ptr-modfeatures])
+		    continue;
+		/*
+		 * Otherwise, marking it for autoload will do the
+		 * right thing when the feature is eventually usd.
+		 */
 	    }
 	    if (!m->autoloads) {
 		m->autoloads = znewlinklist();

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/