Repository: stfc/PSyclone

Issue:
Unprotected symbol-table merge in InlineTrans
After performing inlining, we finally merge the  symbol table of the local scope with that of the parent routine. This merge is not protected by a try...except so if it fails, PSyclone crashes:

https://github.com/stfc/PSyclone/blob/2307ac7584843ee67038384da25af67ef4582f6a/src/psyclone/psyir/transformations/inline_trans.py#L228-L232

Reproducer: psyclone -s examples/nemo/scripts/acc_kernels_trans.py ./icethd_sal.f90

Hints from issue comments:
@arporter: We can workaround this by adding "ice_par" and "ice1d" to `RESOLVE_IMPORTS` in `acc_kernels_trans`. However, the bug remains. We could catch it and suggest this solution to the user...

Solution patch:
diff --git a/src/psyclone/psyir/frontend/fparser2.py b/src/psyclone/psyir/frontend/fparser2.py
index 633860cab..c91b4b083 100644
--- a/src/psyclone/psyir/frontend/fparser2.py
+++ b/src/psyclone/psyir/frontend/fparser2.py
@@ -228,7 +228,7 @@ def _first_type_match(nodelist, typekind):
 
 
 def _find_or_create_unresolved_symbol(location, name, scope_limit=None,
-                                      **kargs):
+                                      **kargs) -> Symbol:
     '''Returns the symbol with the given 'name' from a symbol table
     associated with the 'location' node or one of its ancestors. If a
     symbol is found then the type of the existing symbol is compared
@@ -257,9 +257,11 @@ def _find_or_create_unresolved_symbol(location, name, scope_limit=None,
         searched.
     :type scope_limit: :py:class:`psyclone.psyir.nodes.Node` or
         `NoneType`
+    :param kargs: arguments to pass on when either specialising an
+        existing symbol or creating a new one.
+    :type kargs: unwrapped dict
 
     :returns: the matching symbol.
-    :rtype: :py:class:`psyclone.psyir.symbols.Symbol`
 
     :raises TypeError: if the supplied scope_limit is not a Node.
     :raises ValueError: if the supplied scope_limit node is not an
@@ -283,8 +285,7 @@ def _find_or_create_unresolved_symbol(location, name, scope_limit=None,
         mynode = location.parent
         while mynode is not None:
             if mynode is scope_limit:
-                # The scope_limit node is an ancestor of the
-                # supplied node.
+                # The scope_limit node is an ancestor of the supplied node.
                 break
             mynode = mynode.parent
         else:
@@ -295,19 +296,49 @@ def _find_or_create_unresolved_symbol(location, name, scope_limit=None,
                 f"_find_or_create_unresolved_symbol() is not an ancestor of "
                 f"this node '{location}'.")
 
-    try:
-        sym = location.scope.symbol_table.lookup(name, scope_limit=scope_limit)
-        if "symbol_type" in kargs:
-            expected_type = kargs.pop("symbol_type")
-            if not isinstance(sym, expected_type):
-                # The caller specified a sub-class so we need to
-                # specialise the existing symbol.
-                sym.specialise(expected_type, **kargs)
-        return sym
-    except KeyError:
-        pass
-
-    # find the closest ancestor symbol table attached to a Routine or
+    # In Fortran, no import can clash with the name of a parent Routine.
+    # Therefore, if the name we've been given corresponds to the name of the
+    # enclosing Routine (or its RESULT if it is a function) then it *must*
+    # refer to that and cannot be brought in by an import.
+    parent_scope = location.ancestor(Routine)
+    if parent_scope:
+        if (parent_scope.return_symbol and
+                parent_scope.return_symbol.name.lower() == name.lower()):
+            # The PSyIR canonicalises functions such that they always have
+            # a RESULT clause. As such, according to the Fortan standard, any
+            # reference to the name specified in the RESULT clause is to the
+            # DataSymbol.
+            return parent_scope.return_symbol
+        if parent_scope.name.lower() == name.lower():
+            return parent_scope.symbol
+
+    table = location.scope.symbol_table
+    while table:
+        # By default, `lookup` looks in all ancestor scopes. However, we need
+        # to check for wildcard imports as we work our way up.
+        sym = table.lookup(name, scope_limit=table.node,
+                           otherwise=None)
+        if sym:
+            if "symbol_type" in kargs:
+                expected_type = kargs.pop("symbol_type")
+                if not isinstance(sym, expected_type):
+                    # The caller specified a sub-class so we need to
+                    # specialise the existing symbol.
+                    sym.specialise(expected_type, **kargs)
+            return sym
+
+        if table.wildcard_imports(scope_limit=table.node):
+            # There's a wildcard import into this scope so we stop
+            # searching and create an unresolved symbol (below). This is
+            # permitted to shadow a declaration in an outer scope because
+            # it may be a different entity (coming from the import).
+            # TODO #2915 - it may be that we've already resolved all symbols
+            # from this import but currently we have no way of recording that.
+            kargs["shadowing"] = True
+            break
+        table = table.parent_symbol_table(scope_limit)
+
+    # Find the closest ancestor symbol table attached to a Routine or
     # Container node. We don't want to add to a Schedule node as in
     # some situations PSyclone assumes symbols are declared within
     # Routine or Container symbol tables due to its Fortran provenance
@@ -5158,6 +5189,7 @@ class Fparser2Reader():
         # fparser2 is fixed.
         if str(node.items[0]).lower() == "null" and node.items[1] is None:
             return self._intrinsic_handler(node, parent)
+
         self.process_nodes(parent=call, nodes=[node.items[0]])
         routine = call.children[0]
         # If it's a plain reference, promote the symbol to a RoutineSymbol
@@ -5410,7 +5442,7 @@ class Fparser2Reader():
 
             if isinstance(node, Fortran2003.Function_Subprogram):
                 # Check whether this function-stmt has a suffix containing
-                # 'RETURNS'
+                # 'RESULT'
                 suffix = stmt.children[3]
                 if suffix:
                     # Although the suffix can, in principle, contain a proc-
diff --git a/src/psyclone/psyir/symbols/symbol_table.py b/src/psyclone/psyir/symbols/symbol_table.py
index 9c45b5f3b..a047c1631 100644
--- a/src/psyclone/psyir/symbols/symbol_table.py
+++ b/src/psyclone/psyir/symbols/symbol_table.py
@@ -46,6 +46,7 @@ from collections import OrderedDict
 from collections.abc import Iterable
 import inspect
 import copy
+from typing import Set
 
 from psyclone.configuration import Config
 from psyclone.errors import InternalError
@@ -573,7 +574,6 @@ class SymbolTable():
         if key in self._symbols:
             raise KeyError(f"Symbol table already contains a symbol with "
                            f"name '{new_symbol.name}'.")
-
         if tag:
             if tag in self.get_tags():
                 raise KeyError(
@@ -855,7 +855,7 @@ class SymbolTable():
         except SymbolError as err:
             raise SymbolError(
                 f"Cannot merge {other_table.view()} with {self.view()} due to "
-                f"unresolvable name clashes.") from err
+                f"unresolvable name clashes: {err.value}") from err
 
         # Deal with any Container symbols first.
         self._add_container_symbols_from_table(other_table)
@@ -1864,14 +1864,20 @@ class SymbolTable():
         # Re-insert modified symbol
         self.add(symbol)
 
-    def wildcard_imports(self):
+    def wildcard_imports(self, scope_limit=None) -> Set[str]:
         '''
         Searches this symbol table and then up through any parent symbol
         tables for a ContainerSymbol that has a wildcard import.
 
+        :param scope_limit: optional Node which limits the search to the
+            symbol tables of the nodes within the given scope.
+            If it is None (the default), the whole scope (all symbol tables
+            in ancestor nodes) is searched, otherwise ancestors of the
+            scope_limit node are not searched.
+        :type scope_limit: Optional[:py:class:`psyclone.psyir.nodes.Node`]
+
         :returns: the name(s) of containers which have wildcard imports
             into the current scope.
-        :rtype: set[str]
 
         '''
         wildcards = set()
@@ -1880,7 +1886,8 @@ class SymbolTable():
             for sym in current_table.containersymbols:
                 if sym.wildcard_import:
                     wildcards.add(sym.name)
-            current_table = current_table.parent_symbol_table()
+            current_table = current_table.parent_symbol_table(
+                scope_limit=scope_limit)
         return wildcards
 
     def view(self):


Test patch:
diff --git a/src/psyclone/tests/psyir/frontend/fparser2_call_handler_test.py b/src/psyclone/tests/psyir/frontend/fparser2_call_handler_test.py
index 03a4adebc..c7bcfcac7 100644
--- a/src/psyclone/tests/psyir/frontend/fparser2_call_handler_test.py
+++ b/src/psyclone/tests/psyir/frontend/fparser2_call_handler_test.py
@@ -199,3 +199,36 @@ def test_labelled_call():
     processor = Fparser2Reader()
     processor.process_nodes(fake_parent, [ast])
     assert isinstance(fake_parent[0], CodeBlock)
+
+
+def test_unresolved_shadowed_routine(fortran_reader):
+    '''
+    For the situation where we can't be certain of the source of the target
+    of a call, check that an unresolved RoutineSymbol is created.
+    '''
+    code = '''\
+    module this_mod
+      implicit none
+    contains
+      subroutine do_it()
+        use my_mod
+        integer :: old_my_sub
+        real, dimension(10) :: a
+        ! This call could be to the routine defined in the same module *or*
+        ! to one of the same name brought in by the wildcard import from
+        ! `my_mod`
+        call my_sub(a)
+      end subroutine do_it
+      subroutine my_sub()
+        real, dimension(10) :: b
+        b = 1.0
+      end subroutine my_sub
+    end module this_mod
+    '''
+    psyir = fortran_reader.psyir_from_source(code)
+    do_it = psyir.children[0].find_routine_psyir("do_it")
+    calls = psyir.walk(Call)
+    assert calls[0].routine.symbol.is_unresolved
+    assert isinstance(calls[0].routine.symbol, RoutineSymbol)
+    # Symbol should be in the table of the Routine containing the call.
+    assert calls[0].routine.symbol.find_symbol_table(calls[0]).node is do_it
diff --git a/src/psyclone/tests/psyir/symbols/symbol_table_test.py b/src/psyclone/tests/psyir/symbols/symbol_table_test.py
index f220e7b8f..dc11a6a64 100644
--- a/src/psyclone/tests/psyir/symbols/symbol_table_test.py
+++ b/src/psyclone/tests/psyir/symbols/symbol_table_test.py
@@ -916,7 +916,7 @@ def test_merge_container_syms():
         tab1.merge(tab4)
     err_txt = str(err.value)
     assert "Cannot merge Symbol Table:" in err_txt
-    assert "due to unresolvable name clashes." in err_txt
+    assert "due to unresolvable name clashes" in err_txt
 
 
 def test_merge_same_routine_symbol(fortran_reader):
@@ -1424,6 +1424,10 @@ def test_wildcard_imports():
     csym.wildcard_import = True
     assert container_table.wildcard_imports() == set([csym.name])
     assert sched_table.wildcard_imports() == set([csym.name])
+    # Limiting the scope for the search
+    assert sched_table.wildcard_imports(scope_limit=sched_table.node) == set()
+    assert (sched_table.wildcard_imports(scope_limit=container_table.node)
+            == set([csym.name]))
 
 
 def test_view():
diff --git a/src/psyclone/tests/psyir/transformations/hoist_trans_test.py b/src/psyclone/tests/psyir/transformations/hoist_trans_test.py
index 9a69054d8..b6ef50a31 100644
--- a/src/psyclone/tests/psyir/transformations/hoist_trans_test.py
+++ b/src/psyclone/tests/psyir/transformations/hoist_trans_test.py
@@ -423,7 +423,7 @@ def test_validate_checks_for_side_effects(fortran_reader):
             myfunc = a
         end function
         subroutine test()
-          use other
+          integer :: zanm
           do i=1,10
               zanm = myfunc(3)
           enddo
diff --git a/src/psyclone/tests/psyir/transformations/parallel_loop_trans_test.py b/src/psyclone/tests/psyir/transformations/parallel_loop_trans_test.py
index 27dd66849..5de142422 100644
--- a/src/psyclone/tests/psyir/transformations/parallel_loop_trans_test.py
+++ b/src/psyclone/tests/psyir/transformations/parallel_loop_trans_test.py
@@ -93,13 +93,13 @@ def test_paralooptrans_validate_pure_calls(fortran_reader):
     '''
     psyir = fortran_reader.psyir_from_source('''
         subroutine my_sub()
-          use other, only: my_sub
+          use other, only: my_sub2
           integer :: i
           real :: var(10) = 1
 
           do i = LBOUND(var), UBOUND(var)  ! Inquiry calls are pure
             var(i) = max(i, 3)             ! max is pure
-            call my_sub(i)                 ! purity is unknown
+            call my_sub2(i)                ! purity is unknown
           end do
         end subroutine my_sub''')
 
@@ -109,12 +109,12 @@ def test_paralooptrans_validate_pure_calls(fortran_reader):
     with pytest.raises(TransformationError) as err:
         trans.validate(loop, {"verbose": True})
     assert ("Loop cannot be parallelised because it cannot guarantee that "
-            "the following calls are pure: ['my_sub']" in str(err.value))
+            "the following calls are pure: ['my_sub2']" in str(err.value))
 
     # Check that forcing the transformation or setting it to "pure" let the
     # validation pass
     trans.validate(loop.copy(), {"force": True})
-    loop.scope.symbol_table.lookup("my_sub").is_pure = True
+    loop.scope.symbol_table.lookup("my_sub2").is_pure = True
     trans.validate(loop)