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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ New error codes:
* Introduce Y059: `Generic[]` should always be the last base class, if it is
present in the bases of a class.
* Introduce Y060, which flags redundant inheritance from `Generic[]`.
* Introduce Y061: Do not use `None` inside a `Literal[]` slice.
For example, use `Literal["foo"] | None` instead of `Literal["foo", None]`.

Other changes:
* The undocumented `pyi.__version__` and `pyi.PyiTreeChecker.version`
Expand Down
1 change: 1 addition & 0 deletions ERRORCODES.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ The following warnings are currently emitted by default:
| Y058 | Use `Iterator` rather than `Generator` as the return value for simple `__iter__` methods, and `AsyncIterator` rather than `AsyncGenerator` as the return value for simple `__aiter__` methods. Using `(Async)Iterator` for these methods is simpler and more elegant, and reflects the fact that the precise kind of iterator returned from an `__iter__` method is usually an implementation detail that could change at any time, and should not be relied upon.
| Y059 | `Generic[]` should always be the last base class, if it is present in a class's bases tuple. At runtime, if `Generic[]` is not the final class in a the bases tuple, this [can cause the class creation to fail](https://github.com/python/cpython/issues/106102). In a stub file, however, this rule is enforced purely for stylistic consistency.
| Y060 | Redundant inheritance from `Generic[]`. For example, `class Foo(Iterable[_T], Generic[_T]): ...` can be written more simply as `class Foo(Iterable[_T]): ...`.<br><br>To avoid false-positive errors, and to avoid complexity in the implementation, this check is deliberately conservative: it only looks at classes that have exactly two bases.
| Y061 | Do not use `None` inside a `Literal[]` slice. For example, use `Literal["foo"] \| None` instead of `Literal["foo", None]`. While both are legal according to [PEP 586](https://peps.python.org/pep-0586/), the former is preferred for stylistic consistency.

## Warnings disabled by default

Expand Down
22 changes: 21 additions & 1 deletion pyi.py
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ def visit_Subscript(self, node: ast.Subscript) -> None:
self.visit(subscripted_object)
if subscripted_object_name == "Literal":
with self.string_literals_allowed.enabled():
self.visit(node.slice)
self._visit_typing_Literal(node)
return

if isinstance(node.slice, ast.Tuple):
Expand All @@ -1369,6 +1369,25 @@ def visit_Subscript(self, node: ast.Subscript) -> None:
if subscripted_object_name in {"tuple", "Tuple"}:
self._Y090_error(node)

def _visit_typing_Literal(self, node: ast.Subscript) -> None:
if isinstance(node.slice, ast.Constant) and _is_None(node.slice):
# Special case for `Literal[None]`
self.error(node.slice, Y061.format(suggestion="None"))
elif isinstance(node.slice, ast.Tuple):
elts = node.slice.elts
for i, elt in enumerate(elts):
if _is_None(elt):
elts_without_none = elts[:i] + elts[i + 1 :]
if len(elts_without_none) == 1:
new_literal_slice = unparse(elts_without_none[0])
else:
new_slice_node = ast.Tuple(elts=elts_without_none)
new_literal_slice = unparse(new_slice_node).strip("()")
suggestion = f"Literal[{new_literal_slice}] | None"
self.error(elt, Y061.format(suggestion=suggestion))
break # Only report the first `None`
self.visit(node.slice)

def _visit_slice_tuple(self, node: ast.Tuple, parent: str | None) -> None:
if parent == "Union":
self._check_union_members(node.elts, is_pep_604_union=False)
Expand Down Expand Up @@ -2180,6 +2199,7 @@ def parse_options(options: argparse.Namespace) -> None:
'Y060 Redundant inheritance from "Generic[]"; '
"class would be inferred as generic anyway"
)
Y061 = 'Y061 None inside "Literal[]" expression. Replace with "{suggestion}"'
Y090 = (
'Y090 "{original}" means '
'"a tuple of length 1, in which the sole element is of type {typ!r}". '
Expand Down
5 changes: 5 additions & 0 deletions tests/literals.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from typing import Literal

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 :)