-
Notifications
You must be signed in to change notification settings - Fork 23
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
Relax Y011, Y014 and Y015 to allow default values in many contexts #326
Merged
Merged
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
abd0c24
Relax Y011, Y014 and Y015 to allow default values in many contexts
AlexWaygood df83a85
Address review and fix bugs
AlexWaygood f5efc88
.
AlexWaygood 9394c62
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 71eed1f
Add regression test for #316
AlexWaygood 1493be3
Merge branch 'default-values' of https://github.com/AlexWaygood/flake…
AlexWaygood 5e589a9
Be stricter in `_is_valid_stub_default`
AlexWaygood ae29c90
Update README.md
AlexWaygood 9e5a0a6
More tests
AlexWaygood 5ac18f0
Merge branch 'default-values' of https://github.com/AlexWaygood/flake…
AlexWaygood 1c607b5
Implement Jelle's suggestion
AlexWaygood c3853e5
Improve
AlexWaygood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,7 +359,6 @@ def _is_object(node: ast.expr | None, name: str, *, from_: Container[str]) -> bo | |
_is_Any = partial(_is_object, name="Any", from_={"typing"}) | ||
_is_overload = partial(_is_object, name="overload", from_=_TYPING_MODULES) | ||
_is_final = partial(_is_object, name="final", from_=_TYPING_MODULES) | ||
_is_Final = partial(_is_object, name="Final", from_=_TYPING_MODULES) | ||
_is_Self = partial(_is_object, name="Self", from_=({"_typeshed"} | _TYPING_MODULES)) | ||
_is_TracebackType = partial(_is_object, name="TracebackType", from_={"types"}) | ||
_is_builtins_object = partial(_is_object, name="object", from_={"builtins"}) | ||
|
@@ -852,39 +851,12 @@ def _check_for_typevarlike_assignments( | |
else: | ||
self.error(node, Y001.format(cls_name)) | ||
|
||
def _Y015_error(self, node: ast.Assign | ast.AnnAssign) -> None: | ||
old_syntax = unparse(node) | ||
copy_of_node = deepcopy(node) | ||
copy_of_node.value = ast.Constant(value=...) | ||
new_syntax = unparse(copy_of_node) | ||
error_message = Y015.format(old_syntax=old_syntax, new_syntax=new_syntax) | ||
self.error(node, error_message) | ||
|
||
@staticmethod | ||
def _Y015_violation_detected(node: ast.Assign | ast.AnnAssign) -> bool: | ||
assignment = node.value | ||
|
||
if isinstance(node, ast.AnnAssign): | ||
if assignment and not isinstance(assignment, ast.Ellipsis): | ||
return True | ||
return False | ||
|
||
if isinstance(assignment, (ast.Num, ast.Str, ast.Bytes)): | ||
return True | ||
if ( | ||
isinstance(assignment, ast.UnaryOp) | ||
and isinstance(assignment.op, ast.USub) | ||
and isinstance(assignment.operand, ast.Num) | ||
): | ||
return True | ||
if ( | ||
isinstance(assignment, (ast.Constant, ast.NameConstant)) | ||
and not isinstance(assignment, ast.Ellipsis) | ||
and assignment.value is not None | ||
): | ||
return True | ||
|
||
return False | ||
def _is_valid_assignment_value(self, node: ast.expr) -> bool: | ||
return ( | ||
isinstance(node, (ast.Call, ast.Name, ast.Attribute, ast.Subscript)) | ||
or (isinstance(node, ast.BinOp) and isinstance(node.op, ast.BitOr)) | ||
or self._is_valid_stub_default(node) | ||
) | ||
|
||
def visit_Assign(self, node: ast.Assign) -> None: | ||
if self.in_function.active: | ||
|
@@ -904,16 +876,15 @@ def visit_Assign(self, node: ast.Assign) -> None: | |
is_special_assignment = _is_assignment_which_must_have_a_value( | ||
target_name, in_class=self.in_class.active | ||
) | ||
if is_special_assignment: | ||
assignment = node.value | ||
if is_special_assignment or isinstance(assignment, ast.Str): | ||
with self.string_literals_allowed.enabled(): | ||
self.generic_visit(node) | ||
else: | ||
self.generic_visit(node) | ||
if target_name is None: | ||
return | ||
assert isinstance(target, ast.Name) | ||
assignment = node.value | ||
|
||
if isinstance(assignment, ast.Call): | ||
function = assignment.func | ||
if _is_TypedDict(function): | ||
|
@@ -925,11 +896,10 @@ def visit_Assign(self, node: ast.Assign) -> None: | |
) | ||
return | ||
|
||
if self._Y015_violation_detected(node): | ||
return self._Y015_error(node) | ||
|
||
if not is_special_assignment: | ||
self._check_for_type_aliases(node, target, assignment) | ||
if not self._is_valid_assignment_value(assignment): | ||
self.error(node, Y015) | ||
|
||
def visit_AugAssign(self, node: ast.AugAssign) -> None: | ||
"""Allow `__all__ += ['foo', 'bar']` in a stub file""" | ||
|
@@ -962,7 +932,11 @@ def _check_for_type_aliases( | |
and `X = None` (special-cased because it is so special). | ||
""" | ||
if ( | ||
isinstance(assignment, (ast.Subscript, ast.BinOp)) | ||
isinstance(assignment, ast.Subscript) | ||
or ( | ||
isinstance(assignment, ast.BinOp) | ||
and isinstance(assignment.op, ast.BitOr) | ||
) | ||
or _is_Any(assignment) | ||
or _is_None(assignment) | ||
): | ||
|
@@ -1028,7 +1002,6 @@ def visit_Expr(self, node: ast.Expr) -> None: | |
_Y043_REGEX = re.compile(r"^_.*[a-z]T\d?$") | ||
|
||
def _check_typealias(self, node: ast.AnnAssign, alias_name: str) -> None: | ||
self.generic_visit(node) | ||
if alias_name.startswith("_"): | ||
self.typealias_decls[alias_name] = node | ||
if self._Y042_REGEX.match(alias_name): | ||
|
@@ -1038,29 +1011,35 @@ def _check_typealias(self, node: ast.AnnAssign, alias_name: str) -> None: | |
|
||
def visit_AnnAssign(self, node: ast.AnnAssign) -> None: | ||
node_annotation = node.annotation | ||
if _is_Final(node_annotation): | ||
with self.string_literals_allowed.enabled(): | ||
self.generic_visit(node) | ||
return | ||
|
||
node_target = node.target | ||
if isinstance(node_target, ast.Name): | ||
target_name = node_target.id | ||
if _is_assignment_which_must_have_a_value( | ||
target_name, in_class=self.in_class.active | ||
): | ||
node_value = node.value | ||
|
||
is_special_assignment = isinstance( | ||
node_target, ast.Name | ||
) and _is_assignment_which_must_have_a_value( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not Black's finest hour |
||
node_target.id, in_class=self.in_class.active | ||
) | ||
|
||
self.visit(node_target) | ||
self.visit(node_annotation) | ||
if node_value is not None: | ||
if isinstance(node_value, ast.Str) or is_special_assignment: | ||
with self.string_literals_allowed.enabled(): | ||
self.generic_visit(node) | ||
if node.value is None: | ||
self.error(node, Y035.format(var=target_name)) | ||
return | ||
self.visit(node_value) | ||
else: | ||
self.visit(node_value) | ||
|
||
if is_special_assignment: | ||
if node_value is None: | ||
assert isinstance(node_target, ast.Name) | ||
self.error(node, Y035.format(var=node_target.id)) | ||
return | ||
|
||
if _is_TypeAlias(node_annotation) and isinstance(node_target, ast.Name): | ||
return self._check_typealias(node=node, alias_name=target_name) | ||
self._check_typealias(node=node, alias_name=node_target.id) | ||
|
||
self.generic_visit(node) | ||
if self._Y015_violation_detected(node): | ||
self._Y015_error(node) | ||
if node_value and not self._is_valid_assignment_value(node_value): | ||
self.error(node, Y015) | ||
|
||
def _check_union_members(self, members: Sequence[ast.expr]) -> None: | ||
first_union_member = members[0] | ||
|
@@ -1695,12 +1674,35 @@ def visit_arguments(self, node: ast.arguments) -> None: | |
if node.kwarg is not None: | ||
self.visit(node.kwarg) | ||
|
||
def _is_valid_stub_default(self, default: ast.expr) -> bool: | ||
# `...`, strings, bytes, ints, floats, complex numbers like `3j`, bools, None | ||
if isinstance( | ||
default, (ast.Ellipsis, ast.Str, ast.Bytes, ast.Num, ast.NameConstant) | ||
): | ||
return True | ||
# Complex numbers such as `4+3j` or `4-3j` | ||
if ( | ||
isinstance(default, ast.BinOp) | ||
and isinstance(default.op, (ast.Add, ast.Sub)) | ||
and self._is_valid_stub_default(default.left) | ||
and isinstance(default.right, ast.Num) | ||
): | ||
return True | ||
# Negative ints, negative floats, complex numbers such as `-4+3j` or `-4-3j` | ||
if ( | ||
isinstance(default, ast.UnaryOp) | ||
and isinstance(default.op, ast.USub) | ||
and isinstance(default.operand, ast.Num) | ||
): | ||
return True | ||
return False | ||
|
||
def check_arg_default(self, arg: ast.arg, default: ast.expr | None) -> None: | ||
self.visit(arg) | ||
if default is not None: | ||
with self.string_literals_allowed.enabled(): | ||
self.visit(default) | ||
if default is not None and not isinstance(default, ast.Ellipsis): | ||
if default is not None and not self._is_valid_stub_default(default): | ||
self.error(default, (Y014 if arg.annotation is None else Y011)) | ||
|
||
def error(self, node: ast.AST, message: str) -> None: | ||
|
@@ -1832,11 +1834,11 @@ def parse_options( | |
Y008 = 'Y008 Unrecognized platform "{platform}"' | ||
Y009 = 'Y009 Empty body should contain "...", not "pass"' | ||
Y010 = 'Y010 Function body must contain only "..."' | ||
Y011 = 'Y011 Default values for typed arguments must be "..."' | ||
Y011 = "Y011 Only simple default values allowed for typed arguments" | ||
Y012 = 'Y012 Class body must not contain "pass"' | ||
Y013 = 'Y013 Non-empty class body must not contain "..."' | ||
Y014 = 'Y014 Default values for arguments must be "..."' | ||
Y015 = 'Y015 Bad default value. Use "{new_syntax}" instead of "{old_syntax}"' | ||
Y014 = "Y014 Only simple default values allowed for arguments" | ||
Y015 = "Y015 Only simple default values are allowed for assignments" | ||
Y016 = 'Y016 Duplicate union member "{}"' | ||
Y017 = "Y017 Only simple assignments allowed" | ||
Y018 = 'Y018 {typevarlike_cls} "{typevar_name}" is not used' | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,28 @@ | ||
import os | ||
|
||
def f1(x: int = ...) -> None: ... | ||
def f2(x: int = 3) -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f2(x: int = 3) -> None: ... | ||
def f201(x: int = -3) -> None: ... | ||
def f202(x: float = 3.14) -> None: ... | ||
def f203(x: type = int) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f3(*, x: int = ...) -> None: ... | ||
def f4(*, x: int = 3) -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f5(x=3) -> None: ... # Y014 Default values for arguments must be "..." | ||
def f4(*, x: complex = 3j) -> None: ... | ||
def f401(*, x: complex = 5 + 3j) -> None: ... | ||
def f402(*, x: complex = -42 - 42j) -> None: ... | ||
def f405(*, x: int = 3 * 3) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f406(*, x: int = -3 >> 3) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f5(x=3) -> None: ... | ||
def f6(*, x: int) -> None: ... | ||
def f7(x, y: int = 3) -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f7(x, y: int = 3) -> None: ... | ||
def f8(x, y: int = ...) -> None: ... | ||
def f9(x, y: str = "x") -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f9(x, y: str = "x") -> None: ... | ||
def f901(x, y: str = os.pathsep) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f10(x, y: str = ..., *args: "int") -> None: ... # Y020 Quoted annotations should never be used in stubs | ||
def f11(*, x: str = "x") -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f11(*, x: str = "x") -> None: ... | ||
def f12(*, x: str = ..., **kwargs: "int") -> None: ... # Y020 Quoted annotations should never be used in stubs | ||
def f13(x: list[str] = ["foo", "bar", "baz"]) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f14(x: tuple[str, ...] = ("foo", "bar", "baz")) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f15(x: set[str] = {"foo", "bar", "baz"}) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f16(x: frozenset[bytes] = frozenset({b"foo", b"bar", b"baz"})) -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f17(x: str = "foo" + "bar") -> None: ... # Y011 Only simple default values allowed for typed arguments | ||
def f18(x: str = b"foo" + b"bar") -> None: ... # Y011 Only simple default values allowed for typed arguments |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
import os | ||
|
||
def f(x: "int", /) -> None: ... # Y020 Quoted annotations should never be used in stubs | ||
def f1(x: int, /) -> None: ... | ||
def f2(x: int, /, y: "int") -> None: ... # Y020 Quoted annotations should never be used in stubs | ||
def f3(x: str = "y", /) -> None: ... # Y011 Default values for typed arguments must be "..." | ||
def f3(x: str = "y", /) -> None: ... | ||
def f4(x: str = os.pathsep, /) -> None: ... # Y011 Only simple default values allowed for typed arguments |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For extra paranoia could check that the operands to
|
are themselves valid, but fine to omit that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good idea, I'll do that