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 :)

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).

I have a fix en-route.

Got a patch up if either of you would like to review.

(And sorry about stealing this from you, Terry; I forgot to flip the assignment while I was looking into this.)

Thanks :) I've submitted a review for the patch.

New changeset 5086589 by Miss Islington (bot) (Brett Cannon) in branch 'master':
bpo-36298: Raise ModuleNotFoundError in pyclbr when a module can't be found (GH-12358)
5086589

Thanks, mental!