Fix 'class_definitions_order' linter check

Scony · Scony · commit 78496d74a0e4 · 2023-01-12T20:43:36.000+01:00
diff --git a/gdtoolkit/common/ast.py b/gdtoolkit/common/ast.py
@@ -1,25 +1,38 @@
+# flake8: noqa
 from typing import List
 
 from lark import Tree
 
+from ..formatter.annotation import STANDALONE_ANNOTATIONS
+
 from .utils import find_name_token_among_children, find_tree_among_children
 
 
-# pylint: disable=too-few-public-methods
+# pylint: disable-next=too-few-public-methods
+class Annotation:
+    def __init__(self, node: Tree):
+        self.lark_node = node
+        self.name = node.children[0].value
+
+
+# pylint: disable-next=too-few-public-methods
 class Statement:
     """Abstract representation of statement"""
 
-    def __init__(self, node: Tree):
+    # TODO: remove default = [] and fix Statements w/o annotations passed
+    # pylint: disable-next=dangerous-default-value
+    def __init__(self, node: Tree, annotations: List[Annotation] = []):
         self.lark_node = node
         self.kind = node.data
+        self.annotations = annotations
         self.sub_statements = []  # type: List[Statement]
         self.all_sub_statements = []  # type: List[Statement]
 
         self._load_sub_statements()
 
     def _load_sub_statements(self):
         if self.kind == "class_def":
-            raise NotImplementedError
+            pass  # TODO: implement
         if self.kind == "property_body_def":
             raise NotImplementedError
         if self.kind in ["func_def", "static_func_def"]:
@@ -48,15 +61,15 @@ def __repr__(self):
         )
 
 
-# pylint: disable=too-few-public-methods
+# pylint: disable-next=too-few-public-methods
 class Parameter:
     """Abstract representation of function parameter"""
 
     def __init__(self, node: Tree):
         self.name = node.children[0].value
 
 
-# pylint: disable=too-few-public-methods
+# pylint: disable-next=too-few-public-methods
 class Function(Statement):
     """Abstract representation of function"""
 
@@ -80,7 +93,7 @@ def _load_data_from_func_def(self, func_def: Tree) -> None:
         ]
 
 
-# pylint: disable=too-few-public-methods
+# pylint: disable-next=too-few-public-methods
 class Class:
     """Abstract representation of class.
     Since it contains sub-classes, it forms a tree"""
@@ -92,6 +105,7 @@ def __init__(self, parse_tree: Tree):
         self.all_sub_classes = []  # type: List[Class]
         self.functions = []  # type: List[Function]
         self.all_functions = []  # type: List[Function]
+        self.statements = []  # type: List[Statement]
 
         if parse_tree.data == "start":
             start = parse_tree
@@ -102,9 +116,19 @@ def __init__(self, parse_tree: Tree):
             raise Exception("Cannot load class from that node")
 
     def _load_data_from_node_children(self, node: Tree) -> None:
-        for stmt in node.children:
-            if not isinstance(stmt, Tree):
+        offset = 1 if node.data == "class_def" else 0
+        annotations = []
+        for stmt in node.children[offset:]:
+            if stmt.data == "annotation" and not _is_annotation_standalone(
+                Annotation(stmt)
+            ):
+                annotations.append(Annotation(stmt))
+                continue
+            if stmt.data == "property_body_def":
                 continue
+            assert isinstance(stmt, Tree), stmt
+            self.statements.append(Statement(stmt, annotations))
+            annotations = []
             if stmt.data == "class_def":
                 sub_class = Class(stmt)
                 self.sub_classes.append(sub_class)
@@ -121,7 +145,7 @@ def _load_data_from_class_def(self, class_def: Tree) -> None:
         self._load_data_from_node_children(class_def)
 
 
-# pylint: disable=too-few-public-methods
+# pylint: disable-next=too-few-public-methods
 class AbstractSyntaxTree:
     """Post-processed version of parse tree - more convenient representation
     for further processing"""
@@ -130,3 +154,7 @@ def __init__(self, parse_tree: Tree):
         self.root_class = Class(parse_tree)
         self.all_classes = [self.root_class] + self.root_class.all_sub_classes
         self.all_functions = self.root_class.all_functions
+
+
+def _is_annotation_standalone(annotation: Annotation) -> bool:
+    return annotation.name in STANDALONE_ANNOTATIONS
diff --git a/gdtoolkit/linter/class_checks.py b/gdtoolkit/linter/class_checks.py
@@ -4,6 +4,7 @@
 
 from lark import Token, Tree
 
+from ..common.ast import AbstractSyntaxTree, Class, Statement, Annotation
 from ..common.utils import find_name_token_among_children
 
 from .problem import Problem
@@ -17,15 +18,22 @@ def lint(parse_tree: Tree, config: MappingProxyType) -> List[Problem]:
             "private-method-call",
             _private_method_call_check,
         ),
+    ]  # type: List[Tuple[str, Callable]]
+    problem_clusters = (
+        x[1](parse_tree) if x[0] not in disable else [] for x in checks_to_run_w_tree
+    )
+    problems = [problem for cluster in problem_clusters for problem in cluster]
+    checks_to_run_w_ast = [
         (
             "class-definitions-order",
             partial(_class_definitions_order_check, config["class-definitions-order"]),
         ),
-    ]  # type: List[Tuple[str, Callable]]
+    ]
+    ast = AbstractSyntaxTree(parse_tree)
     problem_clusters = (
-        x[1](parse_tree) if x[0] not in disable else [] for x in checks_to_run_w_tree
+        x[1](ast) if x[0] not in disable else [] for x in checks_to_run_w_ast
     )
-    problems = [problem for cluster in problem_clusters for problem in cluster]
+    problems += [problem for cluster in problem_clusters for problem in cluster]
     return problems
 
 
@@ -55,71 +63,89 @@ def _private_method_call_check(parse_tree: Tree) -> List[Problem]:
     return problems
 
 
+# TODO: drop
 def _is_method_private(method_name: str) -> bool:
     return method_name.startswith("_")  # TODO: consider making configurable
 
 
-def _class_definitions_order_check(order, parse_tree: Tree) -> List[Problem]:
-    problems = _class_definitions_order_check_for_class(
-        "global scope", parse_tree.children, order
-    )
-    for class_def in parse_tree.find_data("class_def"):
-        class_name = class_def.children[0].value
-        problems += _class_definitions_order_check_for_class(
-            "class {}".format(class_name), class_def.children, order
-        )
-    return problems
+def _class_definitions_order_check(
+    order: List[str], ast: AbstractSyntaxTree
+) -> List[Problem]:
+    return [
+        problem
+        for a_class in ast.all_classes
+        for problem in _class_definitions_order_check_for_class(a_class, order)
+    ]
 
 
 def _class_definitions_order_check_for_class(
-    class_name: str, class_children, order
+    a_class: Class, order: List[str]
 ) -> List[Problem]:
-    stmt_to_section_mapping = {
-        "tool_stmt": "tools",
-        "signal_stmt": "signals",
-        "extends_stmt": "extends",
-        "classname_stmt": "classnames",
-        "const_stmt": "consts",
-        "export_stmt": "exports",
-        "enum_def": "enums",
-    }
-    visibility_dependent_stmt_to_section_mapping = {
-        "class_var_stmt": {"pub": "pubvars", "prv": "prvvars"},
-        "onready_stmt": {"pub": "onreadypubvars", "prv": "onreadyprvvars"},
-    }
     problems = []
     current_section = order[0]
-    for class_child in class_children:
-        if not isinstance(class_child, Tree):
-            continue
-        if class_child.data in ["annotation"]:
+    for statement in a_class.statements:
+        if _is_statement_irrelevant(statement):
             continue
