Code Style Checker

Fri, 02 Apr 2021 15:35:44 +0200

author
Detlev Offenbach <detlev@die-offenbachs.de>
date
Fri, 02 Apr 2021 15:35:44 +0200
changeset 8191
9125da0c227e
parent 8189
17df5c8df8c1
child 8192
e1157bd8b4c2

Code Style Checker
- continued to implement checkers for potential code simplifications

eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyChecker.py file | annotate | diff | comparison | revisions
eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyNodeVisitor.py file | annotate | diff | comparison | revisions
eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/translations.py file | annotate | diff | comparison | revisions
--- a/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyChecker.py	Thu Apr 01 19:48:36 2021 +0200
+++ b/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyChecker.py	Fri Apr 02 15:35:44 2021 +0200
@@ -20,7 +20,7 @@
     Codes = [
         # Python-specifics
         "Y101", "Y102", "Y103", "Y104", "Y105", "Y106", "Y107", "Y108",
-        "Y109", "Y110", "Y111", "Y112",
+        "Y109", "Y110", "Y111", "Y112", "Y113", "Y114", "Y115", "Y116",
         
         # Comparations
     ]
@@ -149,5 +149,10 @@
             self.__reportInvalidSyntax()
             return
         
+        # Add parent information
+        for node in ast.walk(self.__tree):
+            for child in ast.iter_child_nodes(node):
+                child.parent = node  # type: ignore
+        
         visitor = SimplifyNodeVisitor(self.__error)
         visitor.visit(self.__tree)
--- a/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyNodeVisitor.py	Thu Apr 01 19:48:36 2021 +0200
+++ b/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyNodeVisitor.py	Fri Apr 02 15:35:44 2021 +0200
@@ -9,6 +9,7 @@
 
 import ast
 import collections
+import itertools
 
 try:
     from ast import unparse
@@ -99,6 +100,8 @@
         self.__check103(node)
         self.__check106(node)
         self.__check108(node)
+        self.__check114(node)
+        self.__check116(node)
         
         self.generic_visit(node)
     
@@ -111,6 +114,7 @@
         """
         self.__check104(node)
         self.__check110_111(node)
+        self.__check113(node)
         
         self.generic_visit(node)
     
@@ -126,6 +130,17 @@
         
         self.generic_visit(node)
     
+    def visit_Call(self, node):
+        """
+        Public method to process a Call node.
+        
+        @param node reference to the Call node
+        @type ast.Call
+        """
+        self.__check115(node)
+        
+        self.generic_visit(node)
+    
     #############################################################
     ## Helper methods for the various checkers below
     #############################################################
@@ -155,6 +170,91 @@
         
         return [name for name, count in counter.items() if count > 1]
     
+    def __isConstantIncrease(self, expression):
+        """
+        Private method check the given expression for being a constant
+        increase.
+        
+        @param expression reference to the expression node
+        @type ast.AugAssign
+        @return flag indicating a constant increase
+        @rtype bool
+        """
+        return (
+            isinstance(expression.op, ast.Add) and
+            isinstance(expression.value, (ast.Constant, ast.Num))
+        )
+    
+    def __getIfBodyPairs(self, node):
+        """
+        Private method to extract a list of pairs of test and body for an
+        If node.
+        
+        @param node reference to the If node to be processed
+        @type ast.If
+        @return list of pairs of test and body
+        @rtype list of tuples of (ast.expr, [ast.stmt])
+        """
+        pairs = [(node.test, node.body)]
+        orelse = node.orelse
+        while (
+            isinstance(orelse, list) and
+            len(orelse) == 1 and
+            isinstance(orelse[0], ast.If)
+        ):
+            pairs.append((orelse[0].test, orelse[0].body))
+            orelse = orelse[0].orelse
+        return pairs
+    
+    def __isSameBody(self, body1, body2):
+        """
+        Private method check, if the given bodies are equivalent.
+        
+        @param body1 list of statements of the first body
+        @type list of ast.stmt
+        @param body2 list of statements of the second body
+        @type list of ast.stmt
+        @return flag indicating identical bodies
+        @rtype bool
+        """
+        if len(body1) != len(body2):
+            return False
+        for a, b in zip(body1, body2):
+            try:
+                statementEqual = self.__isStatementEqual(a, b)
+            except RecursionError:  # maximum recursion depth
+                statementEqual = False
+            if not statementEqual:
+                return False
+        return True
+    
+    def __isStatementEqual(self, a: ast.stmt, b: ast.stmt) -> bool:
+        """
+        Private method to check, if two statements are equal.
+        
+        @param a reference to the first statement
+        @type ast.stmt
+        @param b reference to the second statement
+        @type ast.stmt
+        @return flag indicating if the two statements are equal
+        @rtype bool
+        """
+        if type(a) is not type(b):
+            return False
+        
+        if isinstance(a, ast.AST):
+            for k, v in vars(a).items():
+                if k in ("lineno", "col_offset", "ctx", "end_lineno",
+                         "parent"):
+                    continue
+                if not self.__isStatementEqual(v, getattr(b, k)):
+                    return False
+            return True
+        elif isinstance(a, list):
+            return all(itertools.starmap(self.__isStatementEqual, zip(a, b)))
+        else:
+            return a == b
+    
     #############################################################
     ## Methods to check for possible code simplifications below
     #############################################################
@@ -429,12 +529,12 @@
         #         return False
         # return True
         if (
-            len(node.body) == 1
-            and isinstance(node.body[0], ast.If)
-            and len(node.body[0].body) == 1
-            and isinstance(node.body[0].body[0], ast.Return)
-            and isinstance(node.body[0].body[0].value, BOOL_CONST_TYPES)
-            and hasattr(node.body[0].body[0].value, "value")
+            len(node.body) == 1 and
+            isinstance(node.body[0], ast.If) and
+            len(node.body[0].body) == 1 and
+            isinstance(node.body[0].body[0], ast.Return) and
+            isinstance(node.body[0].body[0].value, BOOL_CONST_TYPES) and
+            hasattr(node.body[0].body[0].value, "value")
         ):
             check = unparse(node.body[0].test)
             target = unparse(node.target)
@@ -451,7 +551,7 @@
     
     def __check112(self, node):
         """
