From 943df40600a3711621fef2ed3fe783165686f6f6 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Mon, 24 Apr 2023 20:13:25 +0100 Subject: [PATCH] Make MatchError a dataclass --- src/ansiblelint/_internal/rules.py | 1 + src/ansiblelint/errors.py | 107 ++++++++++++-------------- src/ansiblelint/rules/__init__.py | 15 ++-- src/ansiblelint/rules/schema.py | 4 +- src/ansiblelint/rules/syntax_check.py | 7 +- src/ansiblelint/runner.py | 4 +- src/ansiblelint/utils.py | 2 +- test/test_formatter.py | 6 +- test/test_formatter_json.py | 6 +- test/test_formatter_sarif.py | 4 +- test/test_matcherrror.py | 13 ++-- 11 files changed, 77 insertions(+), 92 deletions(-) diff --git a/src/ansiblelint/_internal/rules.py b/src/ansiblelint/_internal/rules.py index dae44452e3..4b490ca8e7 100644 --- a/src/ansiblelint/_internal/rules.py +++ b/src/ansiblelint/_internal/rules.py @@ -156,6 +156,7 @@ class RuntimeErrorRule(BaseRule): """Unexpected internal error.""" id = "internal-error" + shortdesc = "Unexpected internal error" severity = "VERY_HIGH" tags = ["core"] version_added = "v5.0.0" diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index 88b4bc9dab..37dd15f7c3 100644 --- a/src/ansiblelint/errors.py +++ b/src/ansiblelint/errors.py @@ -2,11 +2,12 @@ from __future__ import annotations import functools +from dataclasses import dataclass, field from typing import Any from ansiblelint._internal.rules import BaseRule, RuntimeErrorRule from ansiblelint.config import options -from ansiblelint.file_utils import Lintable, normpath +from ansiblelint.file_utils import Lintable class StrictModeError(RuntimeError): @@ -21,6 +22,7 @@ def __init__( # pylint: disable=too-many-instance-attributes +@dataclass(unsafe_hash=True) @functools.total_ordering class MatchError(ValueError): """Rule violation detected during linting. @@ -32,70 +34,61 @@ class MatchError(ValueError): instance. """ - tag = "" - - # IMPORTANT: any additional comparison protocol methods must return - # IMPORTANT: `NotImplemented` singleton to allow the check to use the - # IMPORTANT: other object's fallbacks. - # Ref: https://docs.python.org/3/reference/datamodel.html#object.__lt__ + # order matters for these: + message: str = field(init=True, repr=False, default="") + lintable: Lintable = field(init=True, repr=False, default=Lintable(name="")) + filename: str = field(init=True, repr=False, default="") + + tag: str = field(init=True, repr=False, default="") + lineno: int = 1 + details: str = "" + column: int | None = None + # rule is not included in hash because we might have different instances + # of the same rule, but we use the 'tag' to identify the rule. + rule: BaseRule = field(hash=False, default=RuntimeErrorRule()) + ignored: bool = False + fixed: bool = False # True when a transform has resolved this MatchError + + def __post_init__(self) -> None: + # This can be used by rules that can report multiple errors type, so + # we can still filter by them. - # pylint: disable=too-many-arguments - def __init__( - self, - message: str | None = None, - # most linters report use (1,1) base, including yamllint and flake8 - # we should never report line 0 or column 0 in output. - lineno: int = 1, - column: int | None = None, - details: str = "", - filename: Lintable | None = None, - rule: BaseRule = RuntimeErrorRule(), - tag: str | None = None, # optional fine-graded tag - ignored: bool = False, - ) -> None: - """Initialize a MatchError instance.""" - super().__init__(message) + if not self.lintable and self.filename: + self.lintable = Lintable(self.filename) + elif self.lintable and not self.filename: + self.filename = self.lintable.name + + # We want to catch accidental MatchError() which contains no useful + # information. When no arguments are passed, the '_message' field is + # set to 'property', only if passed it becomes a string. + if self.rule.__class__ is RuntimeErrorRule: + # so instance was created without a rule + if not self.message: + msg = f"{self.__class__.__name__}() missing a required argument: one of 'message' or 'rule'" + raise TypeError(msg) + if not isinstance(self.tag, str): + msg = "MatchErrors must be created with either rule or tag specified." + raise TypeError(msg) + if not self.message: + self.message = self.rule.shortdesc - if rule.__class__ is RuntimeErrorRule and not message: - msg = f"{self.__class__.__name__}() missing a required argument: one of 'message' or 'rule'" - raise TypeError(msg) + self.match_type: str | None = None + # for task matches, save the normalized task object (useful for transforms) + self.task: dict[str, Any] | None = None + # path to the problem area, like: [0,"pre_tasks",3] for [0].pre_tasks[3] + self.yaml_path: list[int | str] = [] - self.message = str(message or getattr(rule, "shortdesc", "")) + if not self.tag: + self.tag = self.rule.id # Safety measure to ensure we do not end-up with incorrect indexes - if lineno == 0: # pragma: no cover + if self.lineno == 0: # pragma: no cover msg = "MatchError called incorrectly as line numbers start with 1" raise RuntimeError(msg) - if column == 0: # pragma: no cover + if self.column == 0: # pragma: no cover msg = "MatchError called incorrectly as column numbers start with 1" raise RuntimeError(msg) - self.lineno = lineno - self.column = column - self.details = details - self.filename = "" - if filename: - if isinstance(filename, Lintable): - self.lintable = filename - self.filename = normpath(str(filename.path)) - else: - self.filename = normpath(filename) - self.lintable = Lintable(self.filename) - self.rule = rule - self.ignored = ignored # If set it will be displayed but not counted as failure - # This can be used by rules that can report multiple errors type, so - # we can still filter by them. - self.tag = tag or rule.id - - # optional indicator on how this error was found - self.match_type: str | None = None - # for task matches, save the normalized task object (useful for transforms) - self.task: dict[str, Any] | None = None - # path to the problem area, like: [0,"pre_tasks",3] for [0].pre_tasks[3] - self.yaml_path: list[int | str] = [] - # True when a transform has resolved this MatchError - self.fixed = False - @functools.cached_property def level(self) -> str: """Return the level of the rule: error, warning or notice.""" @@ -147,10 +140,6 @@ def __lt__(self, other: object) -> bool: return NotImplemented return bool(self._hash_key < other._hash_key) - def __hash__(self) -> int: - """Return a hash value of the MatchError instance.""" - return hash(self._hash_key) - def __eq__(self, other: object) -> bool: """Identify whether the other object represents the same rule match.""" if not isinstance(other, self.__class__): diff --git a/src/ansiblelint/rules/__init__.py b/src/ansiblelint/rules/__init__.py index 0b578d0bf1..c4ce41e78c 100644 --- a/src/ansiblelint/rules/__init__.py +++ b/src/ansiblelint/rules/__init__.py @@ -73,7 +73,7 @@ def unjinja(text: str) -> str: # pylint: disable=too-many-arguments def create_matcherror( self, - message: str | None = None, + message: str = "", lineno: int = 1, details: str = "", filename: Lintable | None = None, @@ -84,11 +84,10 @@ def create_matcherror( message=message, lineno=lineno, details=details, - filename=filename, + lintable=filename or Lintable(""), rule=copy.copy(self), + tag=tag, ) - if tag: - match.tag = tag # search through callers to find one of the match* methods frame = inspect.currentframe() match_type: str | None = None @@ -128,7 +127,7 @@ def matchlines(self, file: Lintable) -> list[MatchError]: result = self.match(line) if not result: continue - message = None + message = "" if isinstance(result, str): message = result matcherror = self.create_matcherror( @@ -198,7 +197,7 @@ def matchtasks(self, file: Lintable) -> list[MatchError]: # noqa: C901 continue match = result else: # bool or string - message = None + message = "" if isinstance(result, str): message = result match = self.create_matcherror( @@ -223,7 +222,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: if isinstance(yaml, str): if yaml.startswith("$ANSIBLE_VAULT"): return [] - return [MatchError(filename=file, rule=LoadingFailureRule())] + return [MatchError(lintable=file, rule=LoadingFailureRule())] if not yaml: return matches @@ -458,7 +457,7 @@ def run( # : max-complexity: 12 return [ MatchError( message=str(exc), - filename=file, + lintable=file, rule=LoadingFailureRule(), tag=f"{LoadingFailureRule.id}[{exc.__class__.__name__.lower()}]", ), diff --git a/src/ansiblelint/rules/schema.py b/src/ansiblelint/rules/schema.py index ab88c5482f..92be23cf13 100644 --- a/src/ansiblelint/rules/schema.py +++ b/src/ansiblelint/rules/schema.py @@ -67,7 +67,7 @@ def matchtask( result.append( MatchError( message=msg, - filename=file, + lintable=file or Lintable(""), rule=ValidateSchemaRule(), details=ValidateSchemaRule.description, tag=f"schema[{tag}]", @@ -93,7 +93,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: result.append( MatchError( message=errors[0], - filename=file, + lintable=file, rule=ValidateSchemaRule(), details=ValidateSchemaRule.description, tag=f"schema[{file.kind}]", diff --git a/src/ansiblelint/rules/syntax_check.py b/src/ansiblelint/rules/syntax_check.py index f7520b0e4b..7a69d21f7c 100644 --- a/src/ansiblelint/rules/syntax_check.py +++ b/src/ansiblelint/rules/syntax_check.py @@ -136,7 +136,6 @@ def _get_ansible_syntax_check_matches( # noqa: C901 filename = lintable lineno = 1 column = None - tag = None stderr = strip_ansi_escape(run.stderr) stdout = strip_ansi_escape(run.stdout) @@ -164,7 +163,7 @@ def _get_ansible_syntax_check_matches( # noqa: C901 results.append( MatchError( message=title, - filename=filename, + lintable=filename, lineno=lineno, column=column, rule=rule, @@ -182,12 +181,12 @@ def _get_ansible_syntax_check_matches( # noqa: C901 results.append( MatchError( message=message, - filename=filename, + lintable=filename, lineno=lineno, column=column, rule=rule, details=details, - tag=tag, + tag="", ), ) diff --git a/src/ansiblelint/runner.py b/src/ansiblelint/runner.py index a1822c4ccd..8f73ec311a 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -124,7 +124,7 @@ def run(self) -> list[MatchError]: # noqa: C901 if isinstance(lintable.data, States) and lintable.exc: matches.append( MatchError( - filename=lintable, + lintable=lintable, message=str(lintable.exc), details=str(lintable.exc.__cause__), rule=LoadingFailureRule(), @@ -208,7 +208,7 @@ def _emit_matches(self, files: list[Lintable]) -> Generator[MatchError, None, No exc.rule = LoadingFailureRule() yield exc except AttributeError: - yield MatchError(filename=lintable, rule=LoadingFailureRule()) + yield MatchError(lintable=lintable, rule=LoadingFailureRule()) visited.add(lintable) diff --git a/src/ansiblelint/utils.py b/src/ansiblelint/utils.py index c4be30417b..056e36af09 100644 --- a/src/ansiblelint/utils.py +++ b/src/ansiblelint/utils.py @@ -270,7 +270,7 @@ def find_children(lintable: Lintable) -> list[Lintable]: # noqa: C901 basedir = os.path.dirname(str(lintable.path)) # playbook_ds can be an AnsibleUnicode string, which we consider invalid if isinstance(playbook_ds, str): - raise MatchError(filename=lintable, rule=LoadingFailureRule()) + raise MatchError(lintable=lintable, rule=LoadingFailureRule()) for item in _playbook_items(playbook_ds): # if lintable.kind not in ["playbook"]: for child in play_children(basedir, item, lintable.kind, playbook_dir): diff --git a/test/test_formatter.py b/test/test_formatter.py index f8b2144fcf..2c8ab96a30 100644 --- a/test/test_formatter.py +++ b/test/test_formatter.py @@ -38,7 +38,7 @@ def test_format_coloured_string() -> None: message="message", lineno=1, details=DETAILS, - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=rule, ) formatter.format(match) @@ -50,7 +50,7 @@ def test_unicode_format_string() -> None: message="\U0001f427", lineno=1, details=DETAILS, - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=rule, ) formatter.format(match) @@ -62,7 +62,7 @@ def test_dict_format_line() -> None: message="xyz", lineno=1, details={"hello": "world"}, # type: ignore - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=rule, ) formatter.format(match) diff --git a/test/test_formatter_json.py b/test/test_formatter_json.py index fd0d3fc00b..85c7758712 100644 --- a/test/test_formatter_json.py +++ b/test/test_formatter_json.py @@ -32,7 +32,7 @@ def setup_class(self) -> None: message="message", lineno=1, details="hello", - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=self.rule, ), ) @@ -41,7 +41,7 @@ def setup_class(self) -> None: message="message", lineno=2, details="hello", - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=self.rule, ignored=True, ), @@ -110,7 +110,7 @@ def test_validate_codeclimate_schema_with_positions(self) -> None: lineno=1, column=42, details="hello", - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=self.rule, ), ], diff --git a/test/test_formatter_sarif.py b/test/test_formatter_sarif.py index fc8fceb83a..01c4fead56 100644 --- a/test/test_formatter_sarif.py +++ b/test/test_formatter_sarif.py @@ -38,7 +38,7 @@ def setup_class(self) -> None: lineno=1, column=10, details="details", - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=self.rule, tag="yaml[test]", ), @@ -48,7 +48,7 @@ def setup_class(self) -> None: message="message", lineno=2, details="", - filename=Lintable("filename.yml", content=""), + lintable=Lintable("filename.yml", content=""), rule=self.rule, tag="yaml[test]", ), diff --git a/test/test_matcherrror.py b/test/test_matcherrror.py index 07a2717c53..6d103e8795 100644 --- a/test/test_matcherrror.py +++ b/test/test_matcherrror.py @@ -64,11 +64,8 @@ def test_matcherror_compare( def test_matcherror_invalid() -> None: """Ensure that MatchError requires message or rule.""" - expected_err = ( - r"^MatchError\(\) missing a required argument: one of 'message' or 'rule'$" - ) - with pytest.raises(TypeError, match=expected_err): - raise MatchError + with pytest.raises(TypeError): + MatchError() # pylint: disable=pointless-exception-statement @pytest.mark.parametrize( @@ -78,8 +75,8 @@ def test_matcherror_invalid() -> None: (MatchError("z"), MatchError("a")), # filenames takes priority in sorting ( - MatchError("a", filename=Lintable("b", content="")), - MatchError("a", filename=Lintable("a", content="")), + MatchError("a", lintable=Lintable("b", content="")), + MatchError("a", lintable=Lintable("a", content="")), ), # rule id partial-become > rule id no-changed-when ( @@ -182,7 +179,7 @@ def test_matcherror_compare_with_other_fallback( expected_value: bool, ) -> None: """Check that MatchError comparison runs other types fallbacks.""" - assert operation(MatchError("foo"), other) is expected_value + assert operation(MatchError(message="foo"), other) is expected_value @pytest.mark.parametrize(