Type-hints: update to pass mypy checks with types-docutils 0.21.0.20240704 #12511

jayaddison · 2024-07-04T14:50:10Z

Feature or Bugfix

Bugfix / maintenance

Purpose

A recent update to types-docutils has resulted in some mypy linting failures; this changeset attempts to address those.

Detail

This was less trivial than I expected; here are some of the resolved and unresolved questions I've encountered along the way:

In the case of an attribute warning about Reporter.get_source_and_line, the mypy type warning is ignored; the relevant attribute is added at runtime by docutils.
Some generic type arguments are added to RSTState typehints. These could be declared in a single location and then re-used using import. Is that worthwhile in this case? I'm not sure.
- Update: the generic type arguments have been removed in the upstream types-docutils, and RSTState moved into sphinx.utils.typing for convenience without any generic-binding divergence risk, so this is resolved.
⚠️ A behaviour change: self.state.inliner is passed to a callee that expects an Inliner type argument instead of the previous self.state value. I'm not 100% certain whether this is correct; some more code archaeology (perhaps in docutils too, in case the interface has changed) may be necessary.
In the tests, there is a make_directive_and_state function with the signature: def make_directive_and_state(*, env: SimpleNamespace, input_lines: StringList | None = None) -> tuple[RSTState, SphinxDirective]:. Deciding on a generic type argument for the RSTState entry in the return-tuple-type spec seems tricky. As an interim measure I've placed Any in there.
- This is (also) resolved and much simplified by the removal of generic type arguments to RSTState in the upstream typestub library.

Relates

Resolves Linting failures with mypy from types-docutils 0.21.0.20240704 #12510.

…m 2024-07-04. * Reporter.get_source_and_line is added at runtime by the ``RSTState.runtime_init`` method. * Pass the RST parser state's ``Inliner`` when calling class roles (could this reflect an interface change, or is it a bug?). * Add a RST-parser-context generic type argument to ``RSTState`` type-hints.

A TypeVar is _not_ added here; the RSTState's generic type argument would only appear in a function return signature, and I'm not sure I can figure out what it _should_ be based on the way an RSTState object is instantiated. Relates-to commit 1b04ddb.

…g from commit 1b04ddb.

jayaddison · 2024-07-04T15:43:59Z

Ah: I had made an incorrect guess that it was the behaviour-change noted in the pull request description that caused the failures that appear in the Render documentation pipelines. Instead, it seems to be this new warning:

/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/sphinx/util/parsing.py:docstring of sphinx.util.parsing.nested_parse_to_nodes:1: WARNING: py:class reference target not found: _RSTContext [ref.class]

...so that's clearly related somehow to the introduction of one of the TypeVars instead.

…r-passing from commit 1b04ddb." This reverts commit 8fc3a85.

jayaddison · 2024-07-04T15:50:23Z

Ah: I had made an incorrect guess that it was the behaviour-change noted in the pull request description that caused the failures that appear in the Render documentation pipelines. Instead, it seems to be this new warning:
/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/sphinx/util/parsing.py:docstring of sphinx.util.parsing.nested_parse_to_nodes:1: WARNING: py:class reference target not found: _RSTContext [ref.class]
...so that's clearly related somehow to the introduction of one of the TypeVars instead.

This seems to be #10974 (or, as it mentions - in fact the same problem reported in #10785).

…b04ddb with ``Any`` generic type arg.

jayaddison · 2024-07-04T16:03:57Z

sphinx/util/nodes.py

