Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type hinting a return type as nullable renders None #8603

Closed
ViRuSTriNiTy opened this issue Apr 23, 2023 · 4 comments · Fixed by #8610
Closed

Type hinting a return type as nullable renders None #8603

ViRuSTriNiTy opened this issue Apr 23, 2023 · 4 comments · Fixed by #8610
Labels
Bug 🪲 pyreverse Related to pyreverse component python 3.10
Milestone

Comments

@ViRuSTriNiTy
Copy link
Contributor

ViRuSTriNiTy commented Apr 23, 2023

Bug description

Hi there,

the following code

class Foo:
    def bar() -> int | None:
        pass

produces

image

meaning None is not considered as part of the type hint. My current workaround is to revert back to Optional[T] like

class Foo:
    def bar() -> Optional[int]:
        pass

which correctly produces

image

I think the handling for the new syntax for nullable is missing, which would explain why Optional[T] is working but T | None is not.

Configuration

`pyreverse --version`


pyreverse is included in pylint:
pylint 2.17.2
astroid 2.15.3
Python 3.11.2 (tags/v3.11.2:878ead1, Feb  7 2023, 16:38:35) [MSC v.1934 64 bit (AMD64)]


### Command used

```shell
`pyreverse -o png -m y src/mt5_proxies`

Pylint output

See bug description

Expected behavior

Type hint T | None is correctly handled like Option[T]

Pylint version

Python 3.11.2

OS / Environment

No response

Additional dependencies

No response

@ViRuSTriNiTy ViRuSTriNiTy added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Apr 23, 2023
@ViRuSTriNiTy
Copy link
Contributor Author

It seems that there is a general issue with the X | Y syntax as using int | str produces the same result.

@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 pyreverse Related to pyreverse component python 3.10 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Apr 23, 2023
@ViRuSTriNiTy
Copy link
Contributor Author

Hmm, seems like the output type is involved here too. Using the built-in mermaidjs printer produces the diagram correctly:

image

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Apr 23, 2023

Thank you for the analysis @ViRuSTriNiTy, we probably need to change the rendering to handle the | operator for renderer that do not support it natively. The code to change would be here for plantuml and here for dot. So probably replace the generic pylint.pyreverse.utils.get_annotation_label to something specific to each printer.

ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 23, 2023
…al bar character in annotation labels to ensure it is not treated as field separator of record-based nodes
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 23, 2023
…notation_label_of_return_type` with a variant for testing return type using syntax `X | Y`
@ViRuSTriNiTy
Copy link
Contributor Author

@Pierre-Sassoulas Thanks for pointing me into the right direction.

I think I found the line that produces the wrong output, already added some commits in my fork. It is caused by missing escape handling. The docs of the DOT language mention this requirement in section Record-based Nodes with

Braces, vertical bars and angle brackets must be escaped with a backslash character if you wish them to appear as a literal character.

I will provide a PR to fix this.

ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 23, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.4 milestone Apr 24, 2023
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 24, 2023
…e handling is now part of the printer as it is DOT language specific
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 24, 2023
…d diadefs and inspector tests to also cover nullable patterns
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 24, 2023
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 25, 2023
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 25, 2023
ViRuSTriNiTy added a commit to ViRuSTriNiTy/pylint that referenced this issue Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 pyreverse Related to pyreverse component python 3.10
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants