bpo-24792: Fix zipimporter masking the cause of import errors

[iritkatriel]

zipimport's _unmarshal_code swallows import errors and then _get_module_code doesn't know the cause of the error, and returns the generic, and sometimes incorrect, 'could not find...'.

Automerge-Triggered-By: GH:brettcannon

https://bugs.python.org/issue24792

[iritkatriel]

Travis is complaining about "Generated files not up to date".
Do I need to commit and push Python/importlib_zipimport.h as well?

[brettcannon]

@iritkatriel yes, you need to update the .h file as well.

[gpshead]

we should also add an additional test_bad_pyc_hash that is like a combination of test_checked_hash_based_change_pyc and this testBadMagic2 where instead of flipping a bit in the magic number, we flip a bit in the pyc hash and expect the error raised by _bootstrap_external._validate_hash_pyc to make its way to us.

[iritkatriel]

Indeed.

[iritkatriel]

Actually the user will never see this exception because the check only happens when the zipfile has a .py file to compare the hash with. And then, if the hash doesn't match it just loads the '.py'.

I can get the exception in a test by temporarily removing the '.py' loader. Is this going too much into internals?
I added this to the end of test_checked_hash_based_change_pyc:

    # now remove the '.py' importer so that the '.pyc'
    # failure propagates the exception
    del sys.modules[TESTMOD]
    prev_zip_searchorder = zipimport._zip_searchorder
    zipimport._zip_searchorder = zipimport._zip_searchorder[0:3]
    try:
        self.doTest(None, files, TESTMOD)
        raise AssertionError("This should not be reached")
    except zipimport.ZipImportError as exc:
        cause = "hash in bytecode doesn't match hash of source"
        cause = f"{cause} {TESTMOD!r}"
        expected_msg = f"module load failed: {cause}" 
        self.assertIn(expected_msg, exc.msg)
        self.assertIsInstance(exc.__cause__, ImportError)
        self.assertIn(cause, exc.__cause__.msg)
    finally:
        zipimport._zip_searchorder = prev_zip_searchorder

[iritkatriel]

I think this test should not be added because it asserts behavior which is not visible to a user. test_checked_hash_based_change_pyc already checks this error case, which this PR does not change because the exception always swallowed.

[brettcannon]

A slight test tweak to make sure we are not too rigid on the exception message, otherwise LGTM!

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[iritkatriel]

I have made the requested changes; please review again. I also rebased to resolve merge conflicts.

[bedevere-bot]

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

[miss-islington]

Sorry, I can't merge this PR. Reason: Pull Request is not mergeable.

[brettcannon]

@iritkatriel sorry for the delay in approval! If you can regenerate importlib_zipimport.h then I will merge this straight-away.

[iritkatriel]

I've rebased and updated importlib_zipimport.h. Thanks.

[miss-islington]

@iritkatriel: Status check is done, and it's a success ✅ .

[miss-islington]

Sorry, I can't merge this PR. Reason: 2 of 4 required status checks are expected..

[iritkatriel]

I've rebased and updated importlib_zipimport.h. Thanks.

Now I did. (It was the wrong h file before).

[miss-islington]

@iritkatriel: Status check is done, and it's a success ✅ .

[brettcannon]

Thanks!