From c990fdf0ee5b23a5a157b5b376166db743b53d30 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Fri, 28 Jul 2023 16:41:59 +0200 Subject: [PATCH 1/5] Fortran90OperatorsRule: _op_map now no longer a class method --- .../lint_rules/ifs_coding_standards_2011.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lint_rules/lint_rules/ifs_coding_standards_2011.py b/lint_rules/lint_rules/ifs_coding_standards_2011.py index 9638fd53a..aa3b3506c 100644 --- a/lint_rules/lint_rules/ifs_coding_standards_2011.py +++ b/lint_rules/lint_rules/ifs_coding_standards_2011.py @@ -472,15 +472,6 @@ class Fortran90OperatorsRule(GenericRule): # Coding standards 4.15 '<': re.compile(r'(?P\.lt\.)|(?P<(?!=))', re.I), } - _op_map = { - '==': '.eq.', - '/=': '.ne.', - '>=': '.ge.', - '<=': '.le.', - '>': '.gt.', - '<': '.lt.' - } - @classmethod def check_subroutine(cls, subroutine, rule_report, config, **kwargs): '''Check for the use of Fortran 90 comparison operators.''' @@ -513,8 +504,16 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): op_str = op if op != '!=' else '/=' line = [line for line in lines if op_str in strip_inline_comments(line.string)] if not line: + _op_map = { + '==': '.eq.', + '/=': '.ne.', + '>=': '.ge.', + '<=': '.le.', + '>': '.gt.', + '<': '.lt.' + } line = [line for line in lines - if op_str in strip_inline_comments(line.string.replace(cls._op_map[op_str], op_str))] + if op_str in strip_inline_comments(line.string.replace(_op_map[op_str], op_str))] source_string = strip_inline_comments(line[0].string) matches = cls._op_patterns[op].findall(source_string) From 5e1676394bc161d81e7048a13c81b3f5d46eacc4 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Fri, 28 Jul 2023 09:32:17 +0200 Subject: [PATCH 2/5] DynamicUboundCheckRule: node_map returned to trigger source invalidation --- lint_rules/lint_rules/debug_rules.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index 0554b12fd..5061da53c 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -10,7 +10,7 @@ FindNodes, CallStatement, Assignment, Scalar, RangeIndex, resolve_associates, simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, Array, symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten, - FindInlineCalls, Conditional, Transformer, FindExpressions, Comparison + FindInlineCalls, Conditional, FindExpressions, Comparison ) from loki.lint import GenericRule, RuleType @@ -288,9 +288,9 @@ def fix_subroutine(cls, subroutine, rule_report, config): # 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 + return node_map + # 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') From 3473ccbcb6d5550426d90b9dd1f3a9401f4905fa Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Mon, 31 Jul 2023 13:17:45 +0200 Subject: [PATCH 3/5] DynamicUboundCheckRule: fixer method now preserves subroutine arguments --- lint_rules/lint_rules/debug_rules.py | 19 +++++++++++++++---- lint_rules/tests/test_debug_rules.py | 10 +++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index 5061da53c..e30c3c2c2 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -10,7 +10,7 @@ FindNodes, CallStatement, Assignment, Scalar, RangeIndex, resolve_associates, simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, Array, symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten, - FindInlineCalls, Conditional, FindExpressions, Comparison + FindInlineCalls, Conditional, FindExpressions, Comparison, single_variable_declaration ) from loki.lint import GenericRule, RuleType @@ -284,11 +284,22 @@ def fix_subroutine(cls, subroutine, rule_report, config): 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' + # simplify variable declarations + single_variable_declaration(subroutine) + + #TODO: '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.variables += new_vars + + routine = subroutine.clone() + routine.variables = [var for var in routine.variables if not var.name.lower() in new_var_names] + routine.variables += new_vars + + old_decls = as_tuple([decl for decl in FindNodes(VariableDeclaration).visit(subroutine.spec) + if decl.symbols[0].name.lower() in new_var_names]) + new_decls = as_tuple([decl for decl in FindNodes(VariableDeclaration).visit(routine.spec) + if decl.symbols[0].name.lower() in new_var_names]) + node_map.update({old_decls: new_decls}) return node_map diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py index 08a8e3036..a5697c541 100644 --- a/lint_rules/tests/test_debug_rules.py +++ b/lint_rules/tests/test_debug_rules.py @@ -11,7 +11,7 @@ import pytest from conftest import run_linter, available_frontends -from loki import Sourcefile, FindInlineCalls +from loki import Sourcefile, FindInlineCalls, FindNodes, VariableDeclaration from loki.lint import DefaultHandler @@ -229,4 +229,12 @@ def test_dynamic_ubound_checks(rules, frontend): 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)) + arg_names = ['klon', 'klev', 'nblk', 'var0', 'var1', 'var2'] + assert [arg.name.lower() for arg in routine.arguments] == arg_names + + # check that variable declarations have not been duplicated + symbols = [s.name.lower() for decl in FindNodes(VariableDeclaration).visit(routine.spec) for s in decl.symbols] + assert len(symbols) == 6 + assert set(symbols) == {'klon', 'klev', 'nblk', 'var0', 'var1', 'var2'} + os.remove(kernel.path) From 0711e9d647205bb3c4e2c94d84e8b92416e77bbc Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Thu, 3 Aug 2023 16:47:45 +0200 Subject: [PATCH 4/5] ArgSizeMismatchRule: can now check for a configurable number of indirections of arg size names --- lint_rules/lint_rules/debug_rules.py | 25 ++++++++++++++++++++++--- lint_rules/tests/test_debug_rules.py | 15 ++++++++++----- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index e30c3c2c2..5e4b2ecd7 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -21,6 +21,10 @@ class ArgSizeMismatchRule(GenericRule): type = RuleType.WARN + config = { + 'max_indirections': 2, + } + @staticmethod def range_to_sum(lower, upper): """ @@ -106,6 +110,8 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): :any:`Subroutine`. """ + max_indirections = config['max_indirections'] + # first resolve associates resolve_associates(subroutine) @@ -183,9 +189,22 @@ def check_subroutine(cls, subroutine, rule_report, config, **kwargs): dummy_size = Product(dummy_arg_size) stat = cls.compare_sizes(arg_size, alt_arg_size, dummy_size) - # if necessary, update dimension names and check - if not stat: - dummy_size = Product(SubstituteExpressions(assign_map).visit(dummy_arg_size)) + # we check for a configurable number of indirections for the dummy and arg dimension names + for _ in range(max_indirections): + if stat: + break + + # if necessary, update dummy arg dimension names and check + dummy_arg_size = SubstituteExpressions(assign_map).visit(dummy_arg_size) + dummy_size = Product(dummy_arg_size) + stat = cls.compare_sizes(arg_size, alt_arg_size, dummy_size) + + if stat: + break + + # if necessary, update arg dimension names and check + arg_size = SubstituteExpressions(assign_map).visit(arg_size) + alt_arg_size = SubstituteExpressions(assign_map).visit(alt_arg_size) stat = cls.compare_sizes(arg_size, alt_arg_size, dummy_size) if not stat: diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py index a5697c541..12117c413 100644 --- a/lint_rules/tests/test_debug_rules.py +++ b/lint_rules/tests/test_debug_rules.py @@ -42,14 +42,16 @@ def test_arg_size_array_slices(rules, frontend): real, intent(in) :: var6(:,:), var7(:,:) real, intent(inout) :: var0(klon, nblk), var1(klon, 138, nblk) real(kind=jphook) :: zhook_handle -integer :: klev, ibl +integer :: klev, ibl, iproma, iend 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), & + iproma = klon + iend = iproma + call kernel(klon, nlev, var0(:,ibl), var1(:,:,ibl), var2(1:iend, 1:nlev), & var3, var4(1:klon, 1:nlev+1), var5(:, 1:nlev+1), & var6_d=var6, var7_d=var7(:,1:nlev)) enddo @@ -84,7 +86,8 @@ def test_arg_size_array_slices(rules, frontend): messages = [] handler = DefaultHandler(target=messages.append) - _ = run_linter(driver_source, [rules.ArgSizeMismatchRule], handlers=[handler], targets=['kernel',]) + _ = run_linter(driver_source, [rules.ArgSizeMismatchRule], config={'ArgSizeMismatchRule': {'max_indirections': 3}}, + handlers=[handler], targets=['kernel',]) assert len(messages) == 3 keyword = 'ArgSizeMismatchRule' @@ -113,13 +116,15 @@ def test_arg_size_array_sequence(rules, frontend): real(kind=jphook) :: zhook_handle real, dimension(klon, 137) :: var4, var5 real :: var6 -integer :: klev, ibl +integer :: klev, ibl, iproma, iend 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), & + iproma = klon + iend = iproma + call kernel(klon, klev, var0(1,ibl), var1(1,1,ibl), var2(1:iend, 1), var3(1), & var4(1, 1), var5, var6, 1, .true.) enddo From e5d2c6df065970607b262da59248dc2e3c8cd742 Mon Sep 17 00:00:00 2001 From: Ahmad Nawab Date: Tue, 29 Aug 2023 15:06:53 +0200 Subject: [PATCH 5/5] DynamicUboundCheckRule: fixer method now tries to preserve declarations wherever possible --- lint_rules/lint_rules/debug_rules.py | 31 ++++++++++------------------ lint_rules/tests/test_debug_rules.py | 29 +++++++++++++++++++------- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/lint_rules/lint_rules/debug_rules.py b/lint_rules/lint_rules/debug_rules.py index 5e4b2ecd7..5f610a1dd 100644 --- a/lint_rules/lint_rules/debug_rules.py +++ b/lint_rules/lint_rules/debug_rules.py @@ -10,7 +10,7 @@ FindNodes, CallStatement, Assignment, Scalar, RangeIndex, resolve_associates, simplify, Sum, Product, IntLiteral, as_tuple, SubstituteExpressions, Array, symbolic_op, StringLiteral, is_constant, LogicLiteral, VariableDeclaration, flatten, - FindInlineCalls, Conditional, FindExpressions, Comparison, single_variable_declaration + FindInlineCalls, Conditional, FindExpressions, Comparison ) from loki.lint import GenericRule, RuleType @@ -273,8 +273,8 @@ def fix_subroutine(cls, subroutine, rule_report, config): ubound_checks = cls.get_ubound_checks(subroutine) args = cls.get_assumed_shape_args(subroutine) - new_vars = () node_map = {} + var_map = {} for arg in args: checks = [c for c in ubound_checks if arg.name in c.arguments] @@ -300,25 +300,16 @@ def fix_subroutine(cls, subroutine, rule_report, config): 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)) + vtype = arg.type.clone(shape=new_shape) + var_map.update({arg: arg.clone(type=vtype, dimensions=new_shape)}) - # simplify variable declarations - single_variable_declaration(subroutine) - - #TODO: '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] - - routine = subroutine.clone() - routine.variables = [var for var in routine.variables if not var.name.lower() in new_var_names] - routine.variables += new_vars - - old_decls = as_tuple([decl for decl in FindNodes(VariableDeclaration).visit(subroutine.spec) - if decl.symbols[0].name.lower() in new_var_names]) - new_decls = as_tuple([decl for decl in FindNodes(VariableDeclaration).visit(routine.spec) - if decl.symbols[0].name.lower() in new_var_names]) - node_map.update({old_decls: new_decls}) + # update variable declarations + subroutine.spec = SubstituteExpressions(var_map).visit(subroutine.spec) + for decl in FindNodes(VariableDeclaration).visit(subroutine.spec): + if decl.dimensions: + if not all(sym.shape == decl.dimensions for sym in decl.symbols): + new_decls = as_tuple(VariableDeclaration(as_tuple(sym)) for sym in decl.symbols) + node_map.update({decl: new_decls}) return node_map diff --git a/lint_rules/tests/test_debug_rules.py b/lint_rules/tests/test_debug_rules.py index 12117c413..56e84928c 100644 --- a/lint_rules/tests/test_debug_rules.py +++ b/lint_rules/tests/test_debug_rules.py @@ -178,12 +178,13 @@ def test_dynamic_ubound_checks(rules, frontend): """ fcode = """ -subroutine kernel(klon, klev, nblk, var0, var1, var2) +subroutine kernel(klon, klev, nblk, var0, var1, var2, var3, var4) use abort_mod implicit none integer, intent(in) :: klon, klev, nblk real, dimension(:,:,:), intent(inout) :: var0, var1 real, dimension(:,:,:), intent(inout) :: var2 +real, intent(inout) :: var3(:,:), var4(:,:,:) if(ubound(var0, 1) < klon)then call abort('kernel: first dimension of var0 too short') @@ -203,7 +204,11 @@ def test_dynamic_ubound_checks(rules, frontend): call abort('kernel: dimensions of var2 too short') endif -call some_other_kernel(klon, klen, nblk, var0, var1, var2) +if(ubound(var4, 1) < klon .and. ubound(var4, 2) < klev .and. ubound(var4, 3) < nblk)then + call abort('kernel: dimensions of var4 too short') +endif + +call some_other_kernel(klon, klen, nblk, var0, var1, var2, var3, var4) end subroutine kernel """.strip() @@ -216,11 +221,12 @@ def test_dynamic_ubound_checks(rules, frontend): _ = run_linter(kernel, [rules.DynamicUboundCheckRule], config={'fix': True}, handlers=[handler]) # check rule violations - assert len(messages) == 2 + assert len(messages) == 3 assert all('DynamicUboundCheckRule' in msg for msg in messages) assert 'var0' in messages[0] assert 'var2' in messages[1] + assert 'var4' in messages[2] # check fixed subroutine routine = kernel['kernel'] @@ -233,13 +239,22 @@ def test_dynamic_ubound_checks(rules, frontend): 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)) + assert all(s.name == d for s, d in zip(routine.variable_map['var4'].shape, shape)) - arg_names = ['klon', 'klev', 'nblk', 'var0', 'var1', 'var2'] + arg_names = ['klon', 'klev', 'nblk', 'var0', 'var1', 'var2', 'var3', 'var4'] assert [arg.name.lower() for arg in routine.arguments] == arg_names # check that variable declarations have not been duplicated - symbols = [s.name.lower() for decl in FindNodes(VariableDeclaration).visit(routine.spec) for s in decl.symbols] - assert len(symbols) == 6 - assert set(symbols) == {'klon', 'klev', 'nblk', 'var0', 'var1', 'var2'} + declarations = FindNodes(VariableDeclaration).visit(routine.spec) + symbols = [s.name.lower() for decl in declarations for s in decl.symbols] + assert len(symbols) == 8 + assert set(symbols) == {'klon', 'klev', 'nblk', 'var0', 'var1', 'var2', 'var3', 'var4'} + + # check number of declarations and symbols per declarations + assert len(declarations) == 5 + assert len(declarations[0].symbols) == 3 + for decl in declarations[1:4]: + assert len(decl.symbols) == 1 + assert len(declarations[4].symbols) == 2 os.remove(kernel.path)