| 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
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 '?'>