-        Public method to check for non-capitalized calls to environment
+        Private method to check for non-capitalized calls to environment
         variables.
         
         @param node reference to the AST node to be checked
@@ -460,17 +560,17 @@
         # os.environ["foo"]
         # os.environ.get("bar")
         isIndexCall = (
-            isinstance(node.value, ast.Subscript)
-            and isinstance(node.value.value, ast.Attribute)
-            and isinstance(node.value.value.value, ast.Name)
-            and node.value.value.value.id == "os"
-            and node.value.value.attr == "environ"
-            and (
+            isinstance(node.value, ast.Subscript) and
+            isinstance(node.value.value, ast.Attribute) and
+            isinstance(node.value.value.value, ast.Name) and
+            node.value.value.value.id == "os" and
+            node.value.value.attr == "environ" and
+            (
                 (
-                    isinstance(node.value.slice, ast.Index)
-                    and isinstance(node.value.slice.value, STR_TYPES)
-                )
-                or isinstance(node.value.slice, ast.Constant)
+                    isinstance(node.value.slice, ast.Index) and
+                    isinstance(node.value.slice.value, STR_TYPES)
+                ) or
+                isinstance(node.value.slice, ast.Constant)
             )
         )
         if isIndexCall:
@@ -491,15 +591,15 @@
             hasChange = envName != envName.upper()
 
         isGetCall = (
-            isinstance(node.value, ast.Call)
-            and isinstance(node.value.func, ast.Attribute)
-            and isinstance(node.value.func.value, ast.Attribute)
-            and isinstance(node.value.func.value.value, ast.Name)
-            and node.value.func.value.value.id == "os"
-            and node.value.func.value.attr == "environ"
-            and node.value.func.attr == "get"
-            and len(node.value.args) in [1, 2]
-            and isinstance(node.value.args[0], STR_TYPES)
+            isinstance(node.value, ast.Call) and
+            isinstance(node.value.func, ast.Attribute) and
+            isinstance(node.value.func.value, ast.Attribute) and
+            isinstance(node.value.func.value.value, ast.Name) and
+            node.value.func.value.value.id == "os" and
+            node.value.func.value.attr == "environ" and
+            node.value.func.attr == "get" and
+            len(node.value.args) in [1, 2] and
+            isinstance(node.value.args[0], STR_TYPES)
         )
         if isGetCall:
             call = node.value
@@ -529,6 +629,164 @@
         
         self.__error(node.lineno - 1, node.col_offset, "Y112", expected,
                      original)
+    
+    def __check113(self, node):
+        """
+        Private method to check for loops in which "enumerate" should be
+        used.
+        
+        @param node reference to the AST node to be checked
+        @type ast.For
+        """
+        # idx = 0
+        # for el in iterable:
+        #     ...
+        #     idx += 1
+        variableCandidates = []
+        for expression in node.body:
+            if (
+                isinstance(expression, ast.AugAssign) and
+                self.__isConstantIncrease(expression) and
+                isinstance(expression.target, ast.Name)
+            ):
+                variableCandidates.append(expression.target)
+
+        for candidate in variableCandidates:
+            self.__error(candidate.lineno - 1, candidate.col_offset, "Y113",
+                         unparse(candidate))
+    
+    def __check114(self, node):
+        """
+        Private method to check for alternative if clauses with identical
+        bodies.
+        
+        @param node reference to the AST node to be checked
+        @type ast.If
+        """
+        # if a:
+        #     b
+        # elif c:
+        #     b
+        ifBodyPairs = self.__getIfBodyPairs(node)
+        errorPairs = []
+        for ifbody1, ifbody2 in itertools.combinations(ifBodyPairs, 2):
+            if self.__isSameBody(ifbody1[1], ifbody2[1]):
+                errorPairs.append((ifbody1, ifbody2))
+        for ifbody1, ifbody2 in errorPairs:
+            self.__error(ifbody1[0].lineno - 1, ifbody1[0].col_offset, "Y114",
+                         unparse(ifbody1[0]), unparse(ifbody2[0]))
+    
+    def __check115(self, node):
+        """
+        Private method to to check for places where open() is called without
+        a context handler.
+        
+        @param node reference to the AST node to be checked
+        @type ast.Call
+        """
+        # f = open(...)
+        #. ..  # (do something with f)
+        # f.close()
+        if (
+            isinstance(node.func, ast.Name) and
+            node.func.id == "open" and
+            not isinstance(node.parent, ast.withitem)
+        ):
+            self.__error(node.lineno - 1, node.col_offset, "Y115")
+    
+    def __check116(self, node):
+        """
+        Private method to check for places with 3 or more consecutive
+        if-statements with direct returns.
+        
+        * Each if-statement must be a check for equality with the
+          same variable
+        * Each if-statement must just have a "return"
+        * Else must also just have a return
+        
+        @param node reference to the AST node to be checked
+        @type ast.If
+        """
+        # if a == "foo":
+        #     return "bar"
+        # elif a == "bar":
+        #     return "baz"
+        # elif a == "boo":
+        #     return "ooh"
+        # else:
+        #    return 42
+        if (
+            isinstance(node.test, ast.Compare) and
+            isinstance(node.test.left, ast.Name) and
+            len(node.test.ops) == 1 and
+            isinstance(node.test.ops[0], ast.Eq) and
+            len(node.test.comparators) == 1 and
+            isinstance(node.test.comparators[0], AST_CONST_TYPES) and
+            len(node.body) == 1 and
+            isinstance(node.body[0], ast.Return) and
+            len(node.orelse) == 1 and
+            isinstance(node.orelse[0], ast.If)
+        ):
+            variable = node.test.left
+            child = node.orelse[0]
+            elseValue = None
+            if isinstance(node.test.comparators[0], ast.Str):
+                keyValuePairs = {
+                    node.test.comparators[0].s:
+                        unparse(node.body[0].value).strip("'")
+                }
+            elif isinstance(node.test.comparators[0], ast.Num):
+                keyValuePairs = {
+                    node.test.comparators[0].n:
+                        unparse(node.body[0].value).strip("'")
+                }
+            else:
+                keyValuePairs = {
+                    node.test.comparators[0].value:
+                        unparse(node.body[0].value).strip("'")
+                }
+            while child:
+                if not (
+                    isinstance(child.test, ast.Compare) and
+                    isinstance(child.test.left, ast.Name) and
+                    child.test.left.id == variable.id and
+                    len(child.test.ops) == 1 and
+                    isinstance(child.test.ops[0], ast.Eq) and
+                    len(child.test.comparators) == 1 and
+                    isinstance(child.test.comparators[0], AST_CONST_TYPES) and
+                    len(child.body) == 1 and
+                    isinstance(child.body[0], ast.Return) and
+                    len(child.orelse) <= 1
+                ):
+                    return
+                
+                if isinstance(child.test.comparators[0], ast.Str):
+                    key = child.test.comparators[0].s
+                elif isinstance(child.test.comparators[0], ast.Num):
+                    key = child.test.comparators[0].n
+                else:
+                    key = child.test.comparators[0].value
+                keyValuePairs[key] = unparse(child.body[0].value).strip("'")
+                if len(child.orelse) == 1:
+                    if isinstance(child.orelse[0], ast.If):
+                        child = child.orelse[0]
+                    elif isinstance(child.orelse[0], ast.Return):
+                        elseValue = unparse(child.orelse[0].value)
+                        child = None
+                    else:
+                        return
+                else:
+                    child = None
+            
+            if len(keyValuePairs) < 3:
+                return
+            
+            if elseValue:
+                ret = f"{keyValuePairs}.get({variable.id}, {elseValue})"
+            else:
+                ret = f"{keyValuePairs}.get({variable.id})"
+            
+            self.__error(node.lineno - 1, node.col_offset, "Y116", ret)
 
 #
 # eflag: noqa = M891
--- a/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/translations.py	Thu Apr 01 19:48:36 2021 +0200
+++ b/eric6/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/translations.py	Fri Apr 02 15:35:44 2021 +0200
@@ -14,42 +14,55 @@
 _simplifyMessages = {
     "Y101": QCoreApplication.translate(
         "SimplifyChecker",
-        """Multiple "isinstance()" calls which can be merged into a single """
-        """call for variable '{0}'"""),
+        '''Multiple "isinstance()" calls which can be merged into a single '''
+        '''call for variable "{0}"'''),
     "Y102": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use a single if-statement instead of nested if-statements"""),
+        '''Use a single if-statement instead of nested if-statements'''),
     "Y103": QCoreApplication.translate(
         "SimplifyChecker",
-        """Return the condition "{0}" directly"""),
+        '''Return the condition "{0}" directly'''),
     "Y104": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "yield from {0}" """),
+        '''Use "yield from {0}"'''),
     "Y105": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "with contextlib.suppress({0}):" """),
+        '''Use "with contextlib.suppress({0}):"'''),
     "Y106": QCoreApplication.translate(
         "SimplifyChecker",
-        """Handle error-cases first"""),
+        '''Handle error-cases first'''),
     "Y107": QCoreApplication.translate(
         "SimplifyChecker",
-        """Don't use return in try/except and finally"""),
+        '''Don't use return in try/except and finally'''),
     "Y108": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use ternary operator "{0} = {1} if {2} else {3}" """
-        """instead of if-else-block"""),
+        '''Use ternary operator "{0} = {1} if {2} else {3}"'''
+        '''instead of if-else-block'''),
     "Y109": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "{0} in {1}" instead of "{2}" """),
+        '''Use "{0} in {1}" instead of "{2}"'''),
     "Y110": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "return any({0} for {1} in {2})" """),
+        '''Use "return any({0} for {1} in {2})"'''),
     "Y111": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "return all({0} for {1} in {2})" """),
+        '''Use "return all({0} for {1} in {2})"'''),
     "Y112": QCoreApplication.translate(
         "SimplifyChecker",
-        """Use "{0}" instead of "{1}" """),
+        '''Use "{0}" instead of "{1}"'''),
+    "Y113": QCoreApplication.translate(
+        "SimplifyChecker",
+        '''Use enumerate instead of "{0}"'''),
+    "Y114": QCoreApplication.translate(
+        "SimplifyChecker",
+        '''Use logical or ("({0}) or ({1})") and a single body'''),
+    "Y115": QCoreApplication.translate(
+        "SimplifyChecker",
+        '''Use context handler for opening files'''),
+    "Y116": QCoreApplication.translate(
+        "SimplifyChecker",
+        '''Use a dictionary lookup instead of 3+ if/elif-statements: '''
+        '''return {0}'''),
 }
 
 _simplifyMessagesSampleArgs = {
@@ -58,8 +71,11 @@
     "Y104": ["iterable"],
     "Y105": ["Exception"],
     "Y108": ["foo", "bar", "condition", "baz"],
-    "Y109": ["foo", "[1, 2]", "foo == 1 or foo == 2"],
+    "Y109": ["foo", "[1, 42]", "foo == 1 or foo == 42"],
     "Y110": ["check", "foo", "iterable"],
     "Y111": ["check", "foo", "iterable"],
     "Y112": ["FOO", "foo"],
+    "Y113": ["foo"],
+    "Y114": ["foo > 42", "bar < 42"],
+    "Y116": ["mapping.get(foo, 42)"]
 }

eric ide

mercurial