Mercurial Repositories > eric / changeset
changeset
Code Style Checker
Thu, 28 Jul 2022 19:44:54 +0200
- author
- Detlev Offenbach <detlev@die-offenbachs.de>
- date
- Thu, 28 Jul 2022 19:44:54 +0200
- branch
- eric7
- changeset 9277
- 471c5a263d53
- parent 9276
- e6748a5e24b9
- child 9278
- 36448ca469c2
Code Style Checker
- updated the simplifications checker to support more cases
--- a/docs/changelog Thu Jul 28 14:19:57 2022 +0200 +++ b/docs/changelog Thu Jul 28 19:44:54 2022 +0200 @@ -6,6 +6,7 @@ -- extended the Naming style checker to be more PEP8 compliant -- updated imports checker to support banned module patterns -- updated the annotations checker to support more cases + -- updated the simplifications checker to support more cases - MicroPython -- added capability to connect to devices for which only the serial port name is available
--- a/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyChecker.py Thu Jul 28 14:19:57 2022 +0200 +++ b/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyChecker.py Thu Jul 28 19:44:54 2022 +0200 @@ -42,6 +42,7 @@ "Y120", "Y121", "Y122", + "Y123", # Python-specifics not part of flake8-simplify "Y181", "Y182", @@ -66,6 +67,13 @@ # General Code Style "Y401", "Y402", + # Additional Checks + "Y901", + "Y904", + "Y905", + "Y906", + "Y907", + "Y909", ] def __init__(self, source, filename, tree, selected, ignored, expected, repeat): @@ -167,9 +175,30 @@ return # Add parent information - for node in ast.walk(self.__tree): - for child in ast.iter_child_nodes(node): - child.parent = node # type: ignore + self.__addMeta(self.__tree) visitor = SimplifyNodeVisitor(self.__error) visitor.visit(self.__tree) + + def __addMeta(self, root, level=0): + """ + Private method to amend the nodes of the given AST tree with backward and + forward references. + + @param root reference to the root node of the tree + @type ast.AST + @param level nesting level (defaults to 0) + @type int (optional) + """ + previousSibling = None + for node in ast.iter_child_nodes(root): + if level == 0: + node.parent = root + node.previous_sibling = previousSibling + node.next_sibling = None + if previousSibling: + node.previous_sibling.next_sibling = node + previousSibling = node + for child in ast.iter_child_nodes(node): + child.parent = node + self.__addMeta(node, level=level + 1)
--- a/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyNodeVisitor.py Thu Jul 28 14:19:57 2022 +0200 +++ b/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/SimplifyNodeVisitor.py Thu Jul 28 19:44:54 2022 +0200 @@ -11,6 +11,7 @@ import collections import copy import itertools +import json try: from ast import unparse @@ -18,8 +19,8 @@ # Python < 3.9 from .ast_unparse import unparse -###################################################################### -## The following code is derived from the flake8-simplify package. +############################################################################### +## The following code is derived from the flake8-simplify package (v0.19.2). ## ## Original License: ## @@ -44,8 +45,7 @@ ## LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING ## FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS ## IN THE SOFTWARE. -###################################################################### -# TODO: update to bandit v0.19.2 +############################################################################### BOOL_CONST_TYPES = (ast.Constant, ast.NameConstant) AST_CONST_TYPES = (ast.Constant, ast.NameConstant, ast.Str, ast.Num) @@ -90,6 +90,8 @@ @type ast.Assign """ self.__check181(node) + self.__check904(node) + self.__check909(node) self.generic_visit(node) @@ -123,6 +125,7 @@ self.__check114(node) self.__check116(node) self.__check122(node) + self.__check123(node) self.generic_visit(node) @@ -176,6 +179,9 @@ self.__check182(node) self.__check401(node) self.__check402(node) + self.__check901(node) + self.__check905(node) + self.__check906(node) self.generic_visit(node) @@ -236,6 +242,17 @@ self.__check208(node) self.generic_visit(node) + + def visit_Subscript(self, node): + """ + Public method to process a Subscript node. + + @param node reference to the Subscript node + @type ast.Subscript + """ + self.__check907(node) + + self.generic_visit(node) ############################################################# ## Helper methods for the various checkers below @@ -266,22 +283,21 @@ return [name for name, count in counter.items() if count > 1] - def __isConstantIncrease(self, expression): + def __isConstantIncrease(self, expr): """ - Private method check the given expression for being a constant - increase. - - @param expression reference to the expression node + Private method to check an expression for being a constant increase. + + @param expr reference to the node to be checked @type ast.AugAssign @return flag indicating a constant increase @rtype bool """ - return isinstance(expression.op, ast.Add) and ( + return isinstance(expr.op, ast.Add) and ( ( - isinstance(expression.value, ast.Constant) - and isinstance(expression.value.value, int) + isinstance(expr.value, ast.Constant) + and expr.value.value == 1 ) - or isinstance(expression.value, ast.Num) + or (isinstance(expr.value, ast.Num) and expr.value.n == 1) ) def __getIfBodyPairs(self, node): @@ -415,6 +431,36 @@ newNode.ops = [op] return newNode + def __expressionUsesVariable(self, expr, var): + """ + Private method to check, if a variable is used by an expression. + + @param expr expression node to be checked + @type ast.expr + @param var variable name to be checked for + @type str + @return flag indicating the expression uses the variable + @rtype bool + """ + return var in unparse(expr) + # This is WAY too broad, but it's better to have false-negatives than + # false-positives. + + def __bodyContainsContinue(self, stmts): + """ + Private method to check, if a list of statements contain a 'continue' statement. + + @param stmts list of statements + @type list of ast.stmt + @return flag indicating a continue statement + @rtype bool + """ + return any( + isinstance(stmt, ast.Continue) + or (isinstance(stmt, ast.If) and self.__bodyContainsContinue(stmt.body)) + for stmt in stmts + ) + ############################################################# ## Methods to check for possible code simplifications below ############################################################# @@ -514,6 +560,7 @@ or not isinstance(node.body[0].value.value, ast.Name) or node.target.id != node.body[0].value.value.id or node.orelse != [] + or isinstance(node.parent, ast.AsyncFunctionDef) ): iterable = unparse(node.iter) self.__error(node.lineno - 1, node.col_offset, "Y104", iterable) @@ -609,7 +656,6 @@ if ( (tryHasReturn or exceptHasReturn) and finallyHasReturn - and finallyReturn is not None ): self.__error(finallyReturn.lineno - 1, finallyReturn.col_offset, "Y107") @@ -645,7 +691,19 @@ and node.body[0].targets[0].id == node.orelse[0].targets[0].id and not isinstance(node.parent, ast.If) ): - assign = unparse(node.body[0].targets[0]) + targetVar = node.body[0].targets[0] + assign = unparse(targetVar) + + # It's part of a bigger if-elseif block: + if isinstance(node.parent, ast.If): + for n in node.parent.body: + if ( + isinstance(n, ast.Assign) + and isinstance(n.targets[0], ast.Name) + and n.targets[0].id == targetVar.id + ): + return + body = unparse(node.body[0].value) cond = unparse(node.test) orelse = unparse(node.orelse[0].value) @@ -720,6 +778,7 @@ 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") + and isinstance(node.next_sibling, ast.Return) ): check = unparse(node.body[0].test) target = unparse(node.target) @@ -729,9 +788,15 @@ node.lineno - 1, node.col_offset, "Y110", check, target, iterable ) elif node.body[0].body[0].value.value is False: - check = "not " + check - if check.startswith("not not "): - check = check[len("not not ") :] + isCompoundExpression = " and " in check or " or " in check + + if isCompoundExpression: + check = f"not ({check})" + else: + if check.startswith("not "): + check = check[len("not "):] + else: + check = f"not {check}" self.__error( node.lineno - 1, node.col_offset, "Y111", check, target, iterable ) @@ -826,19 +891,44 @@ # 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) + if not self.__bodyContainsContinue(node.body): + # Find variables that might just count the iteration of the current loop + 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) + strCandidates = [unparse(x) for x in variableCandidates] + + olderSiblings = [] + for olderSibling in node.parent.body: + if olderSibling is node: + break + olderSiblings.append(olderSibling) - for candidate in variableCandidates: - self.__error( - candidate.lineno - 1, candidate.col_offset, "Y113", unparse(candidate) - ) + matches = [ + n.targets[0] + for n in olderSiblings + if isinstance(n, ast.Assign) + and len(n.targets) == 1 + and isinstance(n.targets[0], ast.Name) + and unparse(n.targets[0]) in strCandidates + ] + if len(matches) == 0: + return + + sibling = node.previous_sibling + while sibling is not None: + sibling = sibling.previous_sibling + + for match in matches: + variable = unparse(match) + self.__error( + match.lineno - 1, match.col_offset, "Y113", variable + ) def __check114(self, node): """ @@ -854,7 +944,10 @@ # b ifBodyPairs = self.__getIfBodyPairs(node) errorPairs = [] - for ifbody1, ifbody2 in itertools.combinations(ifBodyPairs, 2): + for i in range(len(ifBodyPairs) - 1): + # It's not all combinations because of this: + ifbody1 = ifBodyPairs[i] + ifbody2 = ifBodyPairs[i + 1] if self.__isSameBody(ifbody1[1], ifbody2[1]): errorPairs.append((ifbody1, ifbody2)) for ifbody1, ifbody2 in errorPairs: @@ -925,11 +1018,14 @@ else: bodyValueStr = "None" if isinstance(node.test.comparators[0], ast.Str): - keyValuePairs = {node.test.comparators[0].s: bodyValueStr} + value = ( + bodyValueStr + if bodyValueStr[0] == '"' and bodyValueStr[-1] == '"' else + bodyValueStr[1:-1] + ) + keyValuePairs = {node.test.comparators[0].s: value} elif isinstance(node.test.comparators[0], ast.Num): - keyValuePairs = { - node.test.comparators[0].n: bodyValueStr, - } + keyValuePairs = {node.test.comparators[0].n: bodyValueStr} else: keyValuePairs = {node.test.comparators[0].value: bodyValueStr} while child: @@ -947,13 +1043,22 @@ ): return + returnCall = child.body[0] + if isinstance(returnCall.value, ast.Call): + 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("'") + + value = unparse(child.body[0].value) + if value[0] == '"' and value[-1] == '"': + value = value[1:-1] + keyValuePairs[key] = value + if len(child.orelse) == 1: if isinstance(child.orelse[0], ast.If): child = child.orelse[0] @@ -1125,6 +1230,69 @@ dictname = unparse(node.test.comparators[0]) self.__error(node.lineno - 1, node.col_offset, "Y122", dictname, key) + def __check123(self, node): + """ + Private method to check for complicated dictionary access with default value. + + @param node reference to the AST node to be checked + @type ast.If + """ + isPattern1 = ( + len(node.body) == 1 + and isinstance(node.body[0], ast.Assign) + and len(node.body[0].targets) == 1 + and isinstance(node.body[0].value, ast.Subscript) + and len(node.orelse) == 1 + and isinstance(node.orelse[0], ast.Assign) + and len(node.orelse[0].targets) == 1 + and isinstance(node.test, ast.Compare) + and len(node.test.ops) == 1 + and isinstance(node.test.ops[0], ast.In) + ) + + # just like pattern_1, but using NotIn and reversing if/else + isPattern2 = ( + len(node.body) == 1 + and isinstance(node.body[0], ast.Assign) + and len(node.orelse) == 1 + and isinstance(node.orelse[0], ast.Assign) + and isinstance(node.orelse[0].value, ast.Subscript) + and isinstance(node.test, ast.Compare) + and len(node.test.ops) == 1 + and isinstance(node.test.ops[0], ast.NotIn) + ) + + if isPattern1: + key = node.test.left + if unparse(key) != unparse(node.body[0].value.slice): + return + assignToIfBody = node.body[0].targets[0] + assignToElse = node.orelse[0].targets[0] + if unparse(assignToIfBody) != unparse(assignToElse): + return + dictName = node.test.comparators[0] + defaultValue = node.orelse[0].value + valueNode = node.body[0].targets[0] + keyStr = unparse(key) + dictStr = unparse(dictName) + defaultStr = unparse(defaultValue) + valueStr = unparse(valueNode) + elif isPattern2: + key = node.test.left + if unparse(key) != unparse(node.orelse[0].value.slice): + return + dictName = node.test.comparators[0] + defaultValue = node.body[0].value + valueNode = node.body[0].targets[0] + keyStr = unparse(key) + dictStr = unparse(dictName) + defaultStr = unparse(defaultValue) + valueStr = unparse(valueNode) + else: + return + self.__error(node.lineno - 1, node.col_offset, "Y123", valueStr, dictStr, + keyStr, defaultStr) + def __check181(self, node): """ Private method to check for assignments that could be converted into @@ -1550,6 +1718,171 @@ if hasBareNumeric and not isException: self.__error(node.lineno - 1, node.col_offset, "Y402") + def __check901(self, node): + """ + Private method to check for unnecessary bool conversion. + + @param node reference to the AST node to be checked + @type ast.Call + """ + if ( + isinstance(node.func, ast.Name) + and node.func.id == "bool" + and len(node.args) == 1 + and isinstance(node.args[0], ast.Compare) + ): + actual = unparse(node) + expected = unparse(node.args[0]) + self.__error(node.lineno - 1, node.col_offset, "Y901", expected, actual) + + def __check904(self, node): + """ + Private method to check for dictionary initialization. + + @param node reference to the AST node to be checked + @type ast.Assign + """ + # a = {}; a['b'] = 'c' + n2 = node.next_sibling + if ( + isinstance(node.value, ast.Dict) + and isinstance(n2, ast.Assign) + and len(n2.targets) == 1 + and len(node.targets) == 1 + and isinstance(n2.targets[0], ast.Subscript) + and isinstance(n2.targets[0].value, ast.Name) + and isinstance(node.targets[0], ast.Name) + and n2.targets[0].value.id == node.targets[0].id + ): + dictName = unparse(node.targets[0]) + if not self.__expressionUsesVariable(n2.value, dictName): + self.__error(node.lineno - 1, node.col_offset, "Y904", dictName) + + def __check905(self, node): + """ + Private method to check for list initialization by splitting a string. + + @param node reference to the AST node to be checked + @type ast.Call + """ + if ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "split" + and isinstance(node.func.value, (ast.Str, ast.Constant)) + ): + if isinstance(node.func.value, ast.Constant): + value = node.func.value.value + else: + value = node.func.value.s + + expected = json.dumps(value.split()) + actual = unparse(node.func.value) + ".split()" + self.__error(node.lineno - 1, node.col_offset, "Y905", expected, actual) + + def __check906(self, node): + """ + Private method to check for unnecessary nesting of os.path.join(). + + @param node reference to the AST node to be checked + @type ast.Call + """ + # __IGNORE_WARNING_D234r__ + def getOsPathJoinArgs(node): + names = [] + for arg in node.args: + if ( + isinstance(arg, ast.Call) + and isinstance(arg.func, ast.Attribute) + and isinstance(arg.func.value, ast.Attribute) + and isinstance(arg.func.value.value, ast.Name) + and arg.func.value.value.id == "os" + and arg.func.value.attr == "path" + and arg.func.attr == "join" + ): + names += getOsPathJoinArgs(arg) + elif isinstance(arg, ast.Name): + names.append(arg.id) + elif isinstance(arg, ast.Str): + names.append(f"'{arg.s}'") + return names + + if ( + isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Attribute) + and isinstance(node.func.value.value, ast.Name) + and node.func.value.value.id == "os" + and node.func.value.attr == "path" + and node.func.attr == "join" + and len(node.args) == 2 + and any( + ( + isinstance(arg, ast.Call) + and isinstance(arg.func, ast.Attribute) + and isinstance(arg.func.value, ast.Attribute) + and isinstance(arg.func.value.value, ast.Name) + and arg.func.value.value.id == "os" + and arg.func.value.attr == "path" + and arg.func.attr == "join" + ) + for arg in node.args + ) + ): + names = getOsPathJoinArgs(node) + + actual = unparse(node) + expected = "os.path.join({0})".format(', '.join(names)) + self.__error(node.lineno - 1, node.col_offset, "Y906", expected, actual) + + def __check907(self, node): + """ + Private method to check for Union type annotation with None. + + @param node reference to the AST node to be checked + @type ast.Subscript + """ + if (isinstance(node.value, ast.Name) and node.value.id == "Union"): + if ( + isinstance(node.slice, ast.Index) + and isinstance(node.slice.value, ast.Tuple) + ): + # Python 3.8 + tupleVar = node.slice.value + elif isinstance(node.slice, ast.Tuple): + # Python 3.9+ + tupleVar = node.slice + else: + return + + hasNone = False + others = [] + for elt in tupleVar.elts: + if isinstance(elt, BOOL_CONST_TYPES) and elt.value is None: + hasNone = True + else: + others.append(elt) + + if len(others) == 1 and hasNone: + type_ = unparse(others[0]) + self.__error( + node.lineno - 1, node.col_offset, "Y907", type_, unparse(node) + ) + + def __check909(self, node): + """ + Private method to check for reflexive assignments. + + @param node reference to the AST node to be checked + @type ast.Assign + """ + names = [] + if isinstance(node.value, (ast.Name, ast.Subscript, ast.Tuple)): + names.append(unparse(node.value)) + for target in node.targets: + names.append(unparse(target)) + + if len(names) != len(set(names)) and not isinstance(node.parent, ast.ClassDef): + srccode = unparse(node) + self.__error(node.lineno - 1, node.col_offset, "Y909", srccode) # # eflag: noqa = M891
--- a/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/translations.py Thu Jul 28 14:19:57 2022 +0200 +++ b/src/eric7/Plugins/CheckerPlugins/CodeStyleChecker/Simplify/translations.py Thu Jul 28 19:44:54 2022 +0200 @@ -85,6 +85,9 @@ "Y122": QCoreApplication.translate( "SimplifyChecker", '''Use "{0}.get({1})" instead of "if {1} in {0}: {0}[{1}]"''' ), + "Y123": QCoreApplication.translate( + "SimplifyChecker", '''Use "{0} = {1}.get({2}, {3})" instead of an if-block''' + ), # Python-specifics not part of flake8-simplify "Y181": QCoreApplication.translate( "SimplifyChecker", '''Use "{0}" instead of "{1}"''' @@ -151,6 +154,25 @@ "Y402": QCoreApplication.translate( "SimplifyChecker", """Use keyword-argument instead of magic number""" ), + + "Y901": QCoreApplication.translate( + "SimplifyChecker", '''Use "{0}" instead of "{1}"''' + ), + "Y904": QCoreApplication.translate( + "SimplifyChecker", """Initialize dictionary "{0}" directly""" + ), + "Y905": QCoreApplication.translate( + "SimplifyChecker", '''Use "{0}" instead of "{1}"''' + ), + "Y906": QCoreApplication.translate( + "SimplifyChecker", '''Use "{0}" instead of "{1}"''' + ), + "Y907": QCoreApplication.translate( + "SimplifyChecker", '''Use "Optional[{0}]" instead of "{1}"''' + ), + "Y909": QCoreApplication.translate( + "SimplifyChecker", '''Remove reflexive assignment "{0}"''' + ), } _simplifyMessagesSampleArgs = { @@ -173,6 +195,8 @@ "Y120": ["Foo"], "Y121": ["FooBar", "Foo"], "Y122": ["bar_dict", "'foo'"], + "Y123": ["foo", "fooDict", "bar", "default"], + "Y124": ["foo", "bar"], # Python-specifics not part of flake8-simplify "Y181": ["foo += 42", "foo = foo + 42"], "Y182": ["super()"], @@ -193,4 +217,14 @@ # Opinionated "Y301": ["42", "foo"], # General Code Style + + # Additional checks + "Y901": ["foo == bar", "bool(foo == bar)"], + "Y904": ["foo"], + "Y905": [ + """["de", "com", "net", "org"]""", """domains = "de com net org".split()""" + ], + "Y906": ["os.path.join(a, b, c)", "os.path.join(a,os.path.join(b,c))"], + "Y907": ["int", "Union[int, None]"], + "Y909": ["foo = foo"], }