+_RSTContext = TypeVar("_RSTContext")
+
+
+def nested_parse_with_titles(state: RSTState[_RSTContext], content: StringList, node: Node,


Why didn't the same WARNING: py:class reference target not found: _RSTContext [ref.class] failure occur here?

Ah -- perhaps it's because of the :param state: line in the docstring of the former case; this function doesn't list parameters within its docstring?

However, it also seems that this function isn't listed in the generated documentation. That could be another reason that the warning doesn't appear.

You don't need to have a type variable here. Just use a simple Any. Why? because you don't use that specific type variable. A type variable without any bound or constraint is equivalent to Any.

jayaddison · 2024-07-04T16:15:55Z

There are some unsolved mysteries in here if anyone has time to help investigate; as-is, this would allow unblocking the mypy lint pipeline though. I'd be OK with that, although it would be nice to figure out some more of the details.

sphinx/util/parsing.py

picnixz · 2024-07-06T12:59:53Z

sphinx/util/nodes.py

+_RSTContext = TypeVar("_RSTContext")
+
+
+def nested_parse_with_titles(state: RSTState[_RSTContext], content: StringList, node: Node,


You don't need to have a type variable here. Just use a simple Any. Why? because you don't use that specific type variable. A type variable without any bound or constraint is equivalent to Any.

sphinx/ext/inheritance_diagram.py

…he type is unused/unbound. Ref: sphinx-doc#12511 (comment)

AA-Turner · 2024-07-06T18:18:50Z

I agree the genericness is annoying to deal with -- I understand that @danieleades added it to account for the context parameter of StateMachine.run(), but I think context ought really be list[str] | None -- see e.g. the Text state and its subclasses.

A

danieleades · 2024-07-06T18:21:46Z

I definitely wouldn't rule out that fixing this properly requires some upstream fixes in typeshed

AA-Turner · 2024-07-06T18:23:43Z

Actually: see RSTState.bof() -- this unconditionally returns [] for the context parameter, so regardless of any other generic parameters, RSTState's context is always list (and more precisely, list[str]).

https://github.com/docutils/docutils/blob/d3444d5baf9c1da49fd116c3916817d0e082b47f/docutils/docutils/parsers/rst/states.py#L251-L253

https://github.com/docutils/docutils/blob/d3444d5baf9c1da49fd116c3916817d0e082b47f/docutils/docutils/statemachine.py#L221

A

jayaddison · 2024-07-06T19:12:35Z

Thank you @AA-Turner @danieleades.

cc @adamtheturtle for your consideration (ref: python/typeshed#12226 )

danieleades · 2024-07-06T19:20:29Z

Should the context be generic in RSTState at all? Or should it be concrete in this subclass?

AA-Turner · 2024-07-06T21:01:18Z

Should the context be generic in RSTState at all?

No, it should be list[str]. Arguably it could still be generic in statemachine, but I can only find uses of it as None (the default) or list[str] (in parsers.rst.states).

danieleades · 2024-07-07T07:01:49Z

Should the context be generic in RSTState at all?

No, it should be list[str]. Arguably it could still be generic in statemachine, but I can only find uses of it as None (the default) or list[str] (in parsers.rst.states).

hang on, does that mean it should be list[str] | None?

see python/typeshed#12291

jayaddison · 2024-07-11T12:43:49Z

There are some unsolved mysteries in here if anyone has time to help investigate; as-is, this would allow unblocking the mypy lint pipeline though. I'd be OK with that, although it would be nice to figure out some more of the details.

Thanks all for the help; please consider all the mysteries here resolved!

jayaddison added 3 commits July 4, 2024 15:28

Type-hints: revert behaviour-change in state.inliner parameter-passin…

8fc3a85

…g from commit 1b04ddb.

Revert "Type-hints: revert behaviour-change in state.inliner paramete…

e2e59bc

…r-passing from commit 1b04ddb." This reverts commit 8fc3a85.

Type-hints: workaround: partially-replace typevar usage from commit 1…

4ff9627

…b04ddb with ``Any`` generic type arg.

jayaddison commented Jul 4, 2024

View reviewed changes

picnixz reviewed Jul 6, 2024

View reviewed changes

jayaddison and others added 3 commits July 6, 2024 16:57

Code review feedback: replace TypeVar with typing.Any since t…

3378b0b

…he type is unused/unbound. Ref: sphinx-doc#12511 (comment)

Fixup: remove extraneous empty-line.

7bc74ad

Use a generic in util.typing

c31b252

Use list[str]

3f77102

AA-Turner merged commit e63e2f3 into sphinx-doc:master Jul 6, 2024
23 checks passed

jayaddison deleted the issue-12510/docutils-20240704-typehint-updates branch July 6, 2024 19:12

danieleades mentioned this pull request Jul 7, 2024

remove generic 'context' from 'docutils.parsers.rst.RSTState' python/typeshed#12291

Merged

jayaddison mentioned this pull request Jul 9, 2024

Update MyPy for types-docutils 0.21.0.20240708 #12522

Merged

AA-Turner added this to the 7.4.0 milestone Jul 13, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type-hints: update to pass mypy checks with types-docutils 0.21.0.20240704 #12511

Type-hints: update to pass mypy checks with types-docutils 0.21.0.20240704 #12511

jayaddison commented Jul 4, 2024 •

edited

Loading

jayaddison commented Jul 4, 2024

jayaddison commented Jul 4, 2024

jayaddison Jul 4, 2024

jayaddison Jul 4, 2024

picnixz Jul 6, 2024

jayaddison commented Jul 4, 2024

picnixz Jul 6, 2024

AA-Turner commented Jul 6, 2024

danieleades commented Jul 6, 2024

AA-Turner commented Jul 6, 2024 •

edited

Loading

jayaddison commented Jul 6, 2024

danieleades commented Jul 6, 2024

AA-Turner commented Jul 6, 2024

danieleades commented Jul 7, 2024 •

edited

Loading

jayaddison commented Jul 11, 2024

		_RSTContext = TypeVar("_RSTContext")


		def nested_parse_with_titles(state: RSTState[_RSTContext], content: StringList, node: Node,

Type-hints: update to pass mypy checks with types-docutils 0.21.0.20240704 #12511

Type-hints: update to pass mypy checks with types-docutils 0.21.0.20240704 #12511

Conversation

jayaddison commented Jul 4, 2024 • edited Loading

Feature or Bugfix

Purpose

Detail

Relates

jayaddison commented Jul 4, 2024

jayaddison commented Jul 4, 2024

jayaddison Jul 4, 2024

Choose a reason for hiding this comment

jayaddison Jul 4, 2024

Choose a reason for hiding this comment

picnixz Jul 6, 2024

Choose a reason for hiding this comment

jayaddison commented Jul 4, 2024

picnixz Jul 6, 2024

Choose a reason for hiding this comment

AA-Turner commented Jul 6, 2024

danieleades commented Jul 6, 2024

AA-Turner commented Jul 6, 2024 • edited Loading

jayaddison commented Jul 6, 2024

danieleades commented Jul 6, 2024

AA-Turner commented Jul 6, 2024

danieleades commented Jul 7, 2024 • edited Loading

jayaddison commented Jul 11, 2024

jayaddison commented Jul 4, 2024 •

edited

Loading

AA-Turner commented Jul 6, 2024 •

edited

Loading

danieleades commented Jul 7, 2024 •

edited

Loading