-        stmt = class_child.data
-        if stmt == "class_var_stmt":
-            visibility = _class_var_stmt_visibility(class_child)
-            section = visibility_dependent_stmt_to_section_mapping[stmt][visibility]
-        elif stmt == "onready_stmt":
-            class_var_stmt = class_child.children[0]
-            visibility = _class_var_stmt_visibility(class_var_stmt)
-            section = visibility_dependent_stmt_to_section_mapping[stmt][visibility]
-        else:
-            section = stmt_to_section_mapping.get(stmt, "others")
-        section_rank = order.index(section)
-        if section_rank >= order.index(current_section):
-            current_section = section
+        current_section_rank = order.index(current_section)
+        statement_section = _map_statement_to_section(statement)
+        section_rank = order.index(statement_section)
+        if section_rank >= current_section_rank:
+            current_section = statement_section
         else:
             problems.append(
                 Problem(
                     name="class-definitions-order",
-                    description="Definition out of order in {}".format(class_name),
-                    line=class_child.line,
-                    column=class_child.column,
+                    description="Definition out of order in {}".format(a_class.name),
+                    line=statement.lark_node.line,
+                    column=statement.lark_node.column,
                 )
             )
     return problems
 
 
-def _class_var_stmt_visibility(class_var_stmt) -> str:
+def _is_statement_irrelevant(statement: Statement) -> bool:
+    if statement.kind == "pass_stmt":
+        return True
+    if statement.kind == "annotation":
+        return Annotation(statement.lark_node).name != "tool"
+    return False
+
+
+# pylint: disable-next=too-many-return-statements
+def _map_statement_to_section(statement: Statement) -> str:
+    if statement.kind == "class_var_stmt":
+        if any(
+            annotation.name.startswith("export") for annotation in statement.annotations
+        ):
+            return "exports"
+        if any(annotation.name == "onready" for annotation in statement.annotations):
+            return "onready{}vars".format(
+                _class_var_stmt_visibility(statement.lark_node)
+            )
+        return "{}vars".format(_class_var_stmt_visibility(statement.lark_node))
+    if statement.kind == "signal_stmt":
+        return "signals"
+    if statement.kind == "extends_stmt":
+        return "extends"
+    if statement.kind == "enum_stmt":
+        return "enums"
+    if statement.kind == "classname_stmt":
+        return "classnames"
+    if statement.kind == "const_stmt":
+        return "consts"
+    if (
+        statement.kind == "annotation"
+        and Annotation(statement.lark_node).name == "tool"
+    ):
+        return "tools"
+    if statement.kind == "class_def":
+        return "others"
+    if statement.kind == "func_def":
+        return "others"
+    raise NotImplementedError
+
+
+def _class_var_stmt_visibility(class_var_stmt: Tree) -> str:
     some_var_stmt = class_var_stmt.children[0]
     name_token = find_name_token_among_children(some_var_stmt)
     return "pub" if is_function_public(name_token.value) else "prv"  # type: ignore
diff --git a/tests/linter/test_class_checks.py b/tests/linter/test_class_checks.py
@@ -31,7 +31,6 @@ def test_private_method_call_nok(code):
     simple_nok_check(code, 'private-method-call')
 
 
-@pytest.mark.skip(reason='to be fixed in a bundle')
 @pytest.mark.parametrize('code', [
 """
 pass
@@ -40,11 +39,12 @@ def test_private_method_call_nok(code):
 signal s
 enum { A, B, C }
 const X = 1
-export var k = 1
+@export_group("Foo")
+@export var k = 1
 var x = 1
 var _x = 1
-onready var y = null
-onready var _y = null
+@onready var y = null
+@onready var _y = null
 class Z:
     pass
     extends Node
@@ -53,7 +53,7 @@ class Z:
 """,
 ])
 def test_class_definitions_order_ok(code):
-    simple_ok_check(code)
+    simple_ok_check(code, disable="unnecessary-pass")
 
 
 @pytest.mark.parametrize('code', [
@@ -66,6 +66,27 @@ def test_class_definitions_order_ok(code):
 """
 class X: var x;extends Node
 """,
+"""var _x
+var x
+""",
+"""@onready var x
+var y
+""",
+"""@onready var _x
+@onready var x
+""",
+"""var x
+enum X { A, B }
+""",
+"""var x
+class_name Asdf
+""",
+"""var x
+const X = 1
+""",
+"""var x
+@tool
+""",
 ])
 def test_class_definitions_order_nok(code):
     simple_nok_check(code, 'class-definitions-order')
