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

Enforce consistent use of Literal and None #435

Merged
merged 7 commits into from
Oct 29, 2023

Conversation

tomasr8
Copy link
Contributor

@tomasr8 tomasr8 commented Oct 27, 2023

Closes #424

I wrote the first version using match-case but then I realized that's only for python >=3.10 😅 😭

I'd be happy for feedback! Some notes:

  • I don't handle Optional[Literal["foo"]] since we already have a check for the use of Optional[...] in general
  • I'm not sure if we want a custom message specifically for Literal[None] -> None, currently I use the same message for this case and for cases with multiple values
  • If there are multiple None values e.g. Literal[None, "foo", None], I only report the first. It would be easy enough to collect all of them but then I wasn't sure which node to report (i.e. which part of the source gets the squiggly red line - currently it's the first None). There could even a more general rule which checks for duplicates inside Literal[...] analogous to the one for unions

@github-actions

This comment has been minimized.

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great overall! I think you made the right decisions with regard to all your notes in #435 (comment).

Some small comments below. This will also need an entry in CHANGELOG.md, and an addition to ERRORCODES.md :)

pyi.py Outdated Show resolved Hide resolved
pyi.py Outdated Show resolved Hide resolved
tests/literals.pyi Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions

This comment has been minimized.

@tomasr8 tomasr8 marked this pull request as ready for review October 29, 2023 16:54
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@tomasr8
Copy link
Contributor Author

tomasr8 commented Oct 29, 2023

Thanks a lot for the review! I added a changelog and updated the error codes :)

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two docs nits, and another comment about the duplicate-None thing:

CHANGELOG.md Outdated Show resolved Hide resolved
ERRORCODES.md Outdated Show resolved Hide resolved

Literal[None] # Y061 None inside "Literal[]" expression. Replace with "None"
Literal[True, None] # Y061 None inside "Literal[]" expression. Replace with "Literal[True] | None"
Literal[1, None, "foo", None] # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo', None] | None"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still isn't great, because we're suggesting code that we'd immediately complain about, if the user implemented our suggestion. I think you're right, though, that the best way to deal with this would be to introduce another error code that complains about duplicate members inside a Literal[] slice -- a version of Y016, but for Literal[] slices. We could refrain from emitting Y061 if we knew we'd already emitted that error.

Since this is a really unlikely edge case, I think it would be fine to implement that as a followup PR -- are you interested in doing so? If not, I'm happy to take a look at implementing it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a separate check for duplicate elements is probably the cleanest solution here.

I can't guarantee that I'll have the time to work on the followup in the coming days, so if you'd like to have it done sooner rather than later, go for it :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't guarantee I'll have time in the next few days either, so I guess we'll just have to see who gets to it first 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I hope you can still see the notifications on here 😅 I saw that you've improved the error message for the duplicate None edge case. So I was wondering if we still wanted to add a separate rule for duplicate Literal[] members in general?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiya! Yes, I'd still take that as a PR, I think. We already flag duplicate union members; it seems consistent to also flag duplicate Literal members :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'll try to put something together this weekend :)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions
Copy link

⚠ Flake8 diff showing the effect of this PR on typeshed:

> ./stdlib/_ast.pyi:556:37: Y061 None inside "Literal[]" expression. Replace with "Literal[True, False] | None"
> ./stdlib/asyncio/base_events.pyi:426:30: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/asyncio/events.pyi:525:30: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/asyncio/subprocess.pyi:57:30: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/asyncio/subprocess.pyi:123:30: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/asyncio/subprocess.pyi:188:30: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:251:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:263:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:455:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:467:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:653:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:665:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:832:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:844:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1245:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1256:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1431:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1442:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1611:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1622:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1772:44: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1783:23: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:1993:48: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2000:27: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2178:48: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2185:27: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2357:48: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2364:27: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2517:48: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stdlib/subprocess.pyi:2524:27: Y061 None inside "Literal[]" expression. Replace with "Literal[False] | None"
> ./stubs/Pillow/PIL/EpsImagePlugin.pyi:12:61: Y061 None inside "Literal[]" expression. Replace with "Literal['gswin32c', 'gswin64c', 'gs', False] | None"
> ./stubs/Pillow/PIL/EpsImagePlugin.pyi:13:69: Y061 None inside "Literal[]" expression. Replace with "Literal['gswin32c', 'gswin64c', 'gs', False] | None"
> ./stubs/Pillow/PIL/EpsImagePlugin.pyi:15:37: Y061 None inside "Literal[]" expression. Replace with "Literal['gs', False] | None"
> ./stubs/invoke/invoke/runners.pyi:8:28: Y061 None inside "Literal[]" expression. Replace with "Literal[True, False, 'out', 'stdout', 'err', 'stderr', 'both'] | None"
> ./stubs/openpyxl/openpyxl/cell/cell.pyi:35:69: Y061 None inside "Literal[]" expression. Replace with "Literal['n', 's', 'd', 'f'] | None"
> ./stubs/pyinstaller/PyInstaller/compat.pyi:48:108: Y061 None inside "Literal[]" expression. Replace with "Literal['sw_64', 'loongarch64', 'arm', 'intel', 'ppc', 'mips', 'riscv', 's390x', 'unknown'] | None"
> ./stubs/pyinstaller/PyInstaller/depend/imphookapi.pyi:69:87: Y061 None inside "Literal[]" expression. Replace with "Literal['pyz', 'pyc', 'py', 'pyz+py', 'py+pyz'] | None"
> ./stubs/python-xlib/Xlib/support/unix_connect.pyi:12:36: Y061 None inside "Literal[]" expression. Replace with "Literal['tcp', 'unix', 'darwin'] | None"
> ./stubs/python-xlib/Xlib/support/unix_connect.pyi:16:36: Y061 None inside "Literal[]" expression. Replace with "Literal['tcp', 'unix'] | None"
> ./stubs/qrcode/qrcode/main.pyi:60:49: Y061 None inside "Literal[]" expression. Replace with "None"

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- thanks @tomasr8! :D

@AlexWaygood AlexWaygood merged commit 2f5b26e into PyCQA:main Oct 29, 2023
@tomasr8 tomasr8 deleted the literal-none branch October 29, 2023 18:28
AlexWaygood added a commit that referenced this pull request Nov 15, 2023
Previous discussion for context:
#435 (comment)

Example:
`Literal[True, True, False, False]` - emits Y062 for both `True` and `False`.

This is how the new rule interacts with Y061:
- `Literal[None]`, `Literal[None, None]`, etc.. - Only Y061 is emitted
   and the suggestion is always just `None`
   (previously `Literal[None, None]` would suggest `Literal[] | None`)
- Combination of `None` and other duplicates
   e.g. `Literal[None, True, True]` - only Y062 is emitted
---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardizing Union of Literal and None
2 participants