Patchwork [STABLE] demandimport: do not raise ImportError for unknown item in fromlist

login
register
mail settings
Submitter Yuya Nishihara
Date Dec. 19, 2016, 3:18 p.m.
Message ID <58ebfc1af9f290638bed.1482160716@mimosa>
Download mbox | patch
Permalink /patch/17967/
State Changes Requested
Headers show

Comments

Yuya Nishihara - Dec. 19, 2016, 3:18 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1482155160 -32400
#      Mon Dec 19 22:46:00 2016 +0900
# Branch stable
# Node ID 58ebfc1af9f290638bed3cc9faf9d6f2deb0fc20
# Parent  7817df5585db1d87d3f6c7f496085c86d87e2e9a
demandimport: do not raise ImportError for unknown item in fromlist

This is the behavior of the default __import__() function, which doesn't
validate the existence of the fromlist items. Later on, the missing attribute
is detected while processing the import statement.

https://hg.python.org/cpython/file/v2.7.13/Python/import.c#l2575

The comtypes library relies on this (maybe) undocumented behavior, and we
got a bug report to TortoiseHg, sigh.

https://bitbucket.org/tortoisehg/thg/issues/4647/

The test added at 26a4e46af2bc verifies the behavior of the import statement,
so this patch only adds the test of __import__() function and works around
CPython/PyPy difference.
Pierre-Yves David - Dec. 19, 2016, 3:37 p.m.
On 12/19/2016 04:18 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1482155160 -32400
> #      Mon Dec 19 22:46:00 2016 +0900
> # Branch stable
> # Node ID 58ebfc1af9f290638bed3cc9faf9d6f2deb0fc20
> # Parent  7817df5585db1d87d3f6c7f496085c86d87e2e9a
> demandimport: do not raise ImportError for unknown item in fromlist
>
> This is the behavior of the default __import__() function, which doesn't
> validate the existence of the fromlist items. Later on, the missing attribute
> is detected while processing the import statement.
>
> https://hg.python.org/cpython/file/v2.7.13/Python/import.c#l2575

O.o …

>
> The comtypes library relies on this (maybe) undocumented behavior, and we
> got a bug report to TortoiseHg, sigh.
>
> https://bitbucket.org/tortoisehg/thg/issues/4647/
>
> The test added at 26a4e46af2bc verifies the behavior of the import statement,
> so this patch only adds the test of __import__() function and works around
> CPython/PyPy difference.
>
> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
> --- a/mercurial/demandimport.py
> +++ b/mercurial/demandimport.py
> @@ -199,8 +199,11 @@ def _demandimport(name, globals=None, lo
>              nonpkg = getattr(mod, '__path__', nothing) is nothing
>              if symbol is nothing:
>                  if nonpkg:
> -                    # do not try relative import, which would raise ValueError
> -                    raise ImportError('cannot import name %s' % attr)
> +                    # do not try relative import, which would raise ValueError,
> +                    # and leave unknown attribute as the default __import__()
> +                    # would do. the missing attribute will be detected later
> +                    # while processing the import statement.
> +                    return
>                  mn = '%s.%s' % (mod.__name__, attr)
>                  if mn in ignore:
>                      importfunc = _origimport
> diff --git a/tests/test-demandimport.py b/tests/test-demandimport.py
> --- a/tests/test-demandimport.py
> +++ b/tests/test-demandimport.py
> @@ -70,7 +70,16 @@ try:
>      print('no demandmod should be created for attribute of non-package '
>            'module:\ncontextlib.unknownattr =', f(unknownattr))
>  except ImportError as inst:
> -    print('contextlib.unknownattr = ImportError: %s' % inst)
> +    print('contextlib.unknownattr = ImportError: %s'
> +          % rsub(r"'", '', str(inst)))
> +
> +# Unlike the import statement, __import__() function should not raise
> +# ImportError even if fromlist has an unknown item
> +# (see Python/import.c:import_module_level() and ensure_fromlist())
> +contextlibimp = __import__('contextlib', globals(), locals(), ['unknownattr'])
> +print("__import__('contextlib', ..., ['unknownattr']) =", f(contextlibimp))
> +print("hasattr(__import__('contextlib'), 'unknownattr') =",
> +      util.safehasattr(contextlibimp, 'unknownattr'))
>
>  demandimport.disable()
>  os.environ['HGDEMANDIMPORT'] = 'disable'
> diff --git a/tests/test-demandimport.py.out b/tests/test-demandimport.py.out
> --- a/tests/test-demandimport.py.out
> +++ b/tests/test-demandimport.py.out
> @@ -18,4 +18,6 @@ re.stderr = <open file '<whatever>', mod
>  re = <proxied module 'sys'>
>  contextlib = <unloaded module 'contextlib'>
>  contextlib.unknownattr = ImportError: cannot import name unknownattr
> +__import__('contextlib', ..., ['unknownattr']) = <module 'contextlib' from '?'>
> +hasattr(__import__('contextlib'), 'unknownattr') = False

That message is a bit misleading as we are not testing against
"__import__('contextlib')", but
"__import__('contextlib',…,['unknownattr'])"

So the test code is correct but the test output is a bit confusing. One 
way to clear this would be to drop the '__import__()' call around 
contextlib, but I'm open to other idea to make this clearer (including 
insisting the current form is clear).

It also looks like this while file could be transalted to unittest but 
this might be another adventure.

>  node = <module 'mercurial.node' from '?'>

Patch

diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -199,8 +199,11 @@  def _demandimport(name, globals=None, lo
             nonpkg = getattr(mod, '__path__', nothing) is nothing
             if symbol is nothing:
                 if nonpkg:
-                    # do not try relative import, which would raise ValueError
-                    raise ImportError('cannot import name %s' % attr)
+                    # do not try relative import, which would raise ValueError,
+                    # and leave unknown attribute as the default __import__()
+                    # would do. the missing attribute will be detected later
+                    # while processing the import statement.
+                    return
                 mn = '%s.%s' % (mod.__name__, attr)
                 if mn in ignore:
                     importfunc = _origimport
diff --git a/tests/test-demandimport.py b/tests/test-demandimport.py
--- a/tests/test-demandimport.py
+++ b/tests/test-demandimport.py
@@ -70,7 +70,16 @@  try:
     print('no demandmod should be created for attribute of non-package '
           'module:\ncontextlib.unknownattr =', f(unknownattr))
 except ImportError as inst:
-    print('contextlib.unknownattr = ImportError: %s' % inst)
+    print('contextlib.unknownattr = ImportError: %s'
+          % rsub(r"'", '', str(inst)))
+
+# Unlike the import statement, __import__() function should not raise
+# ImportError even if fromlist has an unknown item
+# (see Python/import.c:import_module_level() and ensure_fromlist())
+contextlibimp = __import__('contextlib', globals(), locals(), ['unknownattr'])
+print("__import__('contextlib', ..., ['unknownattr']) =", f(contextlibimp))
+print("hasattr(__import__('contextlib'), 'unknownattr') =",
+      util.safehasattr(contextlibimp, 'unknownattr'))
 
 demandimport.disable()
 os.environ['HGDEMANDIMPORT'] = 'disable'
diff --git a/tests/test-demandimport.py.out b/tests/test-demandimport.py.out
--- a/tests/test-demandimport.py.out
+++ b/tests/test-demandimport.py.out
@@ -18,4 +18,6 @@  re.stderr = <open file '<whatever>', mod
 re = <proxied module 'sys'>
 contextlib = <unloaded module 'contextlib'>
 contextlib.unknownattr = ImportError: cannot import name unknownattr
+__import__('contextlib', ..., ['unknownattr']) = <module 'contextlib' from '?'>
+hasattr(__import__('contextlib'), 'unknownattr') = False
 node = <module 'mercurial.node' from '?'>