Repository: stfc/PSyclone (python)

Issue:
Bug in RegionTrans.get_node_list() when passed `[Schedule]`
RegionTrans.get_node_list is supposed to ensure that we always have a valid list of Nodes to transform. However, @imaranresearch has discovered that it is possible to defeat it by passing it a list containing just a Schedule. In this case, we need to do the same as we do when passed a single Schedule node - i.e. return a new list containing its children.

Pull request:
(Closes #3026) bug with RegionTrans.get_node_list

Patch:
diff --git a/changelog b/changelog
index 5c41d8c50..6691cfb0a 100644
--- a/changelog
+++ b/changelog
@@ -1,3 +1,6 @@
+   37) PR #3029 for #3026. Fixes a bug with RegionTrans.get_node_list if a
+   list containing a single Schedule was provided.
+
    35) PR #3025 for #2928. Removes commas between clauses on directives.
 
    34) pr #3016 for #3015. Apply doxygen -u to update doxygen config file.
diff --git a/doc/developer_guide/transformations.rst b/doc/developer_guide/transformations.rst
index bf50f9a80..05fa34481 100644
--- a/doc/developer_guide/transformations.rst
+++ b/doc/developer_guide/transformations.rst
@@ -516,6 +516,8 @@ an if condition or a loop condition, and that intrinsic contains an array sectio
 then PSyclone may generate extra dependencies, which may hurt code performance. If
 this causes issues, please open an issue.
 
+.. _updating_transformation_options:
+
 Moving to the new transformations options
 =========================================
 PSyclone is currently moving from a dictionary of options to keyword arguments
diff --git a/doc/user_guide/transformations.rst b/doc/user_guide/transformations.rst
index 6d9df1b88..d1180bac0 100644
--- a/doc/user_guide/transformations.rst
+++ b/doc/user_guide/transformations.rst
@@ -81,7 +81,8 @@ Ongoing Changes to the options parameter
 PSyclone has supported an ``options`` dictionary to provide options to both
 validate and apply functions on Transformation classes. This behaviour
 is now deprecated, and we are in the process of adding an alternative
-way to pass options to Transformations.
+way to pass options to Transformations. (If you are a developer, see
+these :ref:`instructions<updating_transformation_options>`.)
 
 In the future, each option will have its own argument, for example::
     
diff --git a/src/psyclone/psyir/transformations/region_trans.py b/src/psyclone/psyir/transformations/region_trans.py
index e06314d41..bc034a680 100644
--- a/src/psyclone/psyir/transformations/region_trans.py
+++ b/src/psyclone/psyir/transformations/region_trans.py
@@ -40,14 +40,17 @@
 '''
 
 import abc
+from typing import Dict, List, Union
 
 from psyclone.errors import LazyString
 from psyclone.psyGen import Transformation
-from psyclone.psyir.transformations.transformation_error \
-    import TransformationError
+from psyclone.psyir.transformations.transformation_error import (
+    TransformationError)
 from psyclone.psyir.nodes import Schedule, Node
+from psyclone.utils import transformation_documentation_wrapper
 
 
+@transformation_documentation_wrapper
 class RegionTrans(Transformation, metaclass=abc.ABCMeta):
     # Avoid pylint warning about abstract functions (apply, name) not
     # overwritten:
@@ -66,38 +69,40 @@ class RegionTrans(Transformation, metaclass=abc.ABCMeta):
     # populated by sub-class.
     excluded_node_types = ()
 
-    def get_node_list(self, nodes):
+    def get_node_list(self,
+                      nodes: Union[Node, Schedule, List[Node]]) -> List[Node]:
         '''This is a helper function for region based transformations.
         The parameter for any of those transformations is either a single
-        node, a schedule, or a list of nodes. This function converts this
+        Node, a Schedule, or a list of nodes. This function converts this
         into a list of nodes according to the parameter type. This function
         will always return a copy, to avoid issues e.g. if a child list
         of a node should be provided, and a transformation changes the order
         in this list (which would then also change the order of the
         nodes in the tree).
 
+        If `nodes` happens to be a list containing a single Schedule node then
+        the behaviour is the same as if it had been a single Schedule node,
+        i.e. we return a list of that Schedule's children.
+
         :param nodes: can be a single node, a schedule or a list of nodes.
