[PATCH 2 of 2] demandimport: delay loading for "from a import b" with absolute_import

[FUJIWARA Katsunori]

# HG changeset patch
# User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
# Date 1466270253 -32400
#      Sun Jun 19 02:17:33 2016 +0900
# Node ID 97bcb2d76d079903e95219fc40bb89d25b33079d
# Parent  04da1198949bcd8c1d1fd6675816eacfdc2f3dcf
demandimport: delay loading for "from a import b" with absolute_import

Before this patch, "from a import b" doesn't delay loading module "b",
if absolute_import is enabled, even though "from . import b" does.

For example:

  - it is assumed that extension X has "from P import M" for module M
    under package P with absolute_import feature

  - if importing module M is already delayed before loading extension
    X, loading module M in extension X is delayed until actually
    referring

    util, cmdutil, scmutil or so of Mercurial itself should be
    imported by "from . import M" style before loading extension X

  - otherwise, module M is loaded immediately at loading extension X,
    even if extension X itself isn't used at that "hg" command invocation

    Some minor modules (e.g. filemerge or so) of Mercurial itself
    aren't imported by "from . import M" style before loading
    extension X. And of course, external libraries aren't, too.

This might cause startup performance problem of hg command, because
many bundled extensions already enable absolute_import feature.

To delay loading module for "from a import b" with absolute_import
feature, this patch does below in "from a (or .a) import b" with
absolute_import case:

  1. import root module of "name" by system built-in __import__
     (referred as _origimport)

  2. recurse down the module chain for hierarchical "name"

     This logic can be shared with non absolute_import
     case. Therefore, this patch also centralizes it into chainmodules().

  3. and fall through to process elements in "fromlist" for the leaf
     module of "name"

     Processing elements in "fromlist" is executed in the code path
     after "if _pypy: .... else: ..." clause. Therefore, this patch
     replaces "if _pypy:" with "elif _pypy:" to share it.

At 4f1144c3c72b introducing original "work around" for "from a import
b" case, elements in "fromlist" were imported with "level=level". But
"level" might be grater than 1 (e.g. level=2 in "from .. import b"
case) at demandimport() invocation, and importing direct sub-module in
"fromlist" with level grater than 1 causes unexpected result.

IMHO, this seems main reason of "errors for unknown reason" described
in 4f1144c3c72b, and we don't have to worry about it, because this
issue was already fixed by 78d05778907b.

This is reason why this patch removes "errors for unknown reasons"
comment.

diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -188,15 +188,23 @@ def _demandimport(name, globals=None, lo
             if globalname and isinstance(symbol, _demandmod):
                 symbol._addref(globalname)
 
+        def chainmodules(rootmod, modname):
+            # recurse down the module chain, and return the leaf module
+            mod = rootmod
+            for comp in modname.split('.')[1:]:
+                if getattr(mod, comp, nothing) is nothing:
+                    setattr(mod, comp,
+                            _demandmod(comp, mod.__dict__, mod.__dict__))
+                mod = getattr(mod, comp)
+            return mod
+
         if level >= 0:
-            # The "from a import b,c,d" or "from .a import b,c,d"
-            # syntax gives errors with some modules for unknown
-            # reasons. Work around the problem.
             if name:
-                return _hgextimport(_origimport, name, globals, locals,
-                                    fromlist, level)
-
-            if _pypy:
+                # "from a import b" or "from .a import b" style
+                rootmod = _hgextimport(_origimport, name, globals, locals,
+                                       level=level)
+                mod = chainmodules(rootmod, name)
+            elif _pypy:
                 # PyPy's __import__ throws an exception if invoked
                 # with an empty name and no fromlist.  Recreate the
                 # desired behaviour by hand.
@@ -220,12 +228,7 @@ def _demandimport(name, globals=None, lo
         # But, we still need to support lazy loading of standard library and 3rd
         # party modules. So handle level == -1.
         mod = _hgextimport(_origimport, name, globals, locals)
-        # recurse down the module chain
-        for comp in name.split('.')[1:]:
-            if getattr(mod, comp, nothing) is nothing:
-                setattr(mod, comp,
-                        _demandmod(comp, mod.__dict__, mod.__dict__))
-            mod = getattr(mod, comp)
+        mod = chainmodules(mod, name)
 
         for x in fromlist:
             processfromitem(mod, x)
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -249,6 +249,191 @@ Check absolute/relative import of extens
   $TESTTMP/a (glob)
 #endif
 
+#if absimport
+
+Examine whether module loading is delayed until actual refering, even
+though module is imported with "absolute_import" feature.
+
+Files below in each packages are used for descirbed purpose:
+
+- "called": examine whether "from MODULE import ATTR" works correctly
+- "unused": examine whether loading is delayed correctly
+- "used":   examine whether "from PACKAGE import MODULE" works correctly
+
+Package hierarchy is needed to examine whether demand importing works
+as expected for "from SUB.PACK.AGE import MODULE".
+
+Setup "external library" to be imported with "absolute_import"
+feature.
+
+  $ mkdir -p $TESTTMP/extlibroot/lsub1/lsub2
+  $ touch $TESTTMP/extlibroot/__init__.py
+  $ touch $TESTTMP/extlibroot/lsub1/__init__.py
+  $ touch $TESTTMP/extlibroot/lsub1/lsub2/__init__.py
+
+  $ cat > $TESTTMP/extlibroot/lsub1/lsub2/called.py <<EOF
+  > def func():
+  >     return "this is extlibroot.lsub1.lsub2.called.func()"
+  > EOF
+  $ cat > $TESTTMP/extlibroot/lsub1/lsub2/unused.py <<EOF
+  > raise Exception("extlibroot.lsub1.lsub2.unused is loaded unintentionally")
+  > EOF
+  $ cat > $TESTTMP/extlibroot/lsub1/lsub2/used.py <<EOF
+  > detail = "this is extlibroot.lsub1.lsub2.used"
+  > EOF
+
+Setup sub-package of "external library", which causes instantiation of
+demandmod in "recurse down the module chain" code path. Relative
+importing with "absolute_import" feature isn't tested, because "level
+>=1 " doesn't cause instantiation of demandmod.
+
+  $ mkdir -p $TESTTMP/extlibroot/recursedown/abs
+  $ cat > $TESTTMP/extlibroot/recursedown/abs/used.py <<EOF
+  > detail = "this is extlibroot.recursedown.abs.used"
+  > EOF
+  $ cat > $TESTTMP/extlibroot/recursedown/abs/__init__.py <<EOF
+  > from __future__ import absolute_import
+  > from extlibroot.recursedown.abs.used import detail
+  > EOF
+
+  $ mkdir -p $TESTTMP/extlibroot/recursedown/legacy
+  $ cat > $TESTTMP/extlibroot/recursedown/legacy/used.py <<EOF
+  > detail = "this is extlibroot.recursedown.legacy.used"
+  > EOF
+  $ cat > $TESTTMP/extlibroot/recursedown/legacy/__init__.py <<EOF
+  > # legacy style (level == -1) import
+  > from extlibroot.recursedown.legacy.used import detail
+  > EOF
+
+  $ cat > $TESTTMP/extlibroot/recursedown/__init__.py <<EOF
+  > from __future__ import absolute_import
+  > from extlibroot.recursedown.abs import detail as absdetail
+  > from .legacy import detail as legacydetail
+  > EOF
+
+Setup extension local modules to be imported with "absolute_import"
+feature.
+
+  $ mkdir -p $TESTTMP/absextroot/xsub1/xsub2
+  $ touch $TESTTMP/absextroot/xsub1/__init__.py
+  $ touch $TESTTMP/absextroot/xsub1/xsub2/__init__.py
+
+  $ cat > $TESTTMP/absextroot/xsub1/xsub2/called.py <<EOF
+  > def func():
+  >     return "this is absextroot.xsub1.xsub2.called.func()"
+  > EOF
+  $ cat > $TESTTMP/absextroot/xsub1/xsub2/unused.py <<EOF
+  > raise Exception("absextroot.xsub1.xsub2.unused is loaded unintentionally")
+  > EOF
+  $ cat > $TESTTMP/absextroot/xsub1/xsub2/used.py <<EOF
+  > detail = "this is absextroot.xsub1.xsub2.used"
+  > EOF
+
+Setup extension local modules to examine whether demand importing
+works as expected in "level > 1" case.
+
+  $ cat > $TESTTMP/absextroot/relimportee.py <<EOF
+  > detail = "this is absextroot.relimportee"
+  > EOF
+  $ cat > $TESTTMP/absextroot/xsub1/xsub2/relimporter.py <<EOF
+  > from __future__ import absolute_import
+  > from ... import relimportee
+  > detail = "this relimporter imports %r" % (relimportee.detail)
+  > EOF
+
+Setup modules, which actually import extension local modules at
+runtime.
+
+  $ cat > $TESTTMP/absextroot/absolute.py << EOF
+  > from __future__ import absolute_import
+  > 
+  > # import extension local modules absolutely (level = 0)
+  > from absextroot.xsub1.xsub2 import used, unused
+  > from absextroot.xsub1.xsub2.called import func
+  > 
+  > def getresult():
+  >     result = []
+  >     result.append(used.detail)
+  >     result.append(func())
+  >     return result
+  > EOF
+
+  $ cat > $TESTTMP/absextroot/relative.py << EOF
+  > from __future__ import absolute_import
+  > 
+  > # import extension local modules relatively (level == 1)
+  > from .xsub1.xsub2 import used, unused
+  > from .xsub1.xsub2.called import func
+  > 
+  > # import a module, which implies "importing with level > 1"
+  > from .xsub1.xsub2 import relimporter
+  > 
+  > def getresult():
+  >     result = []
+  >     result.append(used.detail)
+  >     result.append(func())
+  >     result.append(relimporter.detail)
+  >     return result
+  > EOF
+
+Setup main procedure of extension.
+
+  $ cat > $TESTTMP/absextroot/__init__.py <<EOF
+  > from __future__ import absolute_import
+  > from mercurial import cmdutil
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > 
+  > # "absolute" and "relative" shouldn't be imported before actual
+  > # command execution, because (1) they import same modules, and (2)
+  > # preceding import (= instantiate "demandmod" object instead of
+  > # real "module" object) might hide problem of succeeding import.
+  > 
+  > @command('showabsolute', [], norepo=True)
+  > def showabsolute(ui, *args, **opts):
+  >     from absextroot import absolute
+  >     ui.write('ABS: %s\n' % '\nABS: '.join(absolute.getresult()))
+  > 
+  > @command('showrelative', [], norepo=True)
+  > def showrelative(ui, *args, **opts):
+  >     from . import relative
+  >     ui.write('REL: %s\n' % '\nREL: '.join(relative.getresult()))
+  > 
+  > # import modules from external library
+  > from extlibroot.lsub1.lsub2 import used as lused, unused as lunused
+  > from extlibroot.lsub1.lsub2.called import func as lfunc
+  > from extlibroot.recursedown import absdetail, legacydetail
+  > 
+  > def uisetup(ui):
+  >     result = []
+  >     result.append(lused.detail)
+  >     result.append(lfunc())
+  >     result.append(absdetail)
+  >     result.append(legacydetail)
+  >     ui.write('LIB: %s\n' % '\nLIB: '.join(result))
+  > EOF
+
+Examine module importing.
+
+  $ (PYTHONPATH=${PYTHONPATH}${PATHSEP}${TESTTMP}; hg --config extensions.absextroot=$TESTTMP/absextroot showabsolute)
+  LIB: this is extlibroot.lsub1.lsub2.used
+  LIB: this is extlibroot.lsub1.lsub2.called.func()
+  LIB: this is extlibroot.recursedown.abs.used
+  LIB: this is extlibroot.recursedown.legacy.used
+  ABS: this is absextroot.xsub1.xsub2.used
+  ABS: this is absextroot.xsub1.xsub2.called.func()
+
+  $ (PYTHONPATH=${PYTHONPATH}${PATHSEP}${TESTTMP}; hg --config extensions.absextroot=$TESTTMP/absextroot showrelative)
+  LIB: this is extlibroot.lsub1.lsub2.used
+  LIB: this is extlibroot.lsub1.lsub2.called.func()
+  LIB: this is extlibroot.recursedown.abs.used
+  LIB: this is extlibroot.recursedown.legacy.used
+  REL: this is absextroot.xsub1.xsub2.used
+  REL: this is absextroot.xsub1.xsub2.called.func()
+  REL: this relimporter imports 'this is absextroot.relimportee'
+
+#endif
+
   $ cd ..
 
 hide outer repo