return type for `lxml.etree.fromstring` should be Optional (?)

DMRobertson commented 2 years ago

At least with lxml 4.8.0, I see the following:

$ python
Python 3.10.4 (main, Mar 25 2022, 00:00:00) [GCC 11.2.1 20220127 (Red Hat 11.2.1-9)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import lxml.etree as etree
>>> parser = etree.HTMLParser(recover=True, encoding="utf-8")
>>> tree = etree.fromstring(b"", parser)
>>> tree
>>> tree is None
True

But the type annotation says we'll always return an _Element:

https://github.com/lxml/lxml-stubs/blob/5eaa0f8723da181192ca3e20b478ebb89588228b/lxml-stubs/etree.pyi#L498-L500

Aside: I'm passing an HTMLParser here instead of an XMLParser. The type stub for fromstring doesn't allow that either, but again I'm not sure if that's a mistake in the annotations.

For context, I came across this whilst trying to use the stubs to typecheck this snippet of Synapse.

scoder commented 2 years ago

The None return value is a rare artefact of using recover=True with no data, but given that it exists, I agreed on both points, thanks for bringing this up. Maybe you can send a PR for these changes? We might actually be lacking a test for something that passes a parser at all. seems worth adding.

DMRobertson commented 2 years ago

I've had a go at this in #64---though I'm not massively familiar with lxml's machinery. Hopefully it's a useful drive-by though.

We might actually be lacking a test for something that passes a parser at all.

For completeness: I interpreted this as a test in the stubs repository here, rather than a test in lxml proper.

lxml / lxml-stubs

return type for `lxml.etree.fromstring` should be Optional (?) #63