-        :type nodes: Union[:py:obj:`psyclone.psyir.nodes.Node`,
-                           :py:obj:`psyclone.psyir.nodes.Schedule`,
-                           List[:py:obj:`psyclone.psyir.nodes.Node`]
-        :param options: a dictionary with options for transformations.
-        :type options: Optional[Dict[str,Any]]
 
         :returns: a list of nodes.
-        :rtype: List[:py:class:`psyclone.psyir.nodes.Node`]
 
-        :raises TransformationError: if the supplied parameter is neither a \
+        :raises TransformationError: if the supplied parameter is neither a
             single Node, nor a Schedule, nor a list of Nodes.
 
         '''
-        if isinstance(nodes, list) and \
-                all(isinstance(node, Node) for node in nodes):
-            # We still need to return a copy, since the user might have
-            # provided Node.children as parameter.
-            return nodes[:]
+        if isinstance(nodes, list):
+            if len(nodes) == 1 and isinstance(nodes[0], Schedule):
+                # We've been passed a list containing a single schedule so
+                # return a list of its children.
+                return nodes[0].children[:]
+            if all(isinstance(node, Node) for node in nodes):
+                # We still need to return a copy, since the user might have
+                # provided Node.children as parameter.
+                return nodes[:]
         if isinstance(nodes, Schedule):
-            # We've been passed a Schedule so default to enclosing its
-            # children.
+            # We've been passed a Schedule so return a list of its children.
             return nodes.children[:]
         if isinstance(nodes, Node):
             # Single node that's not a Schedule
@@ -110,31 +115,47 @@ class RegionTrans(Transformation, metaclass=abc.ABCMeta):
                                   f"in a Schedule but have been passed an "
                                   f"object of type: {arg_type}")
 
-    def validate(self, nodes, options=None):
-        '''Checks that the nodes in node_list are valid for a region
+    def apply(self,
+              nodes: Union[Node, Schedule, List[Node]],
+              node_type_check: bool = True,
+              options: Dict = None,
+              **kwargs):
+        '''
+        Apply the RegionTrans transformation.
+
+        :param nodes: can be a single node, a schedule or a list of nodes.
+        :param node_type_check: whether or not the type of the
+            nodes enclosed in the region should be tested to avoid using
+            unsupported nodes inside a region.
+
+        '''
+        # This method is only here to expose the `node_type_check` option
+        # for sub-classes.
+        self.validate(nodes, node_type_check=node_type_check, options=options,
+                      **kwargs)
+        if options:
+            # TODO #2668 - deprecate options dictionary.
+            node_type_check = options.get("node-type-check", True)
+
+        super().apply(nodes, node_type_check=node_type_check)
+
+    def validate(self,
+                 nodes: Union[Node, Schedule, List[Node]],
+                 options: Dict = None,
+                 **kwargs):
+        '''Checks that the nodes in `nodes` are valid for a region
         transformation.
 
         :param nodes: can be a single node, a schedule or a list of nodes.
-        :type nodes: Union[:py:obj:`psyclone.psyir.nodes.Node`,
-                           :py:obj:`psyclone.psyir.nodes.Schedule`,
-                           List[:py:obj:`psyclone.psyir.nodes.Node`]
-        :param options: a dictionary with options for transformations.
-        :type options: Optional[Dict[str,Any]]
-        :param bool options["node-type-check"]: this flag controls if the \
-                type of the nodes enclosed in the region should be tested \
-                to avoid using unsupported nodes inside a region.
-
-        :raises TransformationError: if the nodes in the list are not \
-                in the original order in which they are in the AST, \
+
+        :raises TransformationError: if the nodes in the list are not
+                in the original order in which they are in the AST,
                 a node is duplicated or the nodes have different parents.
-        :raises TransformationError: if any of the nodes to be enclosed in \
+        :raises TransformationError: if any of the nodes to be enclosed in
                 the region are of an unsupported type.
-        :raises TransformationError: if the parent of the supplied Nodes is \
+        :raises TransformationError: if the parent of the supplied Nodes is
                 not a Schedule or a Directive.
-        :raises TransformationError: if the nodes are in a NEMO \
-                Schedule and the transformation acts on the child of a \
-                single-line If or Where statment.
-        :raises TransformationError: if the supplied options are not a \
+        :raises TransformationError: if the supplied options are not a
                 dictionary.
 
         '''
@@ -145,11 +166,16 @@ class RegionTrans(Transformation, metaclass=abc.ABCMeta):
         node_list = self.get_node_list(nodes)
 
         if not options:
-            options = {}
-        if not isinstance(options, dict):
-            raise TransformationError(
-                f"Transformation apply method options argument must be a "
-                f"dictionary but found '{type(options).__name__}'.")
+            self.validate_options(**kwargs)
+            node_type_check = self.get_option("node_type_check", **kwargs)
+        else:
+            # TODO #2668 - deprecate options dictionary.
+            if not isinstance(options, dict):
+                raise TransformationError(
+                    f"Transformation apply method options argument must be a "
+                    f"dictionary but found '{type(options).__name__}'.")
+            node_type_check = options.get("node-type-check", True)
+
         node_parent = node_list[0].parent
         prev_position = -1
         for child in node_list:
@@ -165,7 +191,7 @@ class RegionTrans(Transformation, metaclass=abc.ABCMeta):
             prev_position = child.position
 
         # Check that the proposed region contains only supported node types
-        if options.get("node-type-check", True):
+        if node_type_check:
             for child in node_list:
                 for bad in child.walk(self.excluded_node_types):
                     # Ideally we'd just use `bad.debug_string()` always
@@ -199,8 +225,8 @@ class RegionTrans(Transformation, metaclass=abc.ABCMeta):
         # Sanity check that we've not been passed the condition part of
         # an If statement or the bounds of a Loop. If the parent node is
         # a Loop or IfBlock then we can only accept a single Schedule.
-        if not isinstance(node_parent, Schedule) and \
-                not isinstance(node_list[0], Schedule):
+        if (not isinstance(node_parent, Schedule) and
+                not isinstance(node_list[0], Schedule)):
             # We've already checked for lists with len > 1 that contain a
             # Schedule above so if the first item is a Schedule then that's
             # all the list contains.


Test patch:
diff --git a/src/psyclone/tests/psyir/transformations/region_trans_test.py b/src/psyclone/tests/psyir/transformations/region_trans_test.py
index c18a207a8..a0c18f6b2 100644
--- a/src/psyclone/tests/psyir/transformations/region_trans_test.py
+++ b/src/psyclone/tests/psyir/transformations/region_trans_test.py
@@ -31,14 +31,17 @@
 # ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 # POSSIBILITY OF SUCH DAMAGE.
 # -----------------------------------------------------------------------------
-# Author J. Henrichs, Bureau of Meteorology
+# Authors: J. Henrichs, Bureau of Meteorology
+#          A. R. Porter, STFC Daresbury Laboratory
 
 ''' Module containing tests for testing RegionTrans'''
 
 import pytest
 
 from psyclone.psyir.transformations import TransformationError
-from psyclone.psyir.nodes import CodeBlock, Node, Schedule
+from psyclone.psyir.nodes import (
+    CodeBlock, IfBlock, IntrinsicCall, Node, Reference, Schedule)
+from psyclone.psyir.symbols import DataSymbol, REAL_TYPE
 from psyclone.psyir.transformations import RegionTrans
 from psyclone.tests.utilities import get_invoke
 from psyclone.gocean1p0 import GOLoop
@@ -51,14 +54,32 @@ class MyRegionTrans(RegionTrans):
     '''
     excluded_node_types = ()
 
-    def apply(self, node, options=None):
-        '''Dummy only to make this not abstract.'''
-
     @property
     def name(self):
         '''Dummy only to make this not abstract.'''
 
 
+def test_rt_apply(fortran_reader, monkeypatch):
+    '''Test the apply method of RegionTrans.'''
+    my_rt = MyRegionTrans()
+    # We test that options get passed to validate correctly be making the
+    # transformation reject CodeBlocks by default.
+    monkeypatch.setattr(my_rt, "excluded_node_types", (CodeBlock,))
+    psyir = fortran_reader.psyir_from_source('''\
+    subroutine doit()
+      integer :: i
+      write (*,*) i
+    end subroutine doit''')
+    sched = psyir.walk(Schedule)[0]
+    with pytest.raises(TransformationError) as err:
+        my_rt.apply(sched.children)
+    assert "Nodes of type 'CodeBlock' cannot be enclosed" in str(err.value)
+    # Check that both the deprecated and new way of passing an option work.
+    # TODO #2668: Deprecate options dictionary.
+    my_rt.apply(sched.children, options={"node-type-check": False})
+    my_rt.apply(sched.children, node_type_check=False)
+
+
 # -----------------------------------------------------------------------------
 @pytest.mark.parametrize("node_list", [5, [1], [1, Node()],
                                        [Node(), 1], [Node(), Node(), 1],
@@ -108,6 +129,16 @@ def test_get_node_list():
     assert node_list2 == node_list
     assert node_list2 is not node_list
 
+    # Provide a list containing a single Schedule
+    # -------------------------------------------
+    sym = DataSymbol("x", REAL_TYPE)
+    sched.children = [IntrinsicCall.create(IntrinsicCall.Intrinsic.SIN,
+                                           [Reference(sym)])]
+    node_list3 = my_rt.get_node_list([sched])
+    # Result should be the children of the Schedule.
+    assert len(node_list3) == 1
+    assert node_list3[0] is sched.children[0]
+
 
 # -----------------------------------------------------------------------------
 def test_validate_errors(fortran_reader):
@@ -154,6 +185,31 @@ def test_validate_errors(fortran_reader):
     assert "Children are not consecutive children of one parent:" \
            in str(err.value)
 
+    # Check when list of nodes contains >1 Schdule. Have to use an IfBlock to
+    # exercise this case.
+    psyir = fortran_reader.psyir_from_source('''
+    program my_prog
+      integer :: ji
+      if(ji == 0)then
+        write(*,*) ji
+      else
+        write(*,*) "no"
+      end if
+      do ji = 1, 10
+        ji = ji + 1
+      end do
+    end program my_prog''')
+    ifblck = psyir.walk(IfBlock)[0]
+    with pytest.raises(TransformationError) as err:
+        my_rt.validate(ifblck.children)
+    assert ("Cannot apply a transformation to multiple nodes when one or more "
+            "is a Schedule" in str(err.value))
+    with pytest.raises(TransformationError) as err:
+        my_rt.validate(ifblck.children[0])
+    assert ("Cannot apply transformation to the immediate children of a "
+            "Loop/IfBlock unless it is to a single Schedule representing the "
+            "Loop/If/Else body" in str(err.value))
+
     # Supply incorrect type to options parameter:
     with pytest.raises(TransformationError) as err:
         my_rt.validate([], options=1)
diff --git a/src/psyclone/tests/psyir/transformations/transformations_test.py b/src/psyclone/tests/psyir/transformations/transformations_test.py
index e41f26b5e..d7f55161d 100644
--- a/src/psyclone/tests/psyir/transformations/transformations_test.py
+++ b/src/psyclone/tests/psyir/transformations/transformations_test.py
@@ -44,11 +44,12 @@ import os
 import pytest
 import sys
 from fparser.common.readfortran import FortranStringReader
-from psyclone.psyir.nodes import CodeBlock, IfBlock, Literal, Loop, Node, \
-    Reference, Schedule, Statement, ACCLoopDirective, OMPMasterDirective, \
-    OMPDoDirective, OMPLoopDirective, Routine
-from psyclone.psyir.symbols import DataSymbol, INTEGER_TYPE, BOOLEAN_TYPE, \
-    ImportInterface, ContainerSymbol
+from psyclone.psyir.nodes import (
+    CodeBlock, Literal, Loop, Node, Reference, Schedule, Statement,
+    ACCLoopDirective, OMPMasterDirective,
+    OMPDoDirective, OMPLoopDirective, Routine)
+from psyclone.psyir.symbols import (DataSymbol, INTEGER_TYPE,
+                                    ImportInterface, ContainerSymbol)
 from psyclone.psyir.transformations import ProfileTrans, RegionTrans, \
     TransformationError
 from psyclone.tests.utilities import get_invoke, Compile
@@ -722,28 +723,6 @@ end subroutine x"""
     assert "nowait" not in out
 
 
-def test_ifblock_children_region():
-    ''' Check that we reject attempts to transform the conditional part of
-    an If statement or to include both the if- and else-clauses in a region
-    (without their parent). '''
-    acct = ACCParallelTrans()
-    # Construct a valid IfBlock
-    condition = Reference(DataSymbol('condition', BOOLEAN_TYPE))
-    ifblock = IfBlock.create(condition, [], [])
-
-    # Attempt to put all of the children of the IfBlock into a region. This
-    # is an error because the first child is the conditional part of the
-    # IfBlock.
-    with pytest.raises(TransformationError) as err:
-        super(ACCParallelTrans, acct).validate([ifblock.children[0]])
-    assert ("transformation to the immediate children of a Loop/IfBlock "
-            "unless it is to a single Schedule" in str(err.value))
-    with pytest.raises(TransformationError) as err:
-        super(ACCParallelTrans, acct).validate(ifblock.children[1:])
-    assert (" to multiple nodes when one or more is a Schedule. "
-            "Either target a single Schedule or " in str(err.value))
-
-
 def test_regiontrans_wrong_children():
     ''' Check that the validate method raises the expected error if
         passed the wrong children of a Node. (e.g. those representing the