Issue #24693: Changed some RuntimeError's in the zipfile module to more

appropriate types. Improved some error messages and debugging output.

4 files changed, 94 insertions, 63 deletions

diff --git a/Doc/library/zipfile.rst b/Doc/library/zipfile.rst
index 6c9d207..4b92e44 100644
--- a/Doc/library/zipfile.rst
+++ b/Doc/library/zipfile.rst
@@ -147,10 +147,10 @@ ZipFile Objects
    *compression* is the ZIP compression method to use when writing the archive,
    and should be :const:`ZIP_STORED`, :const:`ZIP_DEFLATED`,
    :const:`ZIP_BZIP2` or :const:`ZIP_LZMA`; unrecognized
-   values will cause :exc:`RuntimeError` to be raised.  If :const:`ZIP_DEFLATED`,
+   values will cause :exc:`NotImplementedError` to be raised.  If :const:`ZIP_DEFLATED`,
    :const:`ZIP_BZIP2` or :const:`ZIP_LZMA` is specified but the corresponding module
    (:mod:`zlib`, :mod:`bz2` or :mod:`lzma`) is not available, :exc:`RuntimeError`
-   is also raised. The default is :const:`ZIP_STORED`.  If *allowZip64* is
+   is raised. The default is :const:`ZIP_STORED`.  If *allowZip64* is
    ``True`` (the default) zipfile will create ZIP files that use the ZIP64
    extensions when the zipfile is larger than 2 GiB. If it is  false :mod:`zipfile`
    will raise an exception when the ZIP file would require ZIP64 extensions.
@@ -179,6 +179,10 @@ ZipFile Objects
       Added support for writing to unseekable streams.
       Added support for the ``'x'`` mode.
 
+   .. versionchanged:: 3.6
+      Previously, a plain :exc:`RuntimeError` was raised for unrecognized
+      compression values.
+
 
 .. method:: ZipFile.close()
 
@@ -211,7 +215,6 @@ ZipFile Objects
    can be either the name of a file within the archive or a :class:`ZipInfo`
    object.  The *mode* parameter, if included, must be ``'r'`` (the default)
    or ``'w'``.  *pwd* is the password used to decrypt encrypted ZIP files.
-   Calling :meth:`.open` on a closed ZipFile will raise a :exc:`RuntimeError`.
 
    :meth:`~ZipFile.open` is also a context manager and therefore supports the
    :keyword:`with` statement::
@@ -230,7 +233,7 @@ ZipFile Objects
    With ``mode='w'``, a writable file handle is returned, which supports the
    :meth:`~io.BufferedIOBase.write` method.  While a writable file handle is open,
    attempting to read or write other files in the ZIP file will raise a
-   :exc:`RuntimeError`.
+   :exc:`ValueError`.
 
    When writing a file, if the file size is not known in advance but may exceed
    2 GiB, pass ``force_zip64=True`` to ensure that the header format is
@@ -252,6 +255,11 @@ ZipFile Objects
       :meth:`open` can now be used to write files into the archive with the
       ``mode='w'`` option.
 
+   .. versionchanged:: 3.6
+      Calling :meth:`.open` on a closed ZipFile will raise a :exc:`ValueError`.
+      Previously, a :exc:`RuntimeError` was raised.
+
+
 .. method:: ZipFile.extract(member, path=None, pwd=None)
 
    Extract a member from the archive to the current working directory; *member*
