diff --git a/README.md b/README.md index 5814b22..4b22013 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,8 @@ Example: [tool.tryceratops] exclude = ["samples"] ignore = ["TRY002", "TRY200", "TRY300"] -experimental = true +experimental = false +check_pickable = false ``` CLI flags always overwrite the config file. diff --git a/docs/violations/README.md b/docs/violations/README.md index 750a00c..5fb3c77 100644 --- a/docs/violations/README.md +++ b/docs/violations/README.md @@ -2,11 +2,12 @@ ## `TRY0xx` - Exception Classes -| Code | Description | -| ------------------- | ---------------------------------------------------------- | -| [TRY002](TRY002.md) | Create your own exception | -| [TRY003](TRY003.md) | Avoid specifying long messages outside the exception class | -| [TRY004](TRY004.md) | Prefer `TypeError` exception for invalid type | +| Code | Description | +| ---------------------------------------------------------- | ---------------------------------------------------------- | +| [TRY002](TRY002.md) | Create your own exception | +| [TRY003](TRY003.md) | Avoid specifying long messages outside the exception class | +| [TRY004](TRY004.md) | Prefer `TypeError` exception for invalid type | +| [TRY005](TRY005.md) (**ENABLED through `check_pickable`**) | Define `__reduce__` to make exception pickable | ## `TRY1xx` - General diff --git a/docs/violations/TRY005.md b/docs/violations/TRY005.md new file mode 100644 index 0000000..be179ad --- /dev/null +++ b/docs/violations/TRY005.md @@ -0,0 +1,56 @@ +# `TRY005` - Define `__reduce__` to make exception pickable + +## Why is it bad + +When using multiprocessing (or anything that serializes with Pickle), and you raise an exception [Pickle uses `__reduce__`](https://docs.python.org/2/library/pickle.html#object.__reduce__) to serialize it. + +It breaks if your exception takes custom args (not string or not optional). + +[Stack Overflow question](https://stackoverflow.com/questions/16244923/how-to-make-a-custom-exception-class-with-multiple-init-args-pickleable) + +## How to enable it + +Since not every project would care about it, this is an optional violation that can be enabled through `check_pickable`. + +## How it looks like + +```py +class ManyArgsMissingReduce(Exception): + def __init__(self, val1: str, val2: str) -> None: # Requires pickable + super().__init__(f"{val1} {val2}") + + +class CustomMissingReduce(Exception): + def __init__(self, age: int) -> None: # Requires pickable + super().__init__(f"You're not old enough: {age}") +``` + +## How it should be + +```py +# Generic implementation: +class GenericReduceException(Exception) + def __init__(self, *args, **kwargs) -> None: + self.args = tuple([*args, *kwargs.values()]) # Saves all args/kwargs + super().__init__(*args) + + def __reduce__(self) -> str | tuple[Any, ...]: + return (self.__class__, self.args) # Return them here + +# You can also be a bit more verbose: +class ManyArgsWITHReduce(Exception): + def __init__(self, val1: str, val2: str) -> None: + self.val1, self.val2 = val1, val2 + super().__init__(f"{val1} {val2}") + + def __reduce__(self) -> str | tuple[Any, ...]: + return (ManyArgsWITHReduce, (self.val1, self.val2)) +``` + + +## When this is fine + +This is ok if you don't care about pickable exceptions 🤷 or either if you have an exception: + +- Without a custom `__init__` defined; or +- With `__init__` that receives only one string argument diff --git a/pyproject.toml b/pyproject.toml index 4ee2732..f3f16c8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,7 @@ astpretty = "^3.0.0" python-semantic-release = "^7.33.5" mypy = "^1.3.0" types-toml = "^0.1.3" +typing-extensions = { version = "^4.5.0", python = "<3.10" } [tool.black] line-length = 100 @@ -74,7 +75,7 @@ line-length = 100 [tool.isort] profile = "black" line_length = 100 -extra_standard_library = ["pytest", "toml", "click"] +extra_standard_library = ["pytest", "toml", "click", "typing_extensions"] [tool.mypy] python_version = 3.8 diff --git a/src/tests/analyzers_class_test.py b/src/tests/analyzers_class_test.py new file mode 100644 index 0000000..02fe8be --- /dev/null +++ b/src/tests/analyzers_class_test.py @@ -0,0 +1,22 @@ +from functools import partial + +from tryceratops import analyzers +from tryceratops.violations import codes + +from .analyzer_helpers import assert_violation, read_sample + + +def test_non_pickable_error(): + tree = read_sample("class_non_pickable") + analyzer = analyzers.classdefs.NonPickableAnalyzer() + + assert_non_pickable = partial( + assert_violation, codes.NON_PICKABLE_CLASS[0], codes.NON_PICKABLE_CLASS[1] + ) + + violations = analyzer.check(tree, "filename") + + assert len(violations) == 2 + + assert_non_pickable(24, 0, violations[0]) + assert_non_pickable(29, 0, violations[1]) diff --git a/src/tests/samples/violations/class_non_pickable.py b/src/tests/samples/violations/class_non_pickable.py new file mode 100644 index 0000000..1a3acab --- /dev/null +++ b/src/tests/samples/violations/class_non_pickable.py @@ -0,0 +1,49 @@ +""" +Violation: + +Implement __reduce__ to enforce class is pickable. +""" + + +from typing import Any + + +class RandomClass: + """This is not even an exception""" + + +class RegularException(Exception): + pass + + +class AnotherOkException(Exception): + def __init__(self, val1: str) -> None: + super().__init__(val1) + + +class ManyArgsMissingReduce(Exception): + def __init__(self, val1: str, val2: str) -> None: # Requires pickable + super().__init__(f"{val1} {val2}") + + +class CustomMissingReduce(Exception): + def __init__(self, age: int) -> None: # Requires pickable + super().__init__(f"You're not old enough: {age}") + + +class ManyArgsWITHReduce(Exception): + def __init__(self, val1: str, val2: str) -> None: + self.val1, self.val2 = val1, val2 + super().__init__(f"{val1} {val2}") + + def __reduce__(self) -> str | tuple[Any, ...]: + return (ManyArgsWITHReduce, (self.val1, self.val2)) + + +class CustomWITHReduce(Exception): + def __init__(self, age: int) -> None: + self.age = age + super().__init__(f"You're not old enough: {age}") + + def __reduce__(self) -> str | tuple[Any, ...]: + return (CustomMissingReduce, (self.age,)) diff --git a/src/tryceratops/analyzers/__init__.py b/src/tryceratops/analyzers/__init__.py index 604c38e..034d3ee 100644 --- a/src/tryceratops/analyzers/__init__.py +++ b/src/tryceratops/analyzers/__init__.py @@ -1,8 +1,8 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Set, Type, cast +from typing import TYPE_CHECKING, Set, Type -from . import call, conditional, exception_block, try_block +from . import call, classdefs, conditional, exception_block, try_block from .base import BaseAnalyzer if TYPE_CHECKING: @@ -15,6 +15,7 @@ call.CallRaiseLongArgsAnalyzer, call.CallAvoidCheckingToContinueAnalyzer, conditional.PreferTypeErrorAnalyzer, + classdefs.NonPickableAnalyzer, exception_block.ExceptReraiseWithoutCauseAnalyzer, exception_block.ExceptVerboseReraiseAnalyzer, exception_block.ExceptBroadPassAnalyzer, diff --git a/src/tryceratops/analyzers/classdefs.py b/src/tryceratops/analyzers/classdefs.py new file mode 100644 index 0000000..a163bae --- /dev/null +++ b/src/tryceratops/analyzers/classdefs.py @@ -0,0 +1,46 @@ +import ast +import typing as t + +from tryceratops.violations import codes + +from .base import BaseAnalyzer, visit_error_handler + + +class NonPickableAnalyzer(BaseAnalyzer): + violation_code = codes.NON_PICKABLE_CLASS + + def _find_method(self, node: ast.ClassDef, name: str) -> t.Optional[ast.FunctionDef]: + for method in node.body: + if isinstance(method, ast.FunctionDef) and method.name == name: + return method + + return None + + @visit_error_handler + def visit_ClassDef(self, node: ast.ClassDef) -> t.Any: + is_exc = any([base for base in node.bases if getattr(base, "id") == "Exception"]) + if is_exc is False: + return self.generic_visit(node) + + init_method = self._find_method(node, "__init__") + if init_method is None: + return self.generic_visit(node) + + reduce_method = self._find_method(node, "__reduce__") + if reduce_method is not None: + # Good enough to say this is not a violation + return self.generic_visit(node) + + # First arg would be self + has_more_than_one_arg = len(init_method.args.args) > 1 + if has_more_than_one_arg is False: + return self.generic_visit(node) + + _, second_arg, *remaining_args = init_method.args.args + if ( + len(remaining_args) > 0 + or second_arg.annotation + and getattr(second_arg.annotation, "id") != "str" + ): + # Pickle would break for non string args or for more than 1 arg + self._mark_violation(node) diff --git a/src/tryceratops/filters.py b/src/tryceratops/filters.py index 539306d..e06880e 100644 --- a/src/tryceratops/filters.py +++ b/src/tryceratops/filters.py @@ -5,6 +5,7 @@ from tryceratops.processors import Processor from tryceratops.violations import Violation +from tryceratops.violations.codes import NON_PICKABLE_CLASS if TYPE_CHECKING: from tryceratops.types import PyprojectConfig @@ -54,6 +55,7 @@ class GlobalSettings: include_experimental: bool ignore_violations: Iterable[str] exclude_dirs: Iterable[str] + check_pickable: bool = False autofix: bool = False def _self_check(self) -> None: @@ -61,6 +63,9 @@ def _self_check(self) -> None: def __post_init__(self) -> None: self._self_check() + if self.check_pickable is False: + self.ignore_violations = list(self.ignore_violations) + self.ignore_violations.append(NON_PICKABLE_CLASS[0]) @property def exclude_experimental(self) -> bool: @@ -88,8 +93,9 @@ def create_from_config(cls, config: PyprojectConfig) -> GlobalSettings: ignore = config.get("ignore", []) exclude = config.get("exclude", []) autofix = config.get("autofix", False) + check_pickable = config.get("check_pickable", False) - return cls(experimental, ignore, exclude, autofix) + return cls(experimental, ignore, exclude, autofix, check_pickable) def overwrite_from_cli( self, diff --git a/src/tryceratops/types.py b/src/tryceratops/types.py index 08a7323..25412fc 100644 --- a/src/tryceratops/types.py +++ b/src/tryceratops/types.py @@ -1,13 +1,14 @@ import ast -from typing import Collection, List, Tuple, TypedDict +import typing as t +import typing_extensions as te from tryceratops.filters import FileFilter -ParsedFileType = Tuple[str, ast.AST, FileFilter] -ParsedFilesType = Collection[ParsedFileType] +ParsedFileType = t.Tuple[str, ast.AST, FileFilter] +ParsedFilesType = t.Collection[ParsedFileType] -class PyprojectConfig(TypedDict): +class PyprojectConfig(t.TypedDict): """ Represents the expected pyproject config to be loaded exclude: a list of path patterns to be excluded e.g. [/tests, /fixtures] @@ -15,7 +16,8 @@ class PyprojectConfig(TypedDict): experimental: whether to enable experimental analyzers """ - exclude: List[str] - ignore: List[str] - experimental: bool - autofix: bool + exclude: te.NotRequired[t.List[str]] + ignore: te.NotRequired[t.List[str]] + experimental: te.NotRequired[bool] + autofix: te.NotRequired[bool] + check_pickable: te.NotRequired[bool] diff --git a/src/tryceratops/violations/codes.py b/src/tryceratops/violations/codes.py index 7cfe849..2e31c6a 100644 --- a/src/tryceratops/violations/codes.py +++ b/src/tryceratops/violations/codes.py @@ -5,6 +5,7 @@ "Avoid specifying long messages outside the exception class", ) PREFER_TYPE_ERROR = ("TRY004", "Prefer TypeError exception for invalid type") +NON_PICKABLE_CLASS = ("TRY005", "Define __reduce__ to make exception pickable") # TRY1xx - General CHECK_TO_CONTINUE = ( @@ -31,16 +32,23 @@ VERBOSE_LOG_MESSAGE = ("TRY401", "Do not log exception object, give context instead") CODE_CHOICES = { + # TRY0xx - Exceptions "TRY002", "TRY003", + "TRY004", + "TRY005", + # TRY1xx - General "TRY100", "TRY101", + # TRY2xx - Except "TRY200", "TRY201", "TRY202", "TRY203", + # TRY3xx - Try "TRY300", "TRY301", + # TRY4xx - Logging "TRY400", "TRY401", }