Complete type stubs for xml.dom.minidom (#6886) by zackw · Pull Request #10879 · python/typeshed

zackw · 2023-10-13T16:02:59Z

This patch completes all the type stubs for xml.dom.minidom. I did not look at the rest of the xml.dom directory except as required to make the typing of minidom complete, so it probably doesn’t completely resolve #6886.

xml.dom.minidom is very poorly documented—its authors seem to have assumed that one could refer to the W3C DOM spec. But at the same time the implementation diverges significantly from the spec, so I do not have complete confidence in all the type annotations. Still, it should be an improvement on what’s there now.

The following commands now pass with no errors when executed from the top of the typeshed tree:

mypy --custom-typeshed-dir . --strict --warn-incomplete-stub \
     -m xml.dom.minidom
stubtest --custom-typeshed-dir . xml.dom.minidom

However, attempting to do the same for xml.dom.minicompat produces the following error:

mypy --custom-typeshed-dir . --strict --warn-incomplete-stub \
     -m xml.dom.minicompat
stdlib/xml/dom/minicompat.pyi:11: error: All bases of a protocol must be protocols  [misc]

I don’t know how to fix this, because in the implementation, EmptyNodeList is not a subclass of NodeList, therefore if I don’t make NodeList a protocol, then xml.dom.minidom.Childless fails stubtest. This patch probably should not land until this is resolved. Any advice you could give would be welcome.

This patch completes all the type stubs for xml.dom.minidom. I did not look at the rest of the xml.dom directory except as required to make the typing of minidom complete, so it probably doesn’t completely resolve python#6886. xml.dom.minidom is very poorly documented—its authors seem to have assumed that one could refer to the W3C DOM spec. But at the same time the implementation diverges significantly from the spec, so I do not have complete confidence in all the type annotations. Still, it should be an improvement on what’s there now. The following commands now pass with no errors when executed from the top of the typeshed tree: ```sh mypy --custom-typeshed-dir . --strict --warn-incomplete-stub \ -m xml.dom.minidom stubtest --custom-typeshed-dir . xml.dom.minidom ``` **However**, attempting to do the same for xml.dom.minicompat produces the following error: ```sh mypy --custom-typeshed-dir . --strict --warn-incomplete-stub \ -m xml.dom.minicompat stdlib/xml/dom/minicompat.pyi:11: error: All bases of a protocol must be protocols [misc] ``` I don’t know how to fix this, because in the implementation, EmptyNodeList is not a subclass of NodeList, therefore if I don’t make NodeList a protocol, then xml.dom.minidom.Childless fails stubtest. This patch probably should not land until this is resolved. Any advice you could give would be welcome.

github-actions · 2023-10-13T16:14:41Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

zackw · 2023-10-13T17:21:05Z

It looks like all the failures are due to the xml.dom.minicompat issue I mentioned up top.

For clarity, this is one of those places where the implementation diverges from the spec. All implementations of the Node abstract interface are required to have a childNodes property that implements the NodeList interface. xml.dom.minicompat defines two concrete implementations of that interface: NodeList, which is a subclass of list, and EmptyNodeList, which is a subclass of tuple. Neither is a subclass of the other. xml.dom.minidom uses xml.dom.minicompat.NodeList for childNodes for some types that implement Node, and xml.dom.minicompat.EmptyNodeList for other types that implement Node.

It ought to be perfectly fine for users of xml.dom.minidom to treat the type of childNodes as always being NodeList, no matter what. That's how the spec designers intended it to be used, and I can get xml.dom.minidom to typecheck correctly with

from xml.dom.minicompat import NodeList
class Node:
    childNodes: NodeList

if I make xml.dom.minicompat.NodeList be a Protocol in the type stubs. But tuple, list, and Sequence are not Protocols, and so that same change breaks the typechecking of minicompat itself. I can't change the relationship between EmptyNodeList and NodeList unilaterally from the stubs -- that would require a change to the actual stdlib. I could put minicompat back the way it was and have Node be typed like so instead

from xml.dom.minicompat import EmptyNodeList, NodeList
class Node:
    childNodes: NodeList | EmptyNodeList

but then every user of the childNodes property would have to know about EmptyNodeList, which the authors of the stdlib intended to be a pure implementation detail. The entire minicompat module is undocumented in the stdlib. So I don't like that option either.

Again, any advice you could give would be welcome.

hamdanal

Note I am not a maintainer of typeshed, just a regular contributor

This is not a full review, just a few things I noticed after a quick spot check

hamdanal · 2023-10-14T09:23:28Z

 StringTypes: tuple[type[str]]

-class NodeList(list[_T]):
+class NodeList(Sequence[_T], Protocol):


This doesn't look right. At runtime NodeList inherits from list and it is indeed a list and its append method is used elsewhere in the code. Why was it changed?

hamdanal · 2023-10-14T09:26:39Z


    def hasChildNodes(self) -> bool: ...
-    def insertBefore(self, newChild, refChild): ...
+    def insertBefore(self, newChild: _N, refChild: Node) -> _N: ...


There is a if refChild is None check at runtime

Suggested change

def insertBefore(self, newChild: _N, refChild: Node) -> _N: ...

def insertBefore(self, newChild: _N, refChild: Node | None) -> _N: ...

hamdanal · 2023-10-14T09:32:10Z

+    def getUserData(self, key: str) -> object: ...
+    def setUserData(self, key: str, data: object, handler: UserDataHandler) -> object | None: ...


Returning object would require every use of the return value to be guarded by isinstance checks. Any is preferred for return values.

Suggested change

def getUserData(self, key: str) -> object: ...

def setUserData(self, key: str, data: object, handler: UserDataHandler) -> object | None: ...

def getUserData(self, key: str) -> Any: ...

def setUserData(self, key: str, data: object, handler: UserDataHandler) -> Any: ...

hamdanal · 2023-10-14T09:33:43Z


 class Node(xml.dom.Node):
+    parentNode: Node | None
+    childNodes: NodeList


To silence pyright. Need to import Incomplete from _typeshed.

Suggested change

childNodes: NodeList

childNodes: NodeList[Incomplete]

(same for the NodeList annotations below)

hamdanal · 2023-10-14T09:37:33Z

-    def insertBefore(self, newChild, refChild) -> NoReturn: ...
-    def removeChild(self, oldChild) -> NoReturn: ...
+    attributes: None
+    childNodes: NodeList


This is an EmptyNodeList. If the type checker complains you can add a type: ignore comment

Suggested change

childNodes: NodeList

childNodes: EmptyNodeList

hamdanal · 2023-10-14T09:43:16Z

+    def splitText(self, offset: int) -> Self: ...
    def writexml(self, writer: SupportsWrite[str], indent: str = "", addindent: str = "", newl: str = "") -> None: ...
-    def replaceWholeText(self, content): ...
+    def replaceWholeText(self, content: str) -> Self: ...


This returns None if content is falsy.

hamdanal · 2023-10-14T09:45:54Z

-    def removeNamedItemNS(self, namespaceURI: str, localName) -> None: ...
-    def setNamedItem(self, node) -> None: ...
-    def setNamedItemNS(self, node) -> None: ...
+    def getNamedItem(self, name: str) -> Node: ...


This returns None if the node is not found in the sequence

Suggested change

def getNamedItem(self, name: str) -> Node: ...

def getNamedItem(self, name: str) -> Node | None: ...

(same below)

hamdanal · 2023-10-14T09:47:13Z

+    def getNamedItem(self, name: str) -> Node: ...
+    def getNamedItemNS(self, namespaceURI: str, localName: str) -> Node: ...
+    def __getitem__(self, name_or_tuple: tuple[str, str] | str) -> Node: ...
+    def item(self, index: int) -> Node: ...


This explicitly returns None. It is similar in behavior to dict.get()

Suggested change

def item(self, index: int) -> Node: ...

def item(self, index: int) -> Node | None: ...

srittau · 2024-04-06T12:28:39Z

Thanks for contributing! I'm closing this PR for now, because it still fails some tests and has unresolved review feedback after three months of inactivity. If you are still interested, please feel free to open a new PR (or ping us to reopen this one).

JelleZijlstra · 2024-04-06T12:31:51Z

Also thanks to @hamdanal for the useful review!

hamdanal reviewed Oct 14, 2023

View reviewed changes

srittau closed this Apr 6, 2024

	def insertBefore(self, newChild: _N, refChild: Node) -> _N: ...
	def insertBefore(self, newChild: _N, refChild: Node \| None) -> _N: ...

		def getUserData(self, key: str) -> object: ...
		def setUserData(self, key: str, data: object, handler: UserDataHandler) -> object \| None: ...

	def getNamedItem(self, name: str) -> Node: ...
	def getNamedItem(self, name: str) -> Node \| None: ...

	def item(self, index: int) -> Node: ...
	def item(self, index: int) -> Node \| None: ...

Uh oh!

Conversation

zackw commented Oct 13, 2023

Uh oh!

github-actions Bot commented Oct 13, 2023

Uh oh!

zackw commented Oct 13, 2023

Uh oh!

hamdanal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srittau commented Apr 6, 2024

Uh oh!

JelleZijlstra commented Apr 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hamdanal left a comment •

edited

Loading