@@ -272,6 +280,10 @@ ZipFile Objects
       characters (``:``, ``<``, ``>``, ``|``, ``"``, ``?``, and ``*``)
       replaced by underscore (``_``).
 
+   .. versionchanged:: 3.6
+      Calling :meth:`extract` on a closed ZipFile will raise a
+      :exc:`ValueError`.  Previously, a :exc:`RuntimeError` was raised.
+
 
 .. method:: ZipFile.extractall(path=None, members=None, pwd=None)
 
@@ -288,6 +300,10 @@ ZipFile Objects
       dots ``".."``.  This module attempts to prevent that.
       See :meth:`extract` note.
 
+   .. versionchanged:: 3.6
+      Calling :meth:`extractall` on a closed ZipFile will raise a
+      :exc:`ValueError`.  Previously, a :exc:`RuntimeError` was raised.
+
 
 .. method:: ZipFile.printdir()
 
@@ -305,18 +321,24 @@ ZipFile Objects
    file in the archive, or a :class:`ZipInfo` object.  The archive must be open for
    read or append. *pwd* is the password used for encrypted  files and, if specified,
    it will override the default password set with :meth:`setpassword`.  Calling
-   :meth:`read` on a closed ZipFile  will raise a :exc:`RuntimeError`. Calling
    :meth:`read` on a ZipFile that uses a compression method other than
    :const:`ZIP_STORED`, :const:`ZIP_DEFLATED`, :const:`ZIP_BZIP2` or
    :const:`ZIP_LZMA` will raise a :exc:`NotImplementedError`. An error will also
    be raised if the corresponding compression module is not available.
 
+   .. versionchanged:: 3.6
+      Calling :meth:`read` on a closed ZipFile will raise a :exc:`ValueError`.
+      Previously, a :exc:`RuntimeError` was raised.
+
 
 .. method:: ZipFile.testzip()
 
    Read all the files in the archive and check their CRC's and file headers.
-   Return the name of the first bad file, or else return ``None``. Calling
-   :meth:`testzip` on a closed ZipFile will raise a :exc:`RuntimeError`.
+   Return the name of the first bad file, or else return ``None``.
+
+   .. versionchanged:: 3.6
+      Calling :meth:`testfile` on a closed ZipFile will raise a
+      :exc:`ValueError`.  Previously, a :exc:`RuntimeError` was raised.
 
 
 .. method:: ZipFile.write(filename, arcname=None, compress_type=None)
@@ -326,10 +348,7 @@ ZipFile Objects
    letter and with leading path separators removed).  If given, *compress_type*
    overrides the value given for the *compression* parameter to the constructor for
    the new entry.
-   The archive must be open with mode ``'w'``, ``'x'`` or ``'a'`` -- calling
-   :meth:`write` on a ZipFile created with mode ``'r'`` will raise a
-   :exc:`RuntimeError`.  Calling  :meth:`write` on a closed ZipFile will raise a
-   :exc:`RuntimeError`.
+   The archive must be open with mode ``'w'``, ``'x'`` or ``'a'``.
 
    .. note::
 
@@ -348,16 +367,19 @@ ZipFile Objects
       If ``arcname`` (or ``filename``, if ``arcname`` is  not given) contains a null
       byte, the name of the file in the archive will be truncated at the null byte.
 
+   .. versionchanged:: 3.6
+      Calling :meth:`write` on a ZipFile created with mode ``'r'`` or
+      a closed ZipFile will raise a :exc:`ValueError`.  Previously,
+      a :exc:`RuntimeError` was raised.
+
+
 .. method:: ZipFile.writestr(zinfo_or_arcname, data[, compress_type])
 
    Write the string *data* to the archive; *zinfo_or_arcname* is either the file
    name it will be given in the archive, or a :class:`ZipInfo` instance.  If it's
    an instance, at least the filename, date, and time must be given.  If it's a
    name, the date and time is set to the current date and time.
-   The archive must be opened with mode ``'w'``, ``'x'`` or ``'a'`` -- calling
-   :meth:`writestr` on a ZipFile created with mode ``'r'`` will raise a
-   :exc:`RuntimeError`.  Calling :meth:`writestr` on a closed ZipFile will
-   raise a :exc:`RuntimeError`.
+   The archive must be opened with mode ``'w'``, ``'x'`` or ``'a'``.
 
    If given, *compress_type* overrides the value given for the *compression*
    parameter to the constructor for the new entry, or in the *zinfo_or_arcname*
@@ -373,6 +395,12 @@ ZipFile Objects
    .. versionchanged:: 3.2
       The *compress_type* argument.
 
+   .. versionchanged:: 3.6
+      Calling :meth:`writestr` on a ZipFile created with mode ``'r'`` or
+      a closed ZipFile will raise a :exc:`ValueError`.  Previously,
+      a :exc:`RuntimeError` was raised.
+
+
 The following data attributes are also available:
 
 
diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py
index ef3c3d8..ff4a6d6 100644
--- a/Lib/test/test_zipfile.py
+++ b/Lib/test/test_zipfile.py
@@ -449,15 +449,15 @@ class StoredTestsWithSourceFile(AbstractTestsWithSourceFile,
 
     def test_write_to_readonly(self):
         """Check that trying to call write() on a readonly ZipFile object
-        raises a RuntimeError."""
+        raises a ValueError."""
         with zipfile.ZipFile(TESTFN2, mode="w") as zipfp:
             zipfp.writestr("somefile.txt", "bogus")
 
         with zipfile.ZipFile(TESTFN2, mode="r") as zipfp:
-            self.assertRaises(RuntimeError, zipfp.write, TESTFN)
+            self.assertRaises(ValueError, zipfp.write, TESTFN)
 
         with zipfile.ZipFile(TESTFN2, mode="r") as zipfp:
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(ValueError):
                 zipfp.open(TESTFN, mode='w')
 
     def test_add_file_before_1980(self):
@@ -1210,27 +1210,27 @@ class OtherTests(unittest.TestCase):
             fp.write("short file")
         self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, TESTFN)
 
-    def test_closed_zip_raises_RuntimeError(self):
+    def test_closed_zip_raises_ValueError(self):
         """Verify that testzip() doesn't swallow inappropriate exceptions."""
         data = io.BytesIO()
         with zipfile.ZipFile(data, mode="w") as zipf:
             zipf.writestr("foo.txt", "O, for a Muse of Fire!")
 
         # This is correct; calling .read on a closed ZipFile should raise
-        # a RuntimeError, and so should calling .testzip.  An earlier
+        # a ValueError, and so should calling .testzip.  An earlier
         # version of .testzip would swallow this exception (and any other)
         # and report that the first file in the archive was corrupt.
-        self.assertRaises(RuntimeError, zipf.read, "foo.txt")
-        self.assertRaises(RuntimeError, zipf.open, "foo.txt")
-        self.assertRaises(RuntimeError, zipf.testzip)
-        self.assertRaises(RuntimeError, zipf.writestr, "bogus.txt", "bogus")
+        self.assertRaises(ValueError, zipf.read, "foo.txt")
+        self.assertRaises(ValueError, zipf.open, "foo.txt")
+        self.assertRaises(ValueError, zipf.testzip)
+        self.assertRaises(ValueError, zipf.writestr, "bogus.txt", "bogus")
         with open(TESTFN, 'w') as f:
             f.write('zipfile test data')
-        self.assertRaises(RuntimeError, zipf.write, TESTFN)
+        self.assertRaises(ValueError, zipf.write, TESTFN)
 
     def test_bad_constructor_mode(self):
         """Check that bad modes passed to ZipFile constructor are caught."""
-        self.assertRaises(RuntimeError, zipfile.ZipFile, TESTFN, "q")
+        self.assertRaises(ValueError, zipfile.ZipFile, TESTFN, "q")
 
     def test_bad_open_mode(self):
         """Check that bad modes passed to ZipFile.open are caught."""
@@ -1240,10 +1240,10 @@ class OtherTests(unittest.TestCase):
         with zipfile.ZipFile(TESTFN, mode="r") as zipf:
             # read the data to make sure the file is there
             zipf.read("foo.txt")
-            self.assertRaises(RuntimeError, zipf.open, "foo.txt", "q")
+            self.assertRaises(ValueError, zipf.open, "foo.txt", "q")
             # universal newlines support is removed
-            self.assertRaises(RuntimeError, zipf.open, "foo.txt", "U")
-            self.assertRaises(RuntimeError, zipf.open, "foo.txt", "rU")
+            self.assertRaises(ValueError, zipf.open, "foo.txt", "U")
+            self.assertRaises(ValueError, zipf.open, "foo.txt", "rU")
 
     def test_read0(self):
         """Check that calling read(0) on a ZipExtFile object returns an empty
@@ -1266,7 +1266,7 @@ class OtherTests(unittest.TestCase):
     def test_bad_compression_mode(self):
         """Check that bad compression methods passed to ZipFile.open are
         caught."""
-        self.assertRaises(RuntimeError, zipfile.ZipFile, TESTFN, "w", -1)
+        self.assertRaises(NotImplementedError, zipfile.ZipFile, TESTFN, "w", -1)
 
     def test_unsupported_compression(self):
         # data is declared as shrunk, but actually deflated
@@ -1423,15 +1423,15 @@ class OtherTests(unittest.TestCase):
             with zipf.open('foo', mode='w') as w2:
                 w2.write(msg1)
             with zipf.open('bar', mode='w') as w1:
-                with self.assertRaises(RuntimeError):
+                with self.assertRaises(ValueError):
                     zipf.open('handle', mode='w')
-                with self.assertRaises(RuntimeError):
+                with self.assertRaises(ValueError):
                     zipf.open('foo', mode='r')
-                with self.assertRaises(RuntimeError):
+                with self.assertRaises(ValueError):
                     zipf.writestr('str', 'abcde')
-                with self.assertRaises(RuntimeError):
+                with self.assertRaises(ValueError):
                     zipf.write(__file__, 'file')
-                with self.assertRaises(RuntimeError):
+                with self.assertRaises(ValueError):
                     zipf.close()
                 w1.write(msg2)
             with zipf.open('baz', mode='w') as w2:
diff --git a/Lib/zipfile.py b/Lib/zipfile.py
index 8dd064a..7ba4e59 100644
--- a/Lib/zipfile.py
+++ b/Lib/zipfile.py
@@ -449,7 +449,7 @@ class ZipInfo (object):
                 elif ln == 0:
                     counts = ()
                 else:
-                    raise RuntimeError("Corrupt extra field %s"%(ln,))
+                    raise BadZipFile("Corrupt extra field %04x (size=%d)" % (tp, ln))
 
                 idx = 0
 
@@ -654,7 +654,7 @@ def _check_compression(compression):
             raise RuntimeError(
                 "Compression requires the (missing) lzma module")
     else:
-        raise RuntimeError("That compression method is not supported")
+        raise NotImplementedError("That compression method is not supported")
 
 
 def _get_compressor(compress_type):
@@ -697,7 +697,7 @@ class _SharedFile:
     def read(self, n=-1):
         with self._lock:
             if self._writing():
-                raise RuntimeError("Can't read from the ZIP file while there "
+                raise ValueError("Can't read from the ZIP file while there "
                         "is an open writing handle on it. "
                         "Close the writing handle before trying to read.")
             self._file.seek(self._pos)
@@ -1055,7 +1055,7 @@ class ZipFile:
         """Open the ZIP file with mode read 'r', write 'w', exclusive create 'x',
         or append 'a'."""
         if mode not in ('r', 'w', 'x', 'a'):
-            raise RuntimeError("ZipFile requires mode 'r', 'w', 'x', or 'a'")
+            raise ValueError("ZipFile requires mode 'r', 'w', 'x', or 'a'")
 
         _check_compression(compression)
 
@@ -1129,7 +1129,7 @@ class ZipFile:
                     self._didModify = True
                     self.start_dir = self.fp.tell()
             else:
-                raise RuntimeError("Mode must be 'r', 'w', 'x', or 'a'")
+                raise ValueError("Mode must be 'r', 'w', 'x', or 'a'")
         except:
             fp = self.fp
             self.fp = None
@@ -1277,7 +1277,7 @@ class ZipFile:
     def setpassword(self, pwd):
         """Set default password for encrypted files."""
         if pwd and not isinstance(pwd, bytes):
-            raise TypeError("pwd: expected bytes, got %s" % type(pwd))
+            raise TypeError("pwd: expected bytes, got %s" % type(pwd).__name__)
         if pwd:
             self.pwd = pwd
         else:
@@ -1291,7 +1291,7 @@ class ZipFile:
     @comment.setter
     def comment(self, comment):
         if not isinstance(comment, bytes):
-            raise TypeError("comment: expected bytes, got %s" % type(comment))
+            raise TypeError("comment: expected bytes, got %s" % type(comment).__name__)
         # check for valid comment length
         if len(comment) > ZIP_MAX_COMMENT:
             import warnings
@@ -1323,13 +1323,13 @@ class ZipFile:
         instance for name, with zinfo.file_size set.
         """
         if mode not in {"r", "w"}:
-            raise RuntimeError('open() requires mode "r" or "w"')
+            raise ValueError('open() requires mode "r" or "w"')
         if pwd and not isinstance(pwd, bytes):
-            raise TypeError("pwd: expected bytes, got %s" % type(pwd))
+            raise TypeError("pwd: expected bytes, got %s" % type(pwd).__name__)
         if pwd and (mode == "w"):
             raise ValueError("pwd is only supported for reading files")
         if not self.fp:
-            raise RuntimeError(
+            raise ValueError(
                 "Attempt to use ZIP archive that was already closed")
 
         # Make sure we have an info object
@@ -1347,7 +1347,7 @@ class ZipFile:
             return self._open_to_write(zinfo, force_zip64=force_zip64)
 
         if self._writing:
-            raise RuntimeError("Can't read from the ZIP file while there "
+            raise ValueError("Can't read from the ZIP file while there "
                     "is an open writing handle on it. "
                     "Close the writing handle before trying to read.")
 
@@ -1394,7 +1394,7 @@ class ZipFile:
                 if not pwd:
                     pwd = self.pwd
                 if not pwd:
-                    raise RuntimeError("File %s is encrypted, password "
+                    raise RuntimeError("File %r is encrypted, password "
                                        "required for extraction" % name)
 
                 zd = _ZipDecrypter(pwd)
@@ -1412,7 +1412,7 @@ class ZipFile:
                     # compare against the CRC otherwise
                     check_byte = (zinfo.CRC >> 24) & 0xff
                 if h[11] != check_byte:
-                    raise RuntimeError("Bad password for file", name)
+                    raise RuntimeError("Bad password for file %r" % name)
 
             return ZipExtFile(zef_file, mode, zinfo, zd, True)
         except:
@@ -1426,9 +1426,9 @@ class ZipFile:
                 "the ZIP file."
             )
         if self._writing:
-            raise RuntimeError("Can't write to the ZIP file while there is "
-                               "another write handle open on it. "
-                               "Close the first handle before opening another.")
+            raise ValueError("Can't write to the ZIP file while there is "
+                             "another write handle open on it. "
+                             "Close the first handle before opening another.")
 
         # Sizes and CRC are overwritten with correct data after processing the file
         if not hasattr(zinfo, 'file_size'):
@@ -1548,9 +1548,9 @@ class ZipFile:
             import warnings
             warnings.warn('Duplicate name: %r' % zinfo.filename, stacklevel=3)
         if self.mode not in ('w', 'x', 'a'):
-            raise RuntimeError("write() requires mode 'w', 'x', or 'a'")
+            raise ValueError("write() requires mode 'w', 'x', or 'a'")
         if not self.fp:
-            raise RuntimeError(
+            raise ValueError(
                 "Attempt to write ZIP archive that was already closed")
         _check_compression(zinfo.compress_type)
         if not self._allowZip64:
@@ -1569,10 +1569,10 @@ class ZipFile:
         """Put the bytes from filename into the archive under the name
         arcname."""
         if not self.fp:
-            raise RuntimeError(
+            raise ValueError(
                 "Attempt to write to ZIP archive that was already closed")
         if self._writing:
-            raise RuntimeError(
+            raise ValueError(
                 "Can't write to ZIP archive while an open writing handle exists"
             )
 
@@ -1628,10 +1628,10 @@ class ZipFile:
             zinfo = zinfo_or_arcname
 
         if not self.fp:
-            raise RuntimeError(
+            raise ValueError(
                 "Attempt to write to ZIP archive that was already closed")
         if self._writing:
-            raise RuntimeError(
+            raise ValueError(
                 "Can't write to ZIP archive while an open writing handle exists."
             )
 
@@ -1654,9 +1654,9 @@ class ZipFile:
             return
 
         if self._writing:
-            raise RuntimeError("Can't close the ZIP file while there is "
-                               "an open writing handle on it. "
-                               "Close the writing handle before closing the zip.")
+            raise ValueError("Can't close the ZIP file while there is "
+                             "an open writing handle on it. "
+                             "Close the writing handle before closing the zip.")
 
         try:
             if self.mode in ('w', 'x', 'a') and self._didModify: # write ending records
@@ -1803,7 +1803,7 @@ class PyZipFile(ZipFile):
         if filterfunc and not filterfunc(pathname):
             if self.debug:
                 label = 'path' if os.path.isdir(pathname) else 'file'
-                print('%s "%s" skipped by filterfunc' % (label, pathname))
+                print('%s %r skipped by filterfunc' % (label, pathname))
             return
         dir, name = os.path.split(pathname)
         if os.path.isdir(pathname):
@@ -1834,7 +1834,7 @@ class PyZipFile(ZipFile):
                     elif ext == ".py":
                         if filterfunc and not filterfunc(path):
                             if self.debug:
-                                print('file "%s" skipped by filterfunc' % path)
+                                print('file %r skipped by filterfunc' % path)
                             continue
                         fname, arcname = self._get_codename(path[0:-3],
                                                             basename)
@@ -1851,7 +1851,7 @@ class PyZipFile(ZipFile):
                     if ext == ".py":
                         if filterfunc and not filterfunc(path):
                             if self.debug:
-                                print('file "%s" skipped by filterfunc' % path)
+                                print('file %r skipped by filterfunc' % path)
                             continue
                         fname, arcname = self._get_codename(path[0:-3],
                                                             basename)
diff --git a/Misc/NEWS b/Misc/NEWS
index 42821a4..27a3d8f 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -135,6 +135,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #24693: Changed some RuntimeError's in the zipfile module to more
+  appropriate types. Improved some error messages and debugging output.
+
 - Issue #17909: ``json.load`` and ``json.loads`` now support binary input
   encoded as UTF-8, UTF-16 or UTF-32. Patch by Serhiy Storchaka.