Array shadowing bugfix for inliner

[skarppinen]

Hello.

This PR fixes a bug in inline_subroutine_calls.
Consider the following example (here demonstrating with inline_member_procedures, which eventually calls the above):

from loki import Sourcefile
from loki.transform import (
    inline_member_procedures
)
fcode = """
    subroutine outer(klon)
        integer :: jg, jlon
        integer :: arr(3, 3)

        jg = 70000
        call inner2()

        contains

        subroutine inner2()
            integer :: jlon, jg 
            integer :: arr(3, 3)
            do jg=1,3
                do jlon=1,3
                   arr(jlon, jg) = 11 
                end do
            end do
        end subroutine inner2

    end subroutine outer
"""
src = Sourcefile.from_source(fcode)
routine = src.routines[0]
inline_member_procedures(routine)
print(routine.to_fortran())

On the main branch, this prints:

SUBROUTINE outer (klon)
  INTEGER :: jg, jlon
  INTEGER :: arr(3, 3)
  INTEGER :: inner2_jlon, inner2_jg
  INTEGER :: inner2_arr(3, 3)

  jg = 70000
  ! [Loki] inlined child subroutine: inner2
  ! =========================================
  DO inner2_jg=1,3
    DO inner2_jlon=1,3
      inner2_arr(jlon, jg) = 11 ! Wrong 'jlon' and 'jg'
    END DO
  END DO
  ! =========================================

  CONTAINS

END SUBROUTINE outer

Inside the double loop, note the wrong jlon and jg.
This occurs because the inliner renames shadowing variables only by "name", i.e arr(jlon, jg) is transformed only to inner_arr(jlon, jg) even though jlon and jg should also be renamed as they are shadowed as well.
This occurs here (var_map only changes name):

loki/loki/transform/transform_inline.py

Lines 337 to 342 in 556db2e

	var_map = {}
	duplicate_names = {dl.name.lower() for dl in duplicates}
	for v in FindVariables(unique=False).visit(callee.body):
	if v.name.lower() in duplicate_names:
	var_map[v] = v.clone(name=f'{callee.name}_{v.name}')
	callee.body = SubstituteExpressions(var_map).visit(callee.body)

This PR fixes the above issue by first building a transformation map for renaming nonarray variables, and then a transformation map for renaming array variables. In the construction of the latter, the dimensions attribute is transformed using the transformation map for nonarray variables. There may be a more elegant way to achieve this.

There is also a new test to check that this remains fixed.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (556db2e) 92.26% compared to head (1796216) 92.26%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #202   +/-   ##
=======================================
  Coverage   92.26%   92.26%           
=======================================
  Files          96       96           
  Lines       17092    17092           
=======================================
  Hits        15770    15770           
  Misses       1322     1322           

Flag	Coverage Δ
lint_rules	96.22% <ø> (ø)
loki	92.24% <100.00%> (ø)
transformations	91.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[reuterbal]

Thank you very much for finding this and supplying test and fix. However, I think all that's missing here is a call to recursive_expression_map_update before applying the SubstituteExpressions. Would you mind adding just that at the place you have flagged, and seeing whether your test still passes?

[skarppinen]

Thanks @reuterbal, that's a lot simpler and works!

Note that there was an additional local import for recursive_expression_map_update in another function. I couldn't think of why it was introduced as a local import rather than global. When I removed it, the tests seemed to still pass, and everything seemed to be fine, so I removed the local import and just added a global import for the function to avoid duplicate imports.

[reuterbal]

Fantastic, many thanks!

I don't remember what the history of the local import was, but the pylint annotations next to it suggest this may have been to break a cyclic import in the past. However, this does not seem to be a problem any more. Thanks for changing this!

[mlange05]

Many thanks for catching this @skarppinen and apologies, I must have dropped this in the recent refactoring. GTG from me.