classification
Title: Lib/pyclbr.py crashes when the package spec cannot be determined by importlib
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, mental, miss-islington
Priority: normal Keywords: patch

Created on 2019-03-15 05:43 by mental, last changed 2019-03-25 18:15 by brett.cannon. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12358 merged brett.cannon, 2019-03-15 21:07
Messages (8)
msg337966 - (view) Author: (mental) Date: 2019-03-15 05:43
Hi folks! (apologies in advance if any of the code blocks come out irregular, this is my first issue)


I was just exploring the `Lib` modules out of curiosity and I discovered `pyclbr` a peculiar artifact from the older days of the Python standard library.

I noticed the module could be run directly and after attempting to run it withan invalid source target `python -m pyclbr somenonexistentfile` it raised ```
Traceback (most recent call last):
  File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/usr/lib/python3.7/pyclbr.py", line 405, in <module>
    _main()
  File "/usr/lib/python3.7/pyclbr.py", line 380, in _main
    tree = readmodule_ex(mod, path)
  File "/usr/lib/python3.7/pyclbr.py", line 116, in readmodule_ex
    return _readmodule(module, path or [])
  File "/usr/lib/python3.7/pyclbr.py", line 169, in _readmodule
    if spec.submodule_search_locations is not None:
AttributeError: 'NoneType' object has no attribute 'submodule_search_locations'```

I was running 3.7.2, although I assume this affects future versions and possibly some older versions. (I suspect the bug originates from importlib silently breaking backwards compatability)

I thought it strange for a script to exit so loudly so after reading through the source I believe the intended behavior meant for an invalid target is to raise an `ImportError` although I admit I'm not convinced this is still the best way to exit from erroneous input (or even if the module is still relevant in todays code?)

I believe this is a bug (but I would very much appreciate a second opinion) and I've identified it as a low priority easy fix, In which case I'd be more than happy to submit a fix :)
msg338028 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-03-15 20:28
I finished patching pyclbr to be a complete module class and function browser in summer 2017, for use by IDLE's class browser.

I verified the bug with the import API, with a twist.

>>> import pyclbr
>>> pyclbr.readmodule_ex('<invalid')
...
AttributeError: 'NoneType' object has no attribute 'submodule_search_locations'
>>> pyclbr.readmodule_ex('<invalid>')
{}


The relevant block in pyclbr is

    spec = importlib.util._find_spec_from_path(fullmodule, search_path)
    _modules[fullmodule] = tree
    # Is module a package?
    if spec.submodule_search_locations is not None:
        tree['__path__'] = spec.submodule_search_locations

Caching (fullmodule, tree) before the spec access is why the repeat call above returns the empty tree.

This block is preceded by a block that explicitly raises 'ImportError', even though no import is attemped.  It is followed by a block that returns the tree in case of error.

    try:
        source = spec.loader.get_source(fullmodule)
        if source is None:
            return tree
    except (AttributeError, ImportError):
        # If module is not Python source, we cannot do anything.
        return tree

So either a) the empty tree should be returned when the  NoneType exception is caught, or b) the invalid names should not be cached and the AttributeError turned into an 'ImportError'. 

I believe we should do the latter, as you suggested, and move the caching line down to follow the new try-except.  If you want, submit a PR for the master branch *after* signing the CLA.  If you can, include a new assertRaises test based on the invalid call above.  And check that '<invalid>' is not in pyclbr._modules with assertNotIn('<invalid>', pyclbr._modules).
msg338032 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-03-15 20:51
I have a fix en-route.
msg338034 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-03-15 21:07
Got a patch up if either of you would like to review.
msg338037 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-03-15 21:19
(And sorry about stealing this from you, Terry; I forgot to flip the assignment while I was looking into this.)
msg338094 - (view) Author: (mental) Date: 2019-03-16 19:55
Thanks :) I've submitted a review for the patch.
msg338618 - (view) Author: miss-islington (miss-islington) Date: 2019-03-22 22:16
New changeset 5086589305ff5538709856b27314b68f06ae93db by Miss Islington (bot) (Brett Cannon) in branch 'master':
bpo-36298: Raise ModuleNotFoundError in pyclbr when a module can't be found (GH-12358)
https://github.com/python/cpython/commit/5086589305ff5538709856b27314b68f06ae93db
msg338817 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-03-25 18:15
Thanks, mental!
History
Date User Action Args
2019-03-25 18:15:15brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg338817

stage: commit review -> resolved
2019-03-22 22:16:52miss-islingtonsetnosy: + miss-islington
messages: + msg338618
2019-03-16 19:55:04mentalsetmessages: + msg338094
2019-03-15 21:19:05brett.cannonsetmessages: + msg338037
2019-03-15 21:07:59brett.cannonsetmessages: + msg338034
2019-03-15 21:07:43brett.cannonsetstage: patch review -> commit review
2019-03-15 21:07:22brett.cannonsetkeywords: + patch
stage: patch review
pull_requests: + pull_request12325
2019-03-15 20:51:00brett.cannonsetmessages: + msg338032
2019-03-15 20:50:28brett.cannonsetnosy: - terry.reedy
title: Lib/pyclbr.py fails when package spec cannot be determined -> Lib/pyclbr.py crashes when the package spec cannot be determined by importlib
assignee: terry.reedy -> brett.cannon
versions: - Python 3.8
stage: needs patch -> (no value)
2019-03-15 20:28:38terry.reedysetassignee: terry.reedy

title: Lib/pyclbr.py crashes when the package spec cannot be determined by importlib -> Lib/pyclbr.py fails when package spec cannot be determined
nosy: + terry.reedy
versions: + Python 3.8
messages: + msg338028
stage: needs patch
2019-03-15 07:01:02SilentGhostsetnosy: + brett.cannon
components: + Library (Lib)
2019-03-15 05:48:50mentalsettype: crash -> behavior
2019-03-15 05:43:26mentalcreate