Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Add proper exception types

Open jleclanche opened this issue 8 years ago • 6 comments

Addressing #1643

This was pretty exhausting. It doesn't cover everything, there's plenty more places where IOError / ValueError etc could be replaced with PILReadError / PILWriteError.

To make this more backwards compatible we could make it subclass SyntaxError but... honestly, I'd be glad to see the end of this.

I'd love it if someone can take these commits and build on them. I suspect they break a ton of stuff due to the try/excepts all over the place.

Couple more commits to clean up the more egregious stuff I found while doing this...

jleclanche avatar Jan 06 '17 04:01 jleclanche

Now with updated tests

jleclanche avatar Jan 06 '17 17:01 jleclanche

And now with a bunch of fixes from @radarhere

jleclanche avatar Jan 06 '17 21:01 jleclanche

First of all, this is going to break everyone who follows recommended practice and limits exceptions that they catch. This is... not good.

Second we get to one of the two hard problems in CS. Naming things.

In general, we're moving away from PIL as a name. I object to adding anything that is PILFoo, especially in places where it's hard to change them. And user code is the hardest place to change. These are essentially hard coded constants that are going to get into other people's code where the lifetime is rather long. We're in a namespace, we might as well use it.

NoPluginFound will trigger based on certain invalid images where there's enough of a header that it's recognized as a specific image type, but some error condition slipped through (e.g. negative size in X).

InvalidFileType is effectively "WrongFileTypeForPlugin". Both this and the NoPluginFound names imply a larger scope than what is a image format specific error.

Stepping back for a second, there are effectively ~three~ four sorts of errors that we could raise from a plugin on open (note that we don't currently raise all of these):

  • The image is the wrong type for the plugin. This is a purely internal error that should never leak into the API.
  • The image is the right type for the plugin, but uses options that we don't support.(e.g. compression)
  • The image is recognized as the correct type, but there are fatal issues with the metadata (negative sizes, overflow)
  • There are non-fatal issues with the metadata. (Warning level e.g. completely borked non core metadata, incorrect types in core tiff metadata)

And from the opening as a whole, there are potentially:

  • A totally unrecognized image. Could be a ham sandwich for all we know.
  • An image that we could decode but are missing a dependency. (webp/jpeg/png/tiff if lib* is not installed)
  • Raised Unsupported feature, Fatal metadata error, or Warnings.

At the load stage, we add the potential for a similar set of errors corresponding to corrupted data or truncated files.

wiredfool avatar Jan 17 '17 11:01 wiredfool

Yep, this PR isn't intended as final, but more as an informal starting point for the discussion.

In general, we're moving away from PIL as a name. I object to adding anything that is PILFoo, especially in places where it's hard to change them

+1 on this. I still would recommend namespacing it though, so PillowFoo, at least for the internal errors (PillowReadError). I'm not too fussed.

Here are the error types I encountered while doing this:

  • A plugin found unexpected/corrupt data while reading an otherwise-correct file. This is sometimes ignorable.
  • A plugin found unexpected/incorrect data or instructions while writing a file. This is sometimes ignorable.
  • A plugin does not support valid or potentially-valid data it found in a file (unsupported mode, palette count, etc)
  • A plugin does not support a certain write operation on its file type
  • A plugin could not identify a file as a type it can read.
  • A plugin is not supported (disabled/missing dependency).
  • Pillow could not pair a given file with a plugin. In other words, its type has not been guessed.

They more or less match up with what you described.

I admittedly don't have the time to improve this further, and given how much it breaks backwards compatibility I don't expect it to be merged any time soon anyway, but I definitely want to see the general improvements land at some point. The pillow API feels atrocious to use and this is one of the big reasons.

jleclanche avatar Jan 17 '17 11:01 jleclanche

I think that some of the trouble you were having with the API was shoehorning a pure python decoder into stage 2 of the the three stage accept/open/load pipeline. There's a PR languishing that adds a decoder registry so that the decoder can be hooked in at the appropriate stage. (it's stalled because the plugin it was reorganizing got redone in c first)

wiredfool avatar Jan 17 '17 12:01 wiredfool

@wiredfool The PIL API just has a ton more problems than that. Lots of different ways to do the same thing. No consistency between different plugins, some of which are decades old and don't even work anymore.

It makes it really, really hard to write a new plugin. Because there is no formal high quality documentation on it, you end up picking a plugin at random and copying that... crossing your fingers that the one you're copying is good.

This was really apparent when writing this PR as I had to look through most of the plugins one by one and saw troves of copypasted code all over the place. Duplicated logic, etc.

And I do not mean to diminish your work, I'm well aware most of this is inherited cruft from the PIL codebase. But the quality of the API is extremely poor, making it really hard to contribute; and the quality of the code is really poor, making it even harder to improve the former.

jleclanche avatar Jan 17 '17 12:01 jleclanche