Type annotations

[0xabu]

This PR adds type annotations and mypy checks to pdfminer. This can catch bugs early, and helps developer productivity -- besides serving as documentation, it also enables a much richer experience with autocompletion, type hints, intellisense, etc. in modern editors.

Many of the annotations are non-trivial (e.g. unions, generics, and more use of Any than one might prefer) because much of pdfminer is highly dynamic in its use of types, and the codebase has lots of sites where multiple types are possible (partly due to PDF parsing, and partly as a result of Python2 legacy, e.g. with str and bytes).

My general strategy to dealing with type errors raised by mypy was:

• If the code would obviously crash at runtime, add an assertion
  (e.g. assert x is not None)
• Otherwise, if there's an unchecked conversion between object types
  (that might be violated, e.g., by a non-conforming PDF file), or if
  there's a valid conversion that mypy cannot recognise (e.g. the size of
  tuples returned by chop and friends, or the type of struct.unpack), add
  a cast. This has no runtime effect, but serves as documentation that
  something potentially-fishy is going on. With some refactoring and stronger
  annotations, some of these casts could be removed in the future.
• Otherwise, add a # type: ignore comment to silence the issue -- the goal
  should eventually be to remove these as annotations expand to cover
  more of the code.

Related: issue #362.

[0xabu]

@pietermarsman, @jstockwin -- this got far out of hand from what I originally intended in scope (I've now annotated nearly the entire codebase), but feels like it is ready for review and discussion. There's plenty of scope for changes, but I'm hoping the value is obvious just from the number of issues it has already uncovered (see comments in the PR).

[pietermarsman]

Impressive work! 👍 This should help a lot to keep pdfminer.six maintainable and bug free (well... less bugs).

I only had time for a shallow review. I want to look at it thoroughly before merging.

[pietermarsman]

Can you add a test case that shows this crashes? If it does, we can change this code such that it doesn't.

[0xabu]

Ok. Note to self:

$ tools/dumppdf.py -o out -a --raw-stream samples/simple1.pdf
Traceback (most recent call last):
  File "tools/dumppdf.py", line 388, in <module>
    main()
  File "tools/dumppdf.py", line 378, in main
    dumppdf(
  File "tools/dumppdf.py", line 259, in dumppdf
    dumpallobjs(outfp, doc, codec, show_fallback_xref)
  File "tools/dumppdf.py", line 133, in dumpallobjs
    dumpxml(out, obj, codec=codec)
  File "tools/dumppdf.py", line 65, in dumpxml
    out.write(obj.get_rawdata())  # type: ignore [arg-type]
TypeError: write() argument must be str, not bytes

$ tools/dumppdf.py -o out -a --binary-stream samples/simple1.pdf
Traceback (most recent call last):
  File "tools/dumppdf.py", line 388, in <module>
    main()
  File "tools/dumppdf.py", line 378, in main
    dumppdf(
  File "tools/dumppdf.py", line 259, in dumppdf
    dumpallobjs(outfp, doc, codec, show_fallback_xref)
  File "tools/dumppdf.py", line 133, in dumpallobjs
    dumpxml(out, obj, codec=codec)
  File "tools/dumppdf.py", line 68, in dumpxml
    out.write(obj.get_data())  # type: ignore [arg-type]
TypeError: write() argument must be str, not bytes

[0xabu]

I've added a test for this (which fails), but it's hard to tell what the expected output actually is here -- both paths were trying to write raw binary data into the middle of an XML file. We could reach around the file and write raw bytes there, but would that be useful? We could choose a codec like base64, but would that be useful? I'm tempted to mark this as a known issue and leave it for someone else to decide how to fix it :)

[pietermarsman]

Yep, indeed. Let's not make this bigger than it already is.

All the comments and type ignores are a big improvement, explicitly showing were we can improve.

[pietermarsman]

@0xabu Thanks for this big effort! I'm going to check the code again in a while (when I'm less drowsy) and merge it if everything looks good.

[jstockwin]

I've not had a chance to look at everything in detail, but the bits I've looked at LGTM. Thanks for working on this, it's a nice improvement! 🎉

[pietermarsman]

Could not spot any potential problems.

But to be honest, if there is a potential problem, I'm not sure I would find it. There are so many changes.

But since mypy is happy (that's a good sign! 😄 ) and the test are happy, and both I and @jstockwin and you are happy, I'm pretty confident it will work.

[pietermarsman]

Thanks @0xabu for all this work!

[0xabu]

No worries. Hopefully it makes life easier in the future. I have a few improvements of my own I can now tackle.

[htInEdin]

Um, there are some unqualified dependencies on typing_extensions:

>: ack typing_extensions --python
tools/pdf2txt.py
8:from typing_extensions import Literal

pdfminer/utils.py
10:from typing_extensions import Literal

I guess these should be wrapped up like this?:

if sys.version_info < (3,8):
  from typing_extensions import Literal
else:
  from typing import Literal

And a note in the README.md that 3.5--3.7 have a dependency on typing_extensions?

[pietermarsman]

Found the culprit: typing-extensions is a dependency of mypy and it is installed during the cicd tests. Will create a fix.

[0xabu]

Darn, I guess we were getting that as an implicit dependency via mypy and I didn't notice.

I think only the use in pdf2txt.py is worth keeping, so I could do as you suggest there, but we probably also have to add typing_extensions as an explicit dependency in setup.py?

https://pypi.org/project/typing-extensions/