From d41f994093ee9b77b2d067d4188432309a9c7698 Mon Sep 17 00:00:00 2001 From: JuanPBedoya Date: Wed, 15 Jul 2026 00:42:12 -0500 Subject: [PATCH 1/2] Fix graphviz_draw return type annotation and docstring graphviz_draw was annotated and documented as returning the PIL.Image module instead of the PIL.Image.Image class. Because graphviz.py does `from PIL import Image`, `Image` is the module, so `-> Image | None` annotated the return with a module rather than the image class. Correct the runtime annotation to `Image.Image | None` and the `:returns:`/`:rtype:` docstring to `PIL.Image.Image`. The stub (graphviz.pyi) already imported the class and was correct, so it is left unchanged. Add tests/visualization/test_graphviz_typing.py with two guards: - a runtime check (get_type_hints + docstring) for the implementation, which is what actually regressed here since the stub shadows the implementation for static type checkers; and - a static assert_type check for every stub overload, wired into the `stubs` nox session via mypy. Fixes #1357 --- noxfile.py | 3 + ...viz-draw-return-type-6b1d3f9c2a7e8f01.yaml | 10 ++ rustworkx/visualization/graphviz.py | 6 +- tests/visualization/test_graphviz_typing.py | 113 ++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml create mode 100644 tests/visualization/test_graphviz_typing.py diff --git a/noxfile.py b/noxfile.py index 5a366d8aff..46a70bb495 100644 --- a/noxfile.py +++ b/noxfile.py @@ -92,3 +92,6 @@ def stubs(session): session.install(*stubs_deps) session.chdir("tests") session.run("python", "-m", "mypy.stubtest", "--concise", "rustworkx", "--allowlist", "stubs_allowlist.txt") + # Static typing regression tests (validate call-site type inference of the + # public stubs, e.g. graphviz_draw's return type, see issue #1357). + session.run("python", "-m", "mypy", "visualization/test_graphviz_typing.py") diff --git a/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml b/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml new file mode 100644 index 0000000000..1a7b608035 --- /dev/null +++ b/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + Fixed the return type annotation and docstring of + :func:`~rustworkx.visualization.graphviz_draw`. It was incorrectly + documented as returning a ``PIL.Image`` (the module) instead of a + ``PIL.Image.Image`` (the class), which caused static type checkers such as + ``pyright`` to infer the wrong return type. Refer to + `#1357 `__ for more + details. diff --git a/rustworkx/visualization/graphviz.py b/rustworkx/visualization/graphviz.py index d4d5d5bba9..ed3b6de781 100644 --- a/rustworkx/visualization/graphviz.py +++ b/rustworkx/visualization/graphviz.py @@ -82,7 +82,7 @@ def graphviz_draw( filename: str | None = None, image_type: str | None = None, method: str | None = None, -) -> Image | None: +) -> Image.Image | None: """Draw a :class:`~rustworkx.PyGraph` or :class:`~rustworkx.PyDiGraph` object using graphviz @@ -133,11 +133,11 @@ def graphviz_draw( more details on the different layout methods. By default ``'dot'`` is used. - :returns: A ``PIL.Image`` object of the generated visualization, if + :returns: A ``PIL.Image.Image`` object of the generated visualization, if ``filename`` is not specified. If ``filename`` is specified then ``None`` will be returned as the visualization was written to the path specified in ``filename`` - :rtype: PIL.Image + :rtype: PIL.Image.Image .. jupyter-execute:: diff --git a/tests/visualization/test_graphviz_typing.py b/tests/visualization/test_graphviz_typing.py new file mode 100644 index 0000000000..b00729f905 --- /dev/null +++ b/tests/visualization/test_graphviz_typing.py @@ -0,0 +1,113 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Typing regression tests for ``graphviz_draw`` (issue #1357). + +Issue #1357 was that ``graphviz_draw`` was annotated/documented as returning +the ``PIL.Image`` *module* instead of the ``PIL.Image.Image`` *class*. This +file guards against that regression on two independent fronts: + +* ``TestGraphvizDrawReturnTypeAnnotation`` -- a **runtime** guard over the + implementation's own ``__annotations__`` and docstring. This is the part that + actually catches the reported bug: the ``graphviz.pyi`` stub shadows + ``graphviz.py`` for static type checkers, so a wrong annotation in the + implementation is invisible to mypy/pyright but is exercised here via + ``typing.get_type_hints``. Before the fix, the buggy ``Image | None`` + annotation makes ``get_type_hints`` raise ``TypeError`` (``module | None``). + +* the ``TYPE_CHECKING`` block -- a **static** guard over the public *stub*, + validated by mypy/pyright (see the ``stubs`` nox session). It exercises every + overload so all return-type branches stay correct. +""" + +from __future__ import annotations + +import inspect +import unittest +from typing import TYPE_CHECKING, get_args, get_type_hints + +import rustworkx +from rustworkx.visualization import graphviz_draw + +if TYPE_CHECKING: + # Type checkers only analyse this branch, so ``PILImageClass`` is always + # bound to the class here (avoids "possibly unbound"/reassignment errors). + from PIL.Image import Image as PILImageClass + + HAS_PIL = True +else: + try: + from PIL.Image import Image as PILImageClass + + HAS_PIL = True + except ImportError: + PILImageClass = None + HAS_PIL = False + + +@unittest.skipUnless(HAS_PIL, "pillow is required for these tests") +class TestGraphvizDrawReturnTypeAnnotation(unittest.TestCase): + """Runtime guard for the implementation's return type (issue #1357).""" + + def test_return_annotation_resolves_to_image_class(self) -> None: + # get_type_hints evaluates the (stringized) annotation in the module + # namespace. With the bug it references the PIL.Image *module* and + # ``module | None`` raises TypeError; the fixed annotation resolves to + # the PIL.Image.Image *class*. + hints = get_type_hints(graphviz_draw) + return_args = get_args(hints["return"]) + self.assertIn(PILImageClass, return_args) + # None is a valid branch (returned when ``filename`` is provided). + self.assertIn(type(None), return_args) + # Guard specifically against regressing back to the module: no union + # member should be a module (the pre-fix ``PIL.Image`` bug). + self.assertFalse(any(inspect.ismodule(arg) for arg in return_args)) + + def test_rtype_docstring_uses_image_class(self) -> None: + doc = graphviz_draw.__doc__ or "" + self.assertIn(":rtype: PIL.Image.Image", doc) + # The pre-fix docstring had a bare ``:rtype: PIL.Image`` (module). + self.assertNotIn(":rtype: PIL.Image\n", doc) + + +if TYPE_CHECKING: + # Static guard for the public *stub* (rustworkx/visualization/graphviz.pyi). + # Validated by mypy/pyright, never executed at runtime, so it needs neither + # graphviz nor Pillow installed. + from PIL.Image import Image + from typing_extensions import assert_type + + graph: rustworkx.PyGraph[int, int] = rustworkx.PyGraph() + digraph: rustworkx.PyDiGraph[int, int] = rustworkx.PyDiGraph() + + def _check_overload_return_types() -> None: + # --- filename omitted / None -> returns a PIL.Image.Image instance --- + assert_type(graphviz_draw(graph), Image) + assert_type(graphviz_draw(digraph), Image) + # image_type as a known Literal member + assert_type(graphviz_draw(graph, image_type="png"), Image) + # image_type as an arbitrary str (matches the broad str overload) + arbitrary_type: str = "png" + assert_type(graphviz_draw(graph, image_type=arbitrary_type), Image) + # method kwarg does not change the return type + assert_type(graphviz_draw(graph, method="neato"), Image) + + # --- filename provided -> writes to disk and returns None --- + assert_type(graphviz_draw(graph, filename="out.png"), None) + assert_type(graphviz_draw(digraph, filename="out.png"), None) + assert_type(graphviz_draw(graph, filename="out.png", image_type="svg"), None) + arbitrary_filename: str = "out.png" + assert_type(graphviz_draw(graph, filename=arbitrary_filename), None) + + +if __name__ == "__main__": + unittest.main() From 8a30f35652e2aad4aa98ab75282d8b3a762f722b Mon Sep 17 00:00:00 2001 From: Ivan Carvalho Date: Sun, 19 Jul 2026 22:14:10 -0400 Subject: [PATCH 2/2] Remove poorly written tests and keep release notes minimal --- noxfile.py | 3 - ...viz-draw-return-type-6b1d3f9c2a7e8f01.yaml | 10 +- tests/visualization/test_graphviz_typing.py | 113 ------------------ 3 files changed, 3 insertions(+), 123 deletions(-) delete mode 100644 tests/visualization/test_graphviz_typing.py diff --git a/noxfile.py b/noxfile.py index 46a70bb495..5a366d8aff 100644 --- a/noxfile.py +++ b/noxfile.py @@ -92,6 +92,3 @@ def stubs(session): session.install(*stubs_deps) session.chdir("tests") session.run("python", "-m", "mypy.stubtest", "--concise", "rustworkx", "--allowlist", "stubs_allowlist.txt") - # Static typing regression tests (validate call-site type inference of the - # public stubs, e.g. graphviz_draw's return type, see issue #1357). - session.run("python", "-m", "mypy", "visualization/test_graphviz_typing.py") diff --git a/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml b/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml index 1a7b608035..23e4ffee79 100644 --- a/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml +++ b/releasenotes/notes/fix-graphviz-draw-return-type-6b1d3f9c2a7e8f01.yaml @@ -1,10 +1,6 @@ --- fixes: - | - Fixed the return type annotation and docstring of - :func:`~rustworkx.visualization.graphviz_draw`. It was incorrectly - documented as returning a ``PIL.Image`` (the module) instead of a - ``PIL.Image.Image`` (the class), which caused static type checkers such as - ``pyright`` to infer the wrong return type. Refer to - `#1357 `__ for more - details. + Fixed the return type annotation of + :func:`~rustworkx.visualization.graphviz_draw` to use ``PIL.Image.Image`` + instead of ``PIL.Image``. diff --git a/tests/visualization/test_graphviz_typing.py b/tests/visualization/test_graphviz_typing.py deleted file mode 100644 index b00729f905..0000000000 --- a/tests/visualization/test_graphviz_typing.py +++ /dev/null @@ -1,113 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - -"""Typing regression tests for ``graphviz_draw`` (issue #1357). - -Issue #1357 was that ``graphviz_draw`` was annotated/documented as returning -the ``PIL.Image`` *module* instead of the ``PIL.Image.Image`` *class*. This -file guards against that regression on two independent fronts: - -* ``TestGraphvizDrawReturnTypeAnnotation`` -- a **runtime** guard over the - implementation's own ``__annotations__`` and docstring. This is the part that - actually catches the reported bug: the ``graphviz.pyi`` stub shadows - ``graphviz.py`` for static type checkers, so a wrong annotation in the - implementation is invisible to mypy/pyright but is exercised here via - ``typing.get_type_hints``. Before the fix, the buggy ``Image | None`` - annotation makes ``get_type_hints`` raise ``TypeError`` (``module | None``). - -* the ``TYPE_CHECKING`` block -- a **static** guard over the public *stub*, - validated by mypy/pyright (see the ``stubs`` nox session). It exercises every - overload so all return-type branches stay correct. -""" - -from __future__ import annotations - -import inspect -import unittest -from typing import TYPE_CHECKING, get_args, get_type_hints - -import rustworkx -from rustworkx.visualization import graphviz_draw - -if TYPE_CHECKING: - # Type checkers only analyse this branch, so ``PILImageClass`` is always - # bound to the class here (avoids "possibly unbound"/reassignment errors). - from PIL.Image import Image as PILImageClass - - HAS_PIL = True -else: - try: - from PIL.Image import Image as PILImageClass - - HAS_PIL = True - except ImportError: - PILImageClass = None - HAS_PIL = False - - -@unittest.skipUnless(HAS_PIL, "pillow is required for these tests") -class TestGraphvizDrawReturnTypeAnnotation(unittest.TestCase): - """Runtime guard for the implementation's return type (issue #1357).""" - - def test_return_annotation_resolves_to_image_class(self) -> None: - # get_type_hints evaluates the (stringized) annotation in the module - # namespace. With the bug it references the PIL.Image *module* and - # ``module | None`` raises TypeError; the fixed annotation resolves to - # the PIL.Image.Image *class*. - hints = get_type_hints(graphviz_draw) - return_args = get_args(hints["return"]) - self.assertIn(PILImageClass, return_args) - # None is a valid branch (returned when ``filename`` is provided). - self.assertIn(type(None), return_args) - # Guard specifically against regressing back to the module: no union - # member should be a module (the pre-fix ``PIL.Image`` bug). - self.assertFalse(any(inspect.ismodule(arg) for arg in return_args)) - - def test_rtype_docstring_uses_image_class(self) -> None: - doc = graphviz_draw.__doc__ or "" - self.assertIn(":rtype: PIL.Image.Image", doc) - # The pre-fix docstring had a bare ``:rtype: PIL.Image`` (module). - self.assertNotIn(":rtype: PIL.Image\n", doc) - - -if TYPE_CHECKING: - # Static guard for the public *stub* (rustworkx/visualization/graphviz.pyi). - # Validated by mypy/pyright, never executed at runtime, so it needs neither - # graphviz nor Pillow installed. - from PIL.Image import Image - from typing_extensions import assert_type - - graph: rustworkx.PyGraph[int, int] = rustworkx.PyGraph() - digraph: rustworkx.PyDiGraph[int, int] = rustworkx.PyDiGraph() - - def _check_overload_return_types() -> None: - # --- filename omitted / None -> returns a PIL.Image.Image instance --- - assert_type(graphviz_draw(graph), Image) - assert_type(graphviz_draw(digraph), Image) - # image_type as a known Literal member - assert_type(graphviz_draw(graph, image_type="png"), Image) - # image_type as an arbitrary str (matches the broad str overload) - arbitrary_type: str = "png" - assert_type(graphviz_draw(graph, image_type=arbitrary_type), Image) - # method kwarg does not change the return type - assert_type(graphviz_draw(graph, method="neato"), Image) - - # --- filename provided -> writes to disk and returns None --- - assert_type(graphviz_draw(graph, filename="out.png"), None) - assert_type(graphviz_draw(digraph, filename="out.png"), None) - assert_type(graphviz_draw(graph, filename="out.png", image_type="svg"), None) - arbitrary_filename: str = "out.png" - assert_type(graphviz_draw(graph, filename=arbitrary_filename), None) - - -if __name__ == "__main__": - unittest.main()