From 12f94c5e9d0011293dc2dd49653cbb04b86ffcb0 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Tue, 4 Jul 2023 17:09:47 +0200 Subject: [PATCH 01/11] IdemDebug: First prototype of IdemDebug trafo --- transformations/transformations/__init__.py | 1 + transformations/transformations/idem_debug.py | 173 ++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 transformations/transformations/idem_debug.py diff --git a/transformations/transformations/__init__.py b/transformations/transformations/__init__.py index 1ef23e183..dc0cfd7d5 100644 --- a/transformations/transformations/__init__.py +++ b/transformations/transformations/__init__.py @@ -17,6 +17,7 @@ from transformations.utility_routines import * # noqa from transformations.scc_cuf import * # noqa from transformations.pool_allocator import * # noqa +from transformations.idem_debug import * # noqa try: __version__ = version("transformations") diff --git a/transformations/transformations/idem_debug.py b/transformations/transformations/idem_debug.py new file mode 100644 index 000000000..6cd6f0d40 --- /dev/null +++ b/transformations/transformations/idem_debug.py @@ -0,0 +1,173 @@ +# (C) Copyright 2018- ECMWF. +# This software is licensed under the terms of the Apache Licence Version 2.0 +# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. +# In applying this licence, ECMWF does not waive the privileges and immunities +# granted to it by virtue of its status as an intergovernmental organisation +# nor does it submit to any jurisdiction. + +import pdb +import operator as _op +from loki import ( + Transformation, FindNodes, CallStatement, Assignment, Scalar, RangeIndex, + simplify, Sum, Product, IntLiteral, DeferredTypeSymbol, as_tuple, SubstituteExpressions, + symbolic_op, warning +) + +__all__ = ['IdemDebugTransformation'] + +class IdemDebugTransformation(Transformation): + + @staticmethod + def _resolve_range(dim, arg, count): + if not dim.upper: + if isinstance(arg.shape[count], RangeIndex): + upper = arg.shape[count].upper + else: + upper = arg.shape[count] + else: + upper = dim.upper + + if not dim.lower: + if isinstance(arg.shape[count], RangeIndex): + lower = arg.shape[count].lower + else: + lower = IntLiteral(1) + else: + lower = dim.lower + + return lower, upper + + @staticmethod + def range_to_sum(lower, upper): + return Sum((IntLiteral(1), upper, Product((IntLiteral(-1), lower)))) + + def get_explicit_arg_size(self, arg, dims): + if isinstance(arg, Scalar): + size = as_tuple(IntLiteral(1)) + else: + size = () + for dim in dims: + if isinstance(dim, RangeIndex): + size += as_tuple(simplify(self.range_to_sum(dim.lower, dim.upper))) + else: + size += as_tuple(dim) + + return size + + def get_implicit_arg_size(self, arg, dims): + size = () + for count, dim in enumerate(dims): + if isinstance(dim, RangeIndex): + if not dim.upper: + if isinstance(arg.shape[count], RangeIndex): + upper = arg.shape[count].upper + else: + upper = arg.shape[count] + else: + upper = dim.upper + if not dim.lower: + if isinstance(arg.shape[count], RangeIndex): + lower = arg.shape[count].lower + else: + lower = IntLiteral(1) + else: + lower = dim.lower + + size += as_tuple(self.range_to_sum(lower, upper)) + else: + size += as_tuple(dim) + + return size + + def argument_size_mismatch(self, routine, **kwargs): + + assign_map = {a.lhs: a.rhs for a in FindNodes(Assignment).visit(routine.body)} + + targets = as_tuple(kwargs.get('targets', None)) + calls = FindNodes(CallStatement).visit(routine.body) + calls = [c for c in calls if c.name.name.lower() in targets] + + for call in calls: + + # check if calls are enriched + if not call.routine: + continue + + arg_map_f = {carg: rarg for rarg, carg in call.arg_iter()} + arg_map_r = {rarg: carg for rarg, carg in call.arg_iter()} + for arg in call.arguments: + + # we can't proceed if dummy arg has assumed shape component + if isinstance(arg_map_f[arg], Scalar): + dummy_arg_size = as_tuple(IntLiteral(1)) + else: + if any(None in (dim.lower, dim.upper) + for dim in arg_map_f[arg].shape if isinstance(dim, RangeIndex)): + continue + dummy_arg_size = self.get_explicit_arg_size(arg_map_f[arg], arg_map_f[arg].shape) + + dummy_arg_size = SubstituteExpressions(arg_map_r).visit(dummy_arg_size) + dummy_arg_size = SubstituteExpressions(assign_map).visit(dummy_arg_size) + dummy_arg_size = Product(dummy_arg_size) + + + arg_size = () + alt_arg_size = () + # check if argument is scalar + if isinstance(arg, Scalar): + arg_size += as_tuple(IntLiteral(1)) + alt_arg_size += as_tuple(IntLiteral(1)) + else: + # check if arg has assumed size component + if any(None in (dim.lower, dim.upper) + for dim in arg.shape if isinstance(dim, RangeIndex)): + + # each dim must have explicit range-index to be sure of arg size + if not arg.dimensions: + continue + if not all(isinstance(dim, RangeIndex) for dim in arg.dimensions): + continue + if any(None in (dim.lower, dim.upper) for dim in arg.dimensions): + continue + + arg_size = self.get_explicit_arg_size(arg, arg.dimensions) + alt_arg_size = arg_size + else: + # compute dim sizes assuming single element + if arg.dimensions: + arg_size = self.get_implicit_arg_size(arg, arg.dimensions) + arg_size = as_tuple([IntLiteral(1) if not isinstance(a, Sum) else simplify(a) + for a in arg_size]) + else: + arg_size = self.get_explicit_arg_size(arg, arg.shape) + + # compute dim sizes assuming element reference + alt_arg_size = self.get_implicit_arg_size(arg, arg.dimensions) + alt_arg_size = as_tuple([simplify(Sum((Product((IntLiteral(-1), a)), + a.shape[i], IntLiteral(1)))) + if not isinstance(a, Sum) else simplify(a) + for i, a in enumerate(alt_arg_size)]) + alt_arg_size += self.get_explicit_arg_size(arg, arg.shape[len(arg.dimensions):]) + + stat = False + for i in range(len(arg_size)): + dims = () + for a in alt_arg_size[:i]: + dims += as_tuple(a) + for a in arg_size[i:]: + dims += as_tuple(a) + + dims = Product(dims) + + if symbolic_op(dims, _op.eq, dummy_arg_size): + stat = True + + if not stat: + warn = f'[Loki::IdemDebug] Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' + warn += f'in {call} in {routine}' + warning(warn) + + def transform_subroutine(self, routine, **kwargs): + + # check for argument size mismatch across subroutine calls + self.argument_size_mismatch(routine, **kwargs) From ca8832908b03f474fd539c52b20c627e11ceac0f Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Sat, 8 Jul 2023 16:53:29 +0200 Subject: [PATCH 02/11] ArgSizeMismatchRule: extended check_subroutine API to include scheduler targets --- .../lint_rules/ifs_coding_standards_2011.py | 18 +++++++++--------- loki/bulk/scheduler.py | 3 ++- loki/lint/linter.py | 6 +++--- loki/lint/rules.py | 14 +++++++------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lint_rules/lint_rules/ifs_coding_standards_2011.py b/lint_rules/lint_rules/ifs_coding_standards_2011.py index 29f1c4105..89f4fb19c 100644 --- a/lint_rules/lint_rules/ifs_coding_standards_2011.py +++ b/lint_rules/lint_rules/ifs_coding_standards_2011.py @@ -72,7 +72,7 @@ def visit_MultiConditional(self, o, **kwargs): return too_deep @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check the code body: Nesting of conditional blocks.''' too_deep = cls.NestingDepthVisitor(config['max_nesting_depth']).visit(subroutine.body) msg = f'Nesting of conditionals exceeds limit of {config["max_nesting_depth"]}' @@ -175,7 +175,7 @@ def _check_lhook_call(cls, call, subroutine, rule_report, pos='First'): rule_report.add(msg, call) @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check that first and last executable statements in the subroutine are conditionals with calls to DR_HOOK in their body and that the correct arguments are given to the call.''' @@ -220,7 +220,7 @@ class LimitSubroutineStatementsRule(GenericRule): # Coding standards 2.2 match_non_exec_intrinsic_node = re.compile(r'\s*(?:PRINT|FORMAT)', re.I) @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Count the number of nodes in the subroutine and check if they exceed a given maximum number. ''' @@ -252,7 +252,7 @@ class MaxDummyArgsRule(GenericRule): # Coding standards 3.6 } @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): """ Count the number of dummy arguments and report if given maximum number exceeded. @@ -274,7 +274,7 @@ class MplCdstringRule(GenericRule): # Coding standards 3.12 } @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check all calls to MPL subroutines for a CDSTRING.''' for call in FindNodes(ir.CallStatement).visit(subroutine.ir): if str(call.name).upper().startswith('MPL_'): @@ -310,7 +310,7 @@ def check_for_implicit_none(ast): return True @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): """ Check for IMPLICIT NONE in the subroutine's spec or any enclosing scope. @@ -390,7 +390,7 @@ def check_kind_literals(subroutine, types, allowed_type_kinds, rule_report): rule_report.add(msg, node) @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check for explicit kind information in constants and variable declarations. ''' @@ -440,7 +440,7 @@ class BannedStatementsRule(GenericRule): # Coding standards 4.11 } @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check for banned statements in intrinsic nodes.''' for intr in FindNodes(ir.Intrinsic).visit(subroutine.ir): for keyword in config['banned']: @@ -473,7 +473,7 @@ class Fortran90OperatorsRule(GenericRule): # Coding standards 4.15 } @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check for the use of Fortran 90 comparison operators.''' # We extract all `Comparison` expression nodes, grouped by the IR node they are in. # Then we run through all such pairs and check the symbol used in the source string. diff --git a/loki/bulk/scheduler.py b/loki/bulk/scheduler.py index 52819df62..eba859888 100644 --- a/loki/bulk/scheduler.py +++ b/loki/bulk/scheduler.py @@ -629,7 +629,8 @@ def process(self, transformation, reverse=False, item_filter=SubroutineItem, use if use_file_graph: for node in traversal: items = graph.nodes[node]['items'] - transformation.apply(items[0].source, item=items[0], items=items) + transformation.apply(items[0].source, item=items[0], items=items, + targets=items[0].targets) else: for item in traversal: if item_filter and not isinstance(item, item_filter): diff --git a/loki/lint/linter.py b/loki/lint/linter.py index 5d4892dc4..7a0fec1c9 100644 --- a/loki/lint/linter.py +++ b/loki/lint/linter.py @@ -120,7 +120,7 @@ def update_config(self, config): else: self.config[key] = val - def check(self, sourcefile, overwrite_rules=None, overwrite_config=None): + def check(self, sourcefile, overwrite_rules=None, overwrite_config=None, **kwargs): """ Check the given :data:`sourcefile` and compile a :any:`FileReport`. @@ -180,7 +180,7 @@ def check(self, sourcefile, overwrite_rules=None, overwrite_config=None): for rule in rules: timer.start() rule_report = RuleReport(rule, disabled=disabled_rules.get(rule.__name__)) - rule.check(sourcefile, rule_report, config[rule.__name__]) + rule.check(sourcefile, rule_report, config[rule.__name__], **kwargs) rule_report.elapsed_sec = timer.stop() file_report.add(rule_report) @@ -257,7 +257,7 @@ def __init__(self, linter, key=None, **kwargs): def transform_file(self, sourcefile, **kwargs): item = kwargs.get('item') - report = self.linter.check(sourcefile) + report = self.linter.check(sourcefile, **kwargs) self.counter += 1 if item: item.trafo_data[self._key] = report diff --git a/loki/lint/rules.py b/loki/lint/rules.py index a2f9848f7..b76e0c0c0 100644 --- a/loki/lint/rules.py +++ b/loki/lint/rules.py @@ -98,7 +98,7 @@ def check_module(cls, module, rule_report, config): """ @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): """ Perform rule checks on subroutine level @@ -114,7 +114,7 @@ def check_file(cls, sourcefile, rule_report, config): """ @classmethod - def check(cls, ast, rule_report, config): + def check(cls, ast, rule_report, config, **kwargs): """ Perform checks on all entities in the given IR object @@ -139,10 +139,10 @@ def check(cls, ast, rule_report, config): # Then recurse for all modules and subroutines in that file if hasattr(ast, 'modules') and ast.modules is not None: for module in ast.modules: - cls.check(module, rule_report, config) + cls.check(module, rule_report, config, **kwargs) if hasattr(ast, 'subroutines') and ast.subroutines is not None: for subroutine in ast.subroutines: - cls.check(subroutine, rule_report, config) + cls.check(subroutine, rule_report, config, **kwargs) # Perform checks on module level elif isinstance(ast, Module): @@ -154,19 +154,19 @@ def check(cls, ast, rule_report, config): # Then recurse for all subroutines in that module if hasattr(ast, 'subroutines') and ast.subroutines is not None: for subroutine in ast.subroutines: - cls.check(subroutine, rule_report, config) + cls.check(subroutine, rule_report, config, **kwargs) # Peform checks on subroutine level elif isinstance(ast, Subroutine): if is_rule_disabled(ast.ir, cls.identifiers()): return - cls.check_subroutine(ast, rule_report, config) + cls.check_subroutine(ast, rule_report, config, **kwargs) # Recurse for any procedures contained in a subroutine if hasattr(ast, 'members') and ast.members is not None: for member in ast.members: - cls.check(member, rule_report, config) + cls.check(member, rule_report, config, **kwargs) @classmethod def fix_module(cls, module, rule_report, config): From 22d407555174e1e1444d14bd680f94b5c2bd4a74 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 5 Jul 2023 21:16:24 +0200 Subject: [PATCH 03/11] ArgSizeMismatchRule: Turned IdemDebugTrafo into a linter rule --- .../lint_rules/debug_rules.py | 96 ++++++++++--------- transformations/transformations/__init__.py | 1 - 2 files changed, 51 insertions(+), 46 deletions(-) rename transformations/transformations/idem_debug.py => lint_rules/lint_rules/debug_rules.py (67%) diff --git a/transformations/transformations/idem_debug.py b/lint_rules/lint_rules/debug_rules.py similarity index 67% rename from transformations/transformations/idem_debug.py rename to lint_rules/lint_rules/debug_rules.py index 6cd6f0d40..5b4e96e3c 100644 --- a/transformations/transformations/idem_debug.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -5,56 +5,56 @@ # granted to it by virtue of its status as an intergovernmental organisation # nor does it submit to any jurisdiction. -import pdb import operator as _op from loki import ( Transformation, FindNodes, CallStatement, Assignment, Scalar, RangeIndex, simplify, Sum, Product, IntLiteral, DeferredTypeSymbol, as_tuple, SubstituteExpressions, - symbolic_op, warning + symbolic_op ) +from loki.lint import GenericRule, RuleType -__all__ = ['IdemDebugTransformation'] +class ArgSizeMismatchRule(GenericRule): + """ + Rule to check for argument size mismatch in subroutine/function calls + """ -class IdemDebugTransformation(Transformation): - - @staticmethod - def _resolve_range(dim, arg, count): - if not dim.upper: - if isinstance(arg.shape[count], RangeIndex): - upper = arg.shape[count].upper - else: - upper = arg.shape[count] - else: - upper = dim.upper - - if not dim.lower: - if isinstance(arg.shape[count], RangeIndex): - lower = arg.shape[count].lower - else: - lower = IntLiteral(1) - else: - lower = dim.lower - - return lower, upper + type = RuleType.WARN @staticmethod def range_to_sum(lower, upper): + """ + Method to convert lower and upper bounds of a :any:`RangeIndex` to a + :any:`Sum` expression. + """ + return Sum((IntLiteral(1), upper, Product((IntLiteral(-1), lower)))) - def get_explicit_arg_size(self, arg, dims): + @classmethod + def get_explicit_arg_size(cls, arg, dims): + """ + Method to return the size of a subroutine argument whose bounds are + explicitly declared. + """ + if isinstance(arg, Scalar): size = as_tuple(IntLiteral(1)) else: size = () for dim in dims: if isinstance(dim, RangeIndex): - size += as_tuple(simplify(self.range_to_sum(dim.lower, dim.upper))) + size += as_tuple(simplify(cls.range_to_sum(dim.lower, dim.upper))) else: size += as_tuple(dim) return size - def get_implicit_arg_size(self, arg, dims): + @classmethod + def get_implicit_arg_size(cls, arg, dims): + """ + Method to return the size of a subroutine argument whose bounds are + potentially implicitly declared. + """ + size = () for count, dim in enumerate(dims): if isinstance(dim, RangeIndex): @@ -73,18 +73,26 @@ def get_implicit_arg_size(self, arg, dims): else: lower = dim.lower - size += as_tuple(self.range_to_sum(lower, upper)) + size += as_tuple(cls.range_to_sum(lower, upper)) else: size += as_tuple(dim) return size - def argument_size_mismatch(self, routine, **kwargs): + @classmethod + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): + """ + Method to check for argument size mismatches across subroutine calls. + It requires all :any:`CallStatement` nodes to be enriched, and requires + all subroutine arguments *to not be* of type :any:`DeferredTypeSymbol`. + Therefore relevant modules should be parsed before parsing the current + :any:`Subroutine`. + """ - assign_map = {a.lhs: a.rhs for a in FindNodes(Assignment).visit(routine.body)} + assign_map = {a.lhs: a.rhs for a in FindNodes(Assignment).visit(subroutine.body)} targets = as_tuple(kwargs.get('targets', None)) - calls = FindNodes(CallStatement).visit(routine.body) + calls = FindNodes(CallStatement).visit(subroutine.body) calls = [c for c in calls if c.name.name.lower() in targets] for call in calls: @@ -104,7 +112,7 @@ def argument_size_mismatch(self, routine, **kwargs): if any(None in (dim.lower, dim.upper) for dim in arg_map_f[arg].shape if isinstance(dim, RangeIndex)): continue - dummy_arg_size = self.get_explicit_arg_size(arg_map_f[arg], arg_map_f[arg].shape) + dummy_arg_size = cls.get_explicit_arg_size(arg_map_f[arg], arg_map_f[arg].shape) dummy_arg_size = SubstituteExpressions(arg_map_r).visit(dummy_arg_size) dummy_arg_size = SubstituteExpressions(assign_map).visit(dummy_arg_size) @@ -130,24 +138,24 @@ def argument_size_mismatch(self, routine, **kwargs): if any(None in (dim.lower, dim.upper) for dim in arg.dimensions): continue - arg_size = self.get_explicit_arg_size(arg, arg.dimensions) + arg_size = cls.get_explicit_arg_size(arg, arg.dimensions) alt_arg_size = arg_size else: # compute dim sizes assuming single element if arg.dimensions: - arg_size = self.get_implicit_arg_size(arg, arg.dimensions) + arg_size = cls.get_implicit_arg_size(arg, arg.dimensions) arg_size = as_tuple([IntLiteral(1) if not isinstance(a, Sum) else simplify(a) for a in arg_size]) else: - arg_size = self.get_explicit_arg_size(arg, arg.shape) + arg_size = cls.get_explicit_arg_size(arg, arg.shape) - # compute dim sizes assuming element reference - alt_arg_size = self.get_implicit_arg_size(arg, arg.dimensions) + # compute dim sizes assuming array sequence + alt_arg_size = cls.get_implicit_arg_size(arg, arg.dimensions) alt_arg_size = as_tuple([simplify(Sum((Product((IntLiteral(-1), a)), a.shape[i], IntLiteral(1)))) if not isinstance(a, Sum) else simplify(a) for i, a in enumerate(alt_arg_size)]) - alt_arg_size += self.get_explicit_arg_size(arg, arg.shape[len(arg.dimensions):]) + alt_arg_size += cls.get_explicit_arg_size(arg, arg.shape[len(arg.dimensions):]) stat = False for i in range(len(arg_size)): @@ -163,11 +171,9 @@ def argument_size_mismatch(self, routine, **kwargs): stat = True if not stat: - warn = f'[Loki::IdemDebug] Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' - warn += f'in {call} in {routine}' - warning(warn) - - def transform_subroutine(self, routine, **kwargs): + msg = f'[Loki::IdemDebug] Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' + msg += f'in {call} in {subroutine}' + rule_report.add(msg, call) - # check for argument size mismatch across subroutine calls - self.argument_size_mismatch(routine, **kwargs) +# Create the __all__ property of the module to contain only the rule names +__all__ = tuple(name for name in dir() if name.endswith('Rule') and name != 'GenericRule') \ No newline at end of file diff --git a/transformations/transformations/__init__.py b/transformations/transformations/__init__.py index dc0cfd7d5..1ef23e183 100644 --- a/transformations/transformations/__init__.py +++ b/transformations/transformations/__init__.py @@ -17,7 +17,6 @@ from transformations.utility_routines import * # noqa from transformations.scc_cuf import * # noqa from transformations.pool_allocator import * # noqa -from transformations.idem_debug import * # noqa try: __version__ = version("transformations") From 7b58199830069dfbdf95e1c175ca365ee01884c6 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Mon, 10 Jul 2023 16:51:19 +0100 Subject: [PATCH 04/11] ArgSizeMismatchRule: tested and working on ecLand --- lint_rules/lint_rules/debug_rules.py | 45 +++++++++++++++++----------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index 5b4e96e3c..72eb03cbc 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -7,9 +7,9 @@ import operator as _op from loki import ( - Transformation, FindNodes, CallStatement, Assignment, Scalar, RangeIndex, - simplify, Sum, Product, IntLiteral, DeferredTypeSymbol, as_tuple, SubstituteExpressions, - symbolic_op + FindNodes, CallStatement, Assignment, Scalar, RangeIndex, + simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, + symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten ) from loki.lint import GenericRule, RuleType @@ -90,10 +90,12 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): """ assign_map = {a.lhs: a.rhs for a in FindNodes(Assignment).visit(subroutine.body)} + decl_symbols = flatten([decl.symbols for decl in FindNodes(VariableDeclaration).visit(subroutine.spec)]) + decl_symbols = [sym for sym in decl_symbols if sym.type.initial] + assign_map.update({sym: sym.initial for sym in decl_symbols}) targets = as_tuple(kwargs.get('targets', None)) - calls = FindNodes(CallStatement).visit(subroutine.body) - calls = [c for c in calls if c.name.name.lower() in targets] + calls = [c for c in FindNodes(CallStatement).visit(subroutine.body) if c.name in targets] for call in calls: @@ -102,13 +104,18 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): continue arg_map_f = {carg: rarg for rarg, carg in call.arg_iter()} - arg_map_r = {rarg: carg for rarg, carg in call.arg_iter()} - for arg in call.arguments: + arg_map_r = dict(call.arg_iter()) + + # combine args and kwargs into single iterable + arguments = call.arguments + arguments += as_tuple([arg for kw, arg in call.kwarguments]) + + for arg in arguments: - # we can't proceed if dummy arg has assumed shape component if isinstance(arg_map_f[arg], Scalar): dummy_arg_size = as_tuple(IntLiteral(1)) else: + # we can't proceed if dummy arg has assumed shape component if any(None in (dim.lower, dim.upper) for dim in arg_map_f[arg].shape if isinstance(dim, RangeIndex)): continue @@ -118,11 +125,14 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): dummy_arg_size = SubstituteExpressions(assign_map).visit(dummy_arg_size) dummy_arg_size = Product(dummy_arg_size) + # TODO: skip string literal args + if isinstance(arg, StringLiteral): + continue arg_size = () alt_arg_size = () # check if argument is scalar - if isinstance(arg, Scalar): + if isinstance(arg, (Scalar, LogicLiteral)) or is_constant(arg): arg_size += as_tuple(IntLiteral(1)) alt_arg_size += as_tuple(IntLiteral(1)) else: @@ -151,29 +161,28 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): # compute dim sizes assuming array sequence alt_arg_size = cls.get_implicit_arg_size(arg, arg.dimensions) + ubounds = [dim.upper if isinstance(dim, RangeIndex) else dim for dim in arg.shape] alt_arg_size = as_tuple([simplify(Sum((Product((IntLiteral(-1), a)), - a.shape[i], IntLiteral(1)))) + ubounds[i], IntLiteral(1)))) if not isinstance(a, Sum) else simplify(a) for i, a in enumerate(alt_arg_size)]) alt_arg_size += cls.get_explicit_arg_size(arg, arg.shape[len(arg.dimensions):]) stat = False - for i in range(len(arg_size)): - dims = () - for a in alt_arg_size[:i]: - dims += as_tuple(a) - for a in arg_size[i:]: - dims += as_tuple(a) + for i in range(len(arg_size) + 1): + dims = tuple(alt_arg_size[:i]) + dims += tuple(arg_size[i:]) dims = Product(dims) if symbolic_op(dims, _op.eq, dummy_arg_size): stat = True + break if not stat: - msg = f'[Loki::IdemDebug] Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' + msg = f'Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' msg += f'in {call} in {subroutine}' rule_report.add(msg, call) # Create the __all__ property of the module to contain only the rule names -__all__ = tuple(name for name in dir() if name.endswith('Rule') and name != 'GenericRule') \ No newline at end of file +__all__ = tuple(name for name in dir() if name.endswith('Rule') and name != 'GenericRule') From f48c28c494d425cd9ce8179bd5d99ee989cd211b Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Tue, 11 Jul 2023 11:33:28 +0100 Subject: [PATCH 05/11] Moved lint_rules testing boilerplate to conftest.py --- lint_rules/tests/conftest.py | 29 ++++++++ .../tests/test_ifs_coding_standards_2011.py | 74 +++++++++---------- 2 files changed, 65 insertions(+), 38 deletions(-) create mode 100644 lint_rules/tests/conftest.py diff --git a/lint_rules/tests/conftest.py b/lint_rules/tests/conftest.py new file mode 100644 index 000000000..243d845d9 --- /dev/null +++ b/lint_rules/tests/conftest.py @@ -0,0 +1,29 @@ +# (C) Copyright 2018- ECMWF. +# This software is licensed under the terms of the Apache Licence Version 2.0 +# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. +# In applying this licence, ECMWF does not waive the privileges and immunities +# granted to it by virtue of its status as an intergovernmental organisation +# nor does it submit to any jurisdiction. + +from loki import HAVE_FP, FP +from loki.lint import Reporter, Linter + + +__all__ = ['available_frontends', 'run_linter'] + + +def available_frontends(): + """Choose frontend to use (Linter currently relies exclusively on Fparser)""" + if HAVE_FP: + return [FP,] + return [] + + +def run_linter(sourcefile, rule_list, config=None, handlers=None): + """ + Run the linter for the given source file with the specified list of rules. + """ + reporter = Reporter(handlers) + linter = Linter(reporter, rules=rule_list, config=config) + linter.check(sourcefile) + return linter diff --git a/lint_rules/tests/test_ifs_coding_standards_2011.py b/lint_rules/tests/test_ifs_coding_standards_2011.py index a39511380..264994e51 100644 --- a/lint_rules/tests/test_ifs_coding_standards_2011.py +++ b/lint_rules/tests/test_ifs_coding_standards_2011.py @@ -9,12 +9,13 @@ from pathlib import Path import pytest -from loki import FP, HAVE_FP, Sourcefile -from loki.lint import Reporter, Linter, DefaultHandler +from conftest import run_linter, available_frontends +from loki import Sourcefile +from loki.lint import DefaultHandler -pytestmark = pytest.mark.skipif(not HAVE_FP, - reason='Fparser frontend not available') +pytestmark = pytest.mark.skipif(not available_frontends(), + reason='Suitable frontend not available') @pytest.fixture(scope='module', name='rules') @@ -23,26 +24,11 @@ def fixture_rules(): return rules -@pytest.fixture(scope='module', name='frontend') -def fixture_frontend(): - """Choose frontend to use (Linter currently relies exclusively on Fparser)""" - return FP - - -def run_linter(sourcefile, rule_list, config=None, handlers=None): - """ - Run the linter for the given source file with the specified list of rules. - """ - reporter = Reporter(handlers) - linter = Linter(reporter, rules=rule_list, config=config) - linter.check(sourcefile) - return linter - - -@pytest.mark.parametrize('frontend, nesting_depth, lines', [ - (FP, 3, []), - (FP, 2, [6, 12, 16, 22, 28, 35]), - (FP, 1, [5, 6, 10, 12, 16, 22, 27, 28, 34, 35])]) +@pytest.mark.parametrize('frontend', available_frontends()) +@pytest.mark.parametrize('nesting_depth, lines', [ + (3, []), + (2, [6, 12, 16, 22, 28, 35]), + (1, [5, 6, 10, 12, 16, 22, 27, 28, 34, 35])]) def test_code_body_messages(rules, frontend, nesting_depth, lines): ''' Test the number and content of messages generated by CodeBodyRule @@ -105,6 +91,7 @@ def test_code_body_messages(rules, frontend, nesting_depth, lines): assert f'l. {ref_line}' in msg +@pytest.mark.parametrize('frontend', available_frontends()) def test_module_naming(rules, frontend): '''Test file and modules for checking that naming is correct and matches each other.''' fcode = """ @@ -149,6 +136,7 @@ def test_module_naming(rules, frontend): assert all(keyword in messages[2] for keyword in ('module_naming_mod.f90', 'filename')) +@pytest.mark.parametrize('frontend', available_frontends()) def test_dr_hook_okay(rules, frontend): fcode = """ subroutine routine_okay @@ -194,6 +182,7 @@ def test_dr_hook_okay(rules, frontend): assert len(messages) == 0 +@pytest.mark.parametrize('frontend', available_frontends()) def test_dr_hook_routine(rules, frontend): fcode = """ subroutine routine_not_okay_a @@ -315,6 +304,7 @@ def test_dr_hook_routine(rules, frontend): for letter, i in (('a', 0), ('c', 4), ('c', 5), ('d', 6), ('e', 7))) +@pytest.mark.parametrize('frontend', available_frontends()) def test_dr_hook_module(rules, frontend): fcode = """ module some_mod @@ -385,10 +375,11 @@ def test_dr_hook_module(rules, frontend): assert '(l. 45)' in messages[3] -@pytest.mark.parametrize('frontend, max_num_statements, passes', [ - (FP, 10, True), - (FP, 4, True), - (FP, 3, False)]) +@pytest.mark.parametrize('frontend', available_frontends()) +@pytest.mark.parametrize('max_num_statements, passes', [ + (10, True), + (4, True), + (3, False)]) def test_limit_subroutine_stmts(rules, frontend, max_num_statements, passes): '''Test for different maximum allowed number of executable statements and content of messages generated by LimitSubroutineStatementsRule.''' @@ -421,11 +412,12 @@ def test_limit_subroutine_stmts(rules, frontend, max_num_statements, passes): assert all(all(keyword in msg for keyword in keywords) for msg in messages) -@pytest.mark.parametrize('frontend, max_num_arguments, passes', [ - (FP, 10, True), - (FP, 8, True), - (FP, 7, False), - (FP, 1, False)]) +@pytest.mark.parametrize('frontend', available_frontends()) +@pytest.mark.parametrize('max_num_arguments, passes', [ + (10, True), + (8, True), + (7, False), + (1, False)]) def test_max_dummy_args(rules, frontend, max_num_arguments, passes): '''Test for different maximum allowed number of dummy arguments and content of messages generated by MaxDummyArgsRule.''' @@ -447,6 +439,7 @@ def test_max_dummy_args(rules, frontend, max_num_arguments, passes): assert all(all(keyword in msg for keyword in keywords) for msg in messages) +@pytest.mark.parametrize('frontend', available_frontends()) def test_mpl_cdstring(rules, frontend): fcode = """ subroutine routine_okay @@ -482,6 +475,7 @@ def test_mpl_cdstring(rules, frontend): assert sum('(l. 18)' in msg for msg in messages) == 1 +@pytest.mark.parametrize('frontend', available_frontends()) def test_implicit_none(rules, frontend): fcode = """ subroutine routine_okay @@ -567,6 +561,7 @@ def test_implicit_none(rules, frontend): assert sum('"contained_contained_routine_not_okay"' in msg for msg in messages) == 1 +@pytest.mark.parametrize('frontend', available_frontends()) def test_explicit_kind(rules, frontend): fcode = """ subroutine routine_okay @@ -624,6 +619,7 @@ def test_explicit_kind(rules, frontend): assert all(kw in msg for kw in keys if kw is not None) +@pytest.mark.parametrize('frontend', available_frontends()) def test_banned_statements_default(rules, frontend): '''Test for banned statements with default.''' fcode = """ @@ -649,11 +645,12 @@ def test_banned_statements_default(rules, frontend): assert all(any(keyword in msg for keyword in banned_statements) for msg in messages) -@pytest.mark.parametrize('frontend, banned_statements, passes', [ - (FP, [], True), - (FP, ['GO TO'], False), - (FP, ['GO TO', 'RETURN'], False), - (FP, ['RETURN'], True)]) +@pytest.mark.parametrize('frontend', available_frontends()) +@pytest.mark.parametrize('banned_statements, passes', [ + ([], True), + (['GO TO'], False), + (['GO TO', 'RETURN'], False), + (['RETURN'], True)]) def test_banned_statements_config(rules, frontend, banned_statements, passes): '''Test for banned statements with custom config.''' fcode = """ @@ -678,6 +675,7 @@ def test_banned_statements_config(rules, frontend, banned_statements, passes): assert all(all(keyword in msg for keyword in keywords) for msg in messages) +@pytest.mark.parametrize('frontend', available_frontends()) def test_fortran_90_operators(rules, frontend): '''Test for existence of non Fortran 90 comparison operators.''' fcode = """ From 2e97a13363be737b6ec0a1723134c8d5f9013e22 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Tue, 11 Jul 2023 17:16:11 +0100 Subject: [PATCH 06/11] ArgSizeMismatchRule: added tests --- lint_rules/lint_rules/debug_rules.py | 52 ++++++--- lint_rules/tests/conftest.py | 4 +- lint_rules/tests/test_debug_rules.py | 166 +++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 21 deletions(-) create mode 100644 lint_rules/tests/test_debug_rules.py diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index 72eb03cbc..d7d5ca9b2 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -7,7 +7,7 @@ import operator as _op from loki import ( - FindNodes, CallStatement, Assignment, Scalar, RangeIndex, + FindNodes, CallStatement, Assignment, Scalar, RangeIndex, resolve_associates, simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten ) @@ -29,6 +29,22 @@ def range_to_sum(lower, upper): return Sum((IntLiteral(1), upper, Product((IntLiteral(-1), lower)))) + @staticmethod + def compare_sizes(arg_size, alt_arg_size, dummy_arg_size): + """ + Compare all possible argument size candidates with dummy arg size. + """ + for i in range(len(arg_size) + 1): + dims = tuple(alt_arg_size[:i]) + dims += tuple(arg_size[i:]) + + dims = Product(dims) + + if symbolic_op(dims, _op.eq, dummy_arg_size): + return True + + return False + @classmethod def get_explicit_arg_size(cls, arg, dims): """ @@ -89,6 +105,9 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): :any:`Subroutine`. """ + # first resolve associates + resolve_associates(subroutine) + assign_map = {a.lhs: a.rhs for a in FindNodes(Assignment).visit(subroutine.body)} decl_symbols = flatten([decl.symbols for decl in FindNodes(VariableDeclaration).visit(subroutine.spec)]) decl_symbols = [sym for sym in decl_symbols if sym.type.initial] @@ -103,8 +122,7 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): if not call.routine: continue - arg_map_f = {carg: rarg for rarg, carg in call.arg_iter()} - arg_map_r = dict(call.arg_iter()) + arg_map = {carg: rarg for rarg, carg in call.arg_iter()} # combine args and kwargs into single iterable arguments = call.arguments @@ -112,18 +130,16 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): for arg in arguments: - if isinstance(arg_map_f[arg], Scalar): + if isinstance(arg_map[arg], Scalar): dummy_arg_size = as_tuple(IntLiteral(1)) else: # we can't proceed if dummy arg has assumed shape component if any(None in (dim.lower, dim.upper) - for dim in arg_map_f[arg].shape if isinstance(dim, RangeIndex)): + for dim in arg_map[arg].shape if isinstance(dim, RangeIndex)): continue - dummy_arg_size = cls.get_explicit_arg_size(arg_map_f[arg], arg_map_f[arg].shape) + dummy_arg_size = cls.get_explicit_arg_size(arg_map[arg], arg_map[arg].shape) - dummy_arg_size = SubstituteExpressions(arg_map_r).visit(dummy_arg_size) - dummy_arg_size = SubstituteExpressions(assign_map).visit(dummy_arg_size) - dummy_arg_size = Product(dummy_arg_size) + dummy_arg_size = SubstituteExpressions(dict(call.arg_iter())).visit(dummy_arg_size) # TODO: skip string literal args if isinstance(arg, StringLiteral): @@ -168,19 +184,17 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): for i, a in enumerate(alt_arg_size)]) alt_arg_size += cls.get_explicit_arg_size(arg, arg.shape[len(arg.dimensions):]) - stat = False - for i in range(len(arg_size) + 1): - dims = tuple(alt_arg_size[:i]) - dims += tuple(arg_size[i:]) + # first check using unmodified dimension names + dummy_size = Product(dummy_arg_size) + stat = cls.compare_sizes(arg_size, alt_arg_size, dummy_size) - dims = Product(dims) - - if symbolic_op(dims, _op.eq, dummy_arg_size): - stat = True - break + # if necessary, update dimension names and check + if not stat: + dummy_size = Product(SubstituteExpressions(assign_map).visit(dummy_arg_size)) + stat = cls.compare_sizes(arg_size, alt_arg_size, dummy_size) if not stat: - msg = f'Size mismatch:: arg: {arg}, dummy_arg: {arg_map_f[arg]} ' + msg = f'Size mismatch:: arg: {arg}, dummy_arg: {arg_map[arg]} ' msg += f'in {call} in {subroutine}' rule_report.add(msg, call) diff --git a/lint_rules/tests/conftest.py b/lint_rules/tests/conftest.py index 243d845d9..3a28cebc2 100644 --- a/lint_rules/tests/conftest.py +++ b/lint_rules/tests/conftest.py @@ -19,11 +19,11 @@ def available_frontends(): return [] -def run_linter(sourcefile, rule_list, config=None, handlers=None): +def run_linter(sourcefile, rule_list, config=None, handlers=None, targets=None): """ Run the linter for the given source file with the specified list of rules. """ reporter = Reporter(handlers) linter = Linter(reporter, rules=rule_list, config=config) - linter.check(sourcefile) + linter.check(sourcefile, targets=targets) return linter diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py new file mode 100644 index 000000000..9912e8f49 --- /dev/null +++ b/lint_rules/tests/test_debug_rules.py @@ -0,0 +1,166 @@ +# (C) Copyright 2018- ECMWF. +# This software is licensed under the terms of the Apache Licence Version 2.0 +# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. +# In applying this licence, ECMWF does not waive the privileges and immunities +# granted to it by virtue of its status as an intergovernmental organisation +# nor does it submit to any jurisdiction. + +import importlib +import pytest + +from conftest import run_linter, available_frontends +from loki import Sourcefile +from loki.lint import DefaultHandler + + +pytestmark = pytest.mark.skipif(not available_frontends(), + reason='Suitable frontend not available') + + +@pytest.fixture(scope='module', name='rules') +def fixture_rules(): + rules = importlib.import_module('lint_rules.debug_rules') + return rules + + +@pytest.mark.parametrize('frontend', available_frontends()) +def test_arg_size_array_slices(rules, frontend): + """ + Test for argument size mismatch when arguments are passed as array slices. + """ + + fcode_driver = """ +subroutine driver(klon, klev, nblk, var0, var1, var2, var3, var4, var5, & + var6, var7) +use yomhook, only : lhook, dr_hook, jphook +implicit none + +integer, intent(in) :: klon, klev, nblk +real, intent(in) :: var2(:,:), var4(:,:), var5(:,:), var3(klon, 137), var5(klon, 138) +real, intent(in) :: var6(:,:), var7(:,:) +real, intent(inout) :: var0(klon, nblk), var1(klon, 138, nblk) +real(kind=jphook) :: zhook_handle +integer :: klev, ibl + +if(lhook) call dr_hook('driver', 0, zhook_handle) + +associate(nlev => klev) +nlev = 137 +do ibl = 1, nblk + call kernel(klon, nlev, var0(:,ibl), var1(:,:,ibl), var2(1:klon, 1:nlev), & + var3, var4(1:klon, 1:nlev+1), var5(:, 1:nlev+1), & + var6_d=var6, var7_d=var7(:,1:nlev)) +enddo +end associate + +if(lhook) call dr_hook('driver', 1, zhook_handle) +end subroutine driver + """.strip() + + fcode_kernel = """ +subroutine kernel(klon, klev, var0_d, var1_d, var2_d, var3_d, var4_d, var5_d, var6_d, var7_d) +use yomhook, only : lhook, dr_hook, jphook +implicit none +integer, intent(in) :: klon, klev +real, dimension(klon, klev), intent(inout) :: var0_d, var1_d +real, dimension(klon, klev), intent(in) :: var2_d, var3_d, var4_d +real, dimension(klon, klev+1), intent(in) :: var5_d +real, intent(in) :: var6_d(klon, klev), var7_d(klon, klev) +real(kind=jphook) :: zhook_handle + +if(lhook) call dr_hook('kernel', 0, zhook_handle) +if(lhook) call dr_hook('kernel', 1, zhook_handle) +end subroutine kernel + """.strip() + + driver_source = Sourcefile.from_source(fcode_driver, frontend=frontend) + kernel_source = Sourcefile.from_source(fcode_kernel, frontend=frontend) + + driver = driver_source['driver'] + kernel = kernel_source['kernel'] + driver.enrich_calls([kernel,]) + + messages = [] + handler = DefaultHandler(target=messages.append) + _ = run_linter(driver_source, [rules.ArgSizeMismatchRule], handlers=[handler], targets=['kernel',]) + + for msg in messages: + print(msg) + assert len(messages) == 3 + keyword = 'ArgSizeMismatchRule' + assert all(keyword in msg for msg in messages) + + args = ('var0', 'var1', 'var4') + for msg, ref_arg in zip(messages, args): + assert f'arg: {ref_arg}' in msg + assert f'dummy_arg: {ref_arg}_d' in msg + + +@pytest.mark.parametrize('frontend', available_frontends()) +def test_arg_size_array_sequence(rules, frontend): + """ + Test for argument size mismatch when arguments are passed as array sequences. + """ + + fcode_driver = """ +subroutine driver(klon, klev, nblk, var0, var1, var2, var3) +use yomhook, only : lhook, dr_hook, jphook +implicit none + +integer, intent(in) :: klon, klev, nblk +real, intent(inout) :: var0(klon, nblk), var1(klon, 138, nblk) +real, intent(in) :: var2(klon, 137), var3(klon*137) +real(kind=jphook) :: zhook_handle +real, dimension(klon, 137) :: var4, var5 +real :: var6 +integer :: klev, ibl + +if(lhook) call dr_hook('driver', 0, zhook_handle) + +klev = 137 +do ibl = 1, nblk + call kernel(klon, klev, var0(1,ibl), var1(1,1,ibl), var2(1, 1), var3(1), & + var4(1, 1), var5, var6, 1, .true.) +enddo + +if(lhook) call dr_hook('driver', 1, zhook_handle) +end subroutine driver + """.strip() + + fcode_kernel = """ +subroutine kernel(klon, klev, var0_d, var1_d, var2_d, var3_d, var4_d, var5_d, var6_d, & + int_arg, log_arg) +use yomhook, only : lhook, dr_hook, jphook +implicit none +integer, intent(in) :: klon, klev +real, dimension(klon, klev), intent(inout) :: var0_d, var1_d +real, dimension(klon, klev), intent(in) :: var2_d, var3_d +real, intent(out) :: var4_d, var5_d, var6_d(klon, klev) +integer, intent(out) :: int_arg +logical, intent(out) :: log_arg +real(kind=jphook) :: zhook_handle + +if(lhook) call dr_hook('kernel', 0, zhook_handle) +if(lhook) call dr_hook('kernel', 1, zhook_handle) +end subroutine kernel + """.strip() + + driver_source = Sourcefile.from_source(fcode_driver, frontend=frontend) + kernel_source = Sourcefile.from_source(fcode_kernel, frontend=frontend) + + driver = driver_source['driver'] + kernel = kernel_source['kernel'] + driver.enrich_calls([kernel,]) + + messages = [] + handler = DefaultHandler(target=messages.append) + _ = run_linter(driver_source, [rules.ArgSizeMismatchRule], handlers=[handler], targets=['kernel',]) + + assert len(messages) == 4 + keyword = 'ArgSizeMismatchRule' + assert all(keyword in msg for msg in messages) + + args = ('var0', 'var1', 'var5', 'var6') + for msg, ref_arg in zip(messages, args): + assert f'arg: {ref_arg}' in msg + assert f'dummy_arg: {ref_arg}_d' in msg From 11434984cc4c21eb06094bdb5fe007f7e7ae0041 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 12 Jul 2023 15:19:42 +0100 Subject: [PATCH 07/11] DynamicUboundCheckRule: implemented check, fix and tests --- lint_rules/lint_rules/debug_rules.py | 104 ++++++++++++++++++++++++++- lint_rules/tests/conftest.py | 5 +- lint_rules/tests/test_debug_rules.py | 72 ++++++++++++++++++- 3 files changed, 175 insertions(+), 6 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index d7d5ca9b2..d781e5253 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -8,8 +8,9 @@ import operator as _op from loki import ( FindNodes, CallStatement, Assignment, Scalar, RangeIndex, resolve_associates, - simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, - symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten + simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, Array, + symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten, + FindInlineCalls, Conditional, Transformer, FindExpressions, Comparison ) from loki.lint import GenericRule, RuleType @@ -198,5 +199,104 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): msg += f'in {call} in {subroutine}' rule_report.add(msg, call) +class DynamicUboundCheckRule(GenericRule): + """ + Rule to check for run-time ubound checks for assumed shape dummy arguments + """ + + type = RuleType.WARN + fixable = True + + @staticmethod + def is_assumed_shape(arg): + """ + Method to check if argument is an assumed shape array. + """ + + if all(isinstance(dim, RangeIndex) for dim in arg.shape): + return all(dim.upper is None and dim.lower is None for dim in arg.shape) + return False + + @staticmethod + def get_ubound_checks(subroutine): + """ + Method to return UBOUND checks nested within a :any:`Conditional`. + """ + + cond_map = {cond: FindInlineCalls(unique=False).visit(cond.condition) + for cond in FindNodes(Conditional).visit(subroutine.body)} + return {call: cond for cond, calls in cond_map.items() for call in calls} + + @classmethod + def get_assumed_shape_args(cls, subroutine): + """ + Method to return all assumed-shape dummy arguments in a :any:`Subroutine`. + """ + args = [arg for arg in subroutine.arguments if isinstance(arg, Array)] + return [arg for arg in args if cls.is_assumed_shape(arg)] + + @classmethod + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): + """ + Method to check for run-time ubound checks for assumed shape dummy arguments + """ + + ubound_checks = cls.get_ubound_checks(subroutine) + args = cls.get_assumed_shape_args(subroutine) + + for arg in args: + checks = [c for c in ubound_checks if arg.name in c.arguments] + params = flatten([p for c in checks for p in c.arguments if not p == arg]) + if all(IntLiteral(d+1) in params for d in range(len(arg.shape))): + msg = f'Run-time UBOUND checks for assumed-shape arg: {arg}' + rule_report.add(msg, subroutine) + + @classmethod + def fix_subroutine(cls, subroutine, rule_report, config): + """ + Method to fix run-time ubound checks for assumed shape dummy arguments + """ + + ubound_checks = cls.get_ubound_checks(subroutine) + args = cls.get_assumed_shape_args(subroutine) + + new_vars = () + node_map = {} + + for arg in args: + checks = [c for c in ubound_checks if arg.name in c.arguments] + params = {p: c for c in checks for p in c.arguments if not p == arg} + + # check if ubounds of all dimensions are tested + if all(IntLiteral(d+1) in params for d in range(len(arg.shape))): + new_shape = () + for d in range(len(arg.shape)): + conditional = ubound_checks[params[IntLiteral(d+1)]] + node_map[conditional] = None + + # extract comparison expressions in case they are nested in a logical operation + conditions = [c for c in FindExpressions().visit(conditional.condition) + if isinstance(c, Comparison)] + conditions = [c for c in conditions if c.operator in ('<', '>')] + + cond = [c for c in conditions if arg.name in c and IntLiteral(d+1) in c][0] + + # build ordered tuple for declaration shape + if 'ubound' in FindExpressions().visit(cond.left): + new_shape += as_tuple(cond.right) + else: + new_shape += as_tuple(cond.left) + + vtype = arg.type.clone(shape=new_shape, scope=subroutine) + new_vars += as_tuple(arg.clone(type=vtype, dimensions=new_shape, scope=subroutine)) + + #TODO: add 'VariableDeclaration.symbols' should be of type 'Variable' rather than 'Expression' + # to enable case-insensitive search here + new_var_names = [v.name.lower() for v in new_vars] + subroutine.variables = [var for var in subroutine.variables if not var.name.lower() in new_var_names] + + subroutine.body = Transformer(node_map).visit(subroutine.body) + subroutine.variables += new_vars + # Create the __all__ property of the module to contain only the rule names __all__ = tuple(name for name in dir() if name.endswith('Rule') and name != 'GenericRule') diff --git a/lint_rules/tests/conftest.py b/lint_rules/tests/conftest.py index 3a28cebc2..adbf5b828 100644 --- a/lint_rules/tests/conftest.py +++ b/lint_rules/tests/conftest.py @@ -25,5 +25,8 @@ def run_linter(sourcefile, rule_list, config=None, handlers=None, targets=None): """ reporter = Reporter(handlers) linter = Linter(reporter, rules=rule_list, config=config) - linter.check(sourcefile, targets=targets) + report = linter.check(sourcefile, targets=targets) + if config: + if config.get('fix', None): + linter.fix(sourcefile, report) return linter diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py index 9912e8f49..5740238a6 100644 --- a/lint_rules/tests/test_debug_rules.py +++ b/lint_rules/tests/test_debug_rules.py @@ -5,11 +5,13 @@ # granted to it by virtue of its status as an intergovernmental organisation # nor does it submit to any jurisdiction. +import os import importlib +from pathlib import Path import pytest from conftest import run_linter, available_frontends -from loki import Sourcefile +from loki import Sourcefile, FindInlineCalls from loki.lint import DefaultHandler @@ -84,8 +86,6 @@ def test_arg_size_array_slices(rules, frontend): handler = DefaultHandler(target=messages.append) _ = run_linter(driver_source, [rules.ArgSizeMismatchRule], handlers=[handler], targets=['kernel',]) - for msg in messages: - print(msg) assert len(messages) == 3 keyword = 'ArgSizeMismatchRule' assert all(keyword in msg for msg in messages) @@ -164,3 +164,69 @@ def test_arg_size_array_sequence(rules, frontend): for msg, ref_arg in zip(messages, args): assert f'arg: {ref_arg}' in msg assert f'dummy_arg: {ref_arg}_d' in msg + + +@pytest.mark.parametrize('frontend', available_frontends()) +def test_dynamic_ubound_checks(rules, frontend): + """ + Test the run-time UBOUND checking linter rule + """ + + fcode = """ +subroutine kernel(klon, klev, nblk, var0, var1, var2) +use abort_mod +implicit none +integer, intent(in) :: klon, klev, nblk +real, dimension(:,:,:), intent(inout) :: var0, var1 +real, dimension(:,:,:), intent(inout) :: var2 + +if(ubound(var0, 1) < klon)then + call abort('kernel: first dimension of var0 too short') +endif +if(ubound(VAR0, 2) < klev)then + call abort('kernel: second dimension of var0 too short') +endif +if(nblk > UBoUND(vAr0, 3))then + call abort('kernel: third dimension of var0 too short') +endif + +if(nblk > UBOUND(var1, 3))then + call abort('kernel: third dimension of var1 too short') +endif + +if(ubound(var2, 1) < klon .and. ubound(var2, 2) < klev .and. ubound(var2, 3) < nblk)then + call abort('kernel: dimensions of var2 too short') +endif + +call some_other_kernel(klon, klen, nblk, var0, var1, var2) + +end subroutine kernel + """.strip() + + kernel = Sourcefile.from_source(fcode, frontend=frontend) + kernel.path = Path(__file__).parent / 'dynamic_ubound_test.F90' + + messages = [] + handler = DefaultHandler(target=messages.append) + _ = run_linter(kernel, [rules.DynamicUboundCheckRule], config={'fix': True}, handlers=[handler]) + + # check rule violations + assert len(messages) == 2 + assert all('DynamicUboundCheckRule' in msg for msg in messages) + + assert 'var0' in messages[0] + assert 'var2' in messages[1] + + # check fixed subroutine + routine = kernel['kernel'] + icalls = [call for call in FindInlineCalls(unique=False).visit(routine.body) + if call.function == 'ubound'] + + assert len(icalls) == 1 + + shape = ('klon', 'klev', 'nblk') + + assert all(s.name == d for s, d in zip(routine.variable_map['var0'].shape, shape)) + assert all(s.name == d for s, d in zip(routine.variable_map['var2'].shape, shape)) + + os.remove(kernel.path) From 16f022a57d83b617943fa28e3fc7534b2e6f1fb3 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 12 Jul 2023 15:36:53 +0100 Subject: [PATCH 08/11] Appease linter --- tests/test_lint/test_linter.py | 14 +++++++------- tests/test_lint/test_reporter.py | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_lint/test_linter.py b/tests/test_lint/test_linter.py index 61e1e2c0a..985b2d0f6 100644 --- a/tests/test_lint/test_linter.py +++ b/tests/test_lint/test_linter.py @@ -48,7 +48,7 @@ class TestRule(GenericRule): config = {'key': 'default_value'} @classmethod - def check(cls, ast, rule_report, config): + def check(cls, ast, rule_report, config, **kwargs): assert len(config) == 1 assert 'key' in config assert config['key'] == 'default_value' @@ -58,7 +58,7 @@ class TestRule2(GenericRule): config = {'key': 'default_value'} @classmethod - def check(cls, ast, rule_report, config): + def check(cls, ast, rule_report, config, **kwargs): assert len(config) == 2 assert 'key' in config assert config['key'] == 'non_default_value' @@ -215,7 +215,7 @@ class AssignmentComplainRule(GenericRule): docs = {'id': '13.37'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): # pylint: disable=unused-argument + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): # pylint: disable=unused-argument for node in FindNodes(Assignment).visit(subroutine.ir): rule_report.add(cls.__name__ + '_' + str(node.source.lines[0]), node) @@ -223,7 +223,7 @@ class VariableComplainRule(GenericRule): docs = {'id': '23.42'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): # pylint: disable=unused-argument + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): # pylint: disable=unused-argument for node, variables in FindVariables(with_ir_node=True).visit(subroutine.body): for var in variables: rule_report.add(cls.__name__ + '_' + str(var), node) @@ -278,7 +278,7 @@ class AssignmentComplainRule(GenericRule): docs = {'id': '13.37'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): # pylint: disable=unused-argument + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): # pylint: disable=unused-argument for node in FindNodes(Assignment).visit(subroutine.ir): rule_report.add(cls.__name__ + '_' + str(node.source.lines[0]), node) @@ -286,7 +286,7 @@ class VariableComplainRule(GenericRule): docs = {'id': '23.42'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): # pylint: disable=unused-argument + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): # pylint: disable=unused-argument for node, variables in FindVariables(with_ir_node=True).visit(subroutine.body): for var in variables: rule_report.add(cls.__name__ + '_' + str(var), node) @@ -511,7 +511,7 @@ class TestRule(GenericRule): fixable = True @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): if not subroutine.name.isupper(): rule_report.add(f'Subroutine name "{subroutine.name}" is not upper case', subroutine) diff --git a/tests/test_lint/test_reporter.py b/tests/test_lint/test_reporter.py index a5d5142dd..b3ae33f54 100644 --- a/tests/test_lint/test_reporter.py +++ b/tests/test_lint/test_reporter.py @@ -152,7 +152,7 @@ class RandomFailingRule(GenericRule): config = {'dummy_key': 'dummy value'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): if fail_on and fail_on in subroutine.name: rule_report.add(cls.__name__, subroutine) @@ -191,7 +191,7 @@ class RandomFailingRule(GenericRule): config = {'dummy_key': 'dummy value'} @classmethod - def check_subroutine(cls, subroutine, rule_report, config): + def check_subroutine(cls, subroutine, rule_report, config, **kwargs): if fail_on and fail_on in subroutine.name: rule_report.add(cls.__name__, subroutine) From 20738566711f4b85fd590d15d0c8ba120f070f8d Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 19 Jul 2023 11:37:14 +0100 Subject: [PATCH 09/11] ArgSizeMismatchRule: small fixes after PR review --- lint_rules/lint_rules/debug_rules.py | 10 ++-------- lint_rules/tests/test_debug_rules.py | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index d781e5253..0554b12fd 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -124,12 +124,7 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): continue arg_map = {carg: rarg for rarg, carg in call.arg_iter()} - - # combine args and kwargs into single iterable - arguments = call.arguments - arguments += as_tuple([arg for kw, arg in call.kwarguments]) - - for arg in arguments: + for arg in arg_map: if isinstance(arg_map[arg], Scalar): dummy_arg_size = as_tuple(IntLiteral(1)) @@ -139,8 +134,7 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): for dim in arg_map[arg].shape if isinstance(dim, RangeIndex)): continue dummy_arg_size = cls.get_explicit_arg_size(arg_map[arg], arg_map[arg].shape) - - dummy_arg_size = SubstituteExpressions(dict(call.arg_iter())).visit(dummy_arg_size) + dummy_arg_size = SubstituteExpressions(dict(call.arg_iter())).visit(dummy_arg_size) # TODO: skip string literal args if isinstance(arg, StringLiteral): diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py index 5740238a6..08a8e3036 100644 --- a/lint_rules/tests/test_debug_rules.py +++ b/lint_rules/tests/test_debug_rules.py @@ -16,7 +16,7 @@ pytestmark = pytest.mark.skipif(not available_frontends(), - reason='Suitable frontend not available') + reason='Supported frontend not available') @pytest.fixture(scope='module', name='rules') From 2fe20c5755a4b93e5248a73a9a88dbbc8233f9e7 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 19 Jul 2023 13:42:16 +0100 Subject: [PATCH 10/11] ArgSizeMismatchRule: added debug_rules to loki-lint documentation --- docs/source/loki_lint.rst | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/docs/source/loki_lint.rst b/docs/source/loki_lint.rst index 4d8b1c9fb..2cafcab4f 100644 --- a/docs/source/loki_lint.rst +++ b/docs/source/loki_lint.rst @@ -191,14 +191,29 @@ This default configuration can then be used as a template for creating an individual configuration file. Any options not specified explicitly in the configuration file are chosen to be default values. +Rules-module +------------ + +The rules against which Loki-lint performs checks can be configured as follows: + +.. code-block:: bash + + loki-lint.py --rules-module check [options/arguments] + +If a rules-module is not specified, then the default :mod:`lint_rules.ifs_coding_standards_2011` +is used. Implementing own rules ====================== -All rules are implemented in :mod:`lint_rules`. Currently, this includes -only one module (:mod:`lint_rules.ifs_coding_standards_2011`) that -contains (a small subset of) the rules defined in the IFS coding standards -document. To be able to write own rules a rudimentary understanding of +All rules are implemented in :mod:`lint_rules`. Currently, this includes: + +#. :mod:`lint_rules.ifs_coding_standards_2011` - A (small) subset of the rules defined in the IFS coding standards document. +#. :mod:`lint_rules.debug_rules` - A set of rules to identify common mistakes/anti-patterns: + * :any:`ArgSizeMismatchRule` - Check for argument/dummy-argument size consistency + * :any:`DynamicUboundCheckRule` - Check if run-time bounds checking is used rather than compile-time bounds checking. + +To be able to write own rules a rudimentary understanding of :doc:`internal_representation` is helpful. Each rule is represented by a subclass of :any:`GenericRule` with the From cb73473971c874afa1f812572895c540a514caae Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Wed, 19 Jul 2023 16:27:22 +0100 Subject: [PATCH 11/11] ArgSizeMismatchRule: targets now retrieved in GenericRule.check --- loki/bulk/scheduler.py | 3 +-- loki/lint/rules.py | 8 +++++++- tests/test_lint/test_linter.py | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/loki/bulk/scheduler.py b/loki/bulk/scheduler.py index eba859888..52819df62 100644 --- a/loki/bulk/scheduler.py +++ b/loki/bulk/scheduler.py @@ -629,8 +629,7 @@ def process(self, transformation, reverse=False, item_filter=SubroutineItem, use if use_file_graph: for node in traversal: items = graph.nodes[node]['items'] - transformation.apply(items[0].source, item=items[0], items=items, - targets=items[0].targets) + transformation.apply(items[0].source, item=items[0], items=items) else: for item in traversal: if item_filter and not isinstance(item, item_filter): diff --git a/loki/lint/rules.py b/loki/lint/rules.py index b76e0c0c0..6ed1b5f59 100644 --- a/loki/lint/rules.py +++ b/loki/lint/rules.py @@ -132,6 +132,7 @@ def check(cls, ast, rule_report, config, **kwargs): The rule configuration, filled with externally provided configuration values or the rule's default configuration. """ + # Perform checks on source file level if isinstance(ast, Sourcefile): cls.check_file(ast, rule_report, config) @@ -161,7 +162,12 @@ def check(cls, ast, rule_report, config, **kwargs): if is_rule_disabled(ast.ir, cls.identifiers()): return - cls.check_subroutine(ast, rule_report, config, **kwargs) + if not (targets := kwargs.pop('targets', None)): + items = kwargs.get('items', ()) + item = [item for item in items if item.local_name.lower() == ast.name.lower()] + if len(item) > 0: + targets = item[0].targets + cls.check_subroutine(ast, rule_report, config, targets=targets, **kwargs) # Recurse for any procedures contained in a subroutine if hasattr(ast, 'members') and ast.members is not None: diff --git a/tests/test_lint/test_linter.py b/tests/test_lint/test_linter.py index 985b2d0f6..ab53ece30 100644 --- a/tests/test_lint/test_linter.py +++ b/tests/test_lint/test_linter.py @@ -156,7 +156,7 @@ class AlwaysComplainRule(GenericRule): docs = {'id': '13.37'} @classmethod - def check_file(cls, sourcefile, rule_report, config): # pylint: disable=unused-argument + def check_file(cls, sourcefile, rule_report, config, **kwargs): # pylint: disable=unused-argument rule_report.add(cls.__name__, sourcefile) check_module = check_file