Repository: MetOffice/stylist

Issue:
Rule for trapping uninitialised pointers
Initialising a Fortran pointer is syntactically optional but not doing so is a bad idea. Why? Because the `associated(...)` intrinsic returns a boolean based on the tri-state of a pointer. A pointer may be associated, unassociated or undefined. The intrinsic will return `.true.` if it is associated, `.false.` if it is not and one of those if it is undefined.
Therefore pointers should always be initialised unassociated. Then you know where you stand.

Discussion:

Patch:
diff --git a/setup.py b/setup.py
index 2699d2a..5b65503 100644
--- a/setup.py
+++ b/setup.py
@@ -44,7 +44,7 @@ setup(
     package_dir={'': 'source'},
     packages=find_packages(where='source'),
     python_requires='>=3.6, <4',
-    install_requires=['fparser'],
+    install_requires=['fparser > 0.0.10'],
     extras_require={
         'dev': ['check-manifest', 'flake8'],
         'test': ['pytest', 'pytest-cov'],
diff --git a/source/stylist/fortran.py b/source/stylist/fortran.py
index c62dbbf..89638f6 100644
--- a/source/stylist/fortran.py
+++ b/source/stylist/fortran.py
@@ -4,14 +4,13 @@
 # The file LICENCE, distributed with this code, contains details of the terms
 # under which the code may be used.
 ##############################################################################
-'''
+"""
 Rules relating to Fortran source.
-'''
-
+"""
 from abc import ABCMeta, abstractmethod
 from typing import List
 
-import fparser  # type: ignore
+import fparser.two.Fortran2003 as Fortran2003  # type: ignore
 
 from stylist.issue import Issue
 from stylist.rule import Rule
@@ -51,9 +50,9 @@ class FortranRule(Rule, metaclass=ABCMeta):
 
 class FortranCharacterset(Rule):
     # pylint: disable=too-few-public-methods
-    '''
+    """
     Scans the source for characters which are not supported by Fortran.
-    '''
+    """
     _FORTRAN_LETTER = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'
     _FORTRAN_DIGIT = '0123456789'
     _FORTRAN_ALPHANUMERIC = _FORTRAN_LETTER + _FORTRAN_DIGIT + '_'
@@ -67,13 +66,13 @@ class FortranCharacterset(Rule):
 
     def examine(self, subject: FortranSource) -> List[Issue]:
         # pylint: disable=too-many-branches
-        '''
+        """
         Examines the source code for none Fortran characters.
 
         This is complicated by the fact that the source must consist of only
         certain characters except comments and strings. These may contain
         anything.
-        '''
+        """
         issues = super(FortranCharacterset, self).examine(subject)
 
         text = subject.get_text()
@@ -117,12 +116,13 @@ class FortranCharacterset(Rule):
 
 class MissingImplicit(FortranRule):
     # pylint: disable=too-few-public-methods
-    '''
+    """
     Catches cases where code blocks which could have an "implicit" statement
     don't.
-    '''
-    def __init__(self, default: str):
-        '''
+    """
+
+    def __init__(self, default='none'):
+        """
         Constructor taking a default implication.
 
         Todo: This rule was designed to check merely for the presence of an
@@ -135,14 +135,14 @@ class MissingImplicit(FortranRule):
         Todo: At the moment the default is not used. In the future we may want
         to allow rules to enforce style rather than simply reporting it. In
         this case the default would be used where none is present.
-        '''
+        """
         assert default.lower() in ('none', 'private', 'public')
         self._default = default.lower()
 
-    _NATURE_MAP = {fparser.two.Fortran2003.Program_Stmt: 'Program',
-                   fparser.two.Fortran2003.Module_Stmt: 'Module',
-                   fparser.two.Fortran2003.Subroutine_Stmt: 'Subroutine',
-                   fparser.two.Fortran2003.Function_Stmt: 'Function'}
+    _NATURE_MAP = {Fortran2003.Program_Stmt: 'Program',
+                   Fortran2003.Module_Stmt: 'Module',
+                   Fortran2003.Subroutine_Stmt: 'Subroutine',
+                   Fortran2003.Function_Stmt: 'Function'}
 
     def examine_fortran(self, subject: FortranSource) -> List[Issue]:
         issues = []
@@ -167,31 +167,136 @@ class MissingImplicit(FortranRule):
                 description = "{thing} '{name}' is missing " \
                               + "an implicit statement"
                 description = description.format(thing=nature, name=name)
-                issues.append(Issue(description))
+                issues.append(Issue(description,
+                                    line=scope_statement.item.span[0]))
         return issues
 
 
 class MissingOnly(FortranRule):
-    '''
+    """
     Catches cases where a "use" statement is present but has no "only" claus.
-    '''
+    """
+
     def __init__(self, ignore: List[str] = []):
-        '''
+        """
         Constructs a "MissingOnly" rule object taking a list of exception
         modules which are not required to have an "only" clause.
-        '''
+        """
         assert isinstance(ignore, list)
         self._ignore = ignore
 
     def examine_fortran(self, subject: FortranSource) -> List[Issue]:
         issues = []
 
-        for statement in subject.find_all(fparser.two.Fortran2003.Use_Stmt):
+        for statement in subject.find_all(Fortran2003.Use_Stmt):
             module = statement.items[2]
             onlies = statement.items[4]
             if str(module).lower() not in self._ignore:
                 if onlies is None:
                     description = 'Usage of "{module}" without "only" clause.'
-                    issues.append(Issue(description.format(module=module)))
+                    issues.append(Issue(description.format(module=module),
+                                        line=statement.item.span[0]))
+
+        return issues
+
+
+class MissingPointerInit(FortranRule):
+    """
+    Catches cases where a pointer is declared without being initialised.
+    """
+
+    def __init__(self, default='null()'):
+        """
+        Constructs a MissingPointerInit rule object taking a default
+        assignment.
+
+        Todo: Obviously the default is not used as we don't support coercing
+              source at the moment.
+
+        @param default: Target to be used if missing assignment found.
+        """
+        self._default = default
+
+    def examine_fortran(self, subject: FortranSource):
+        issues: List[Issue] = []
+
+        candidates: List[Fortran2003.StmtBase] = []
+        # Component variables
+        candidates.extend(
+            subject.find_all(Fortran2003.Data_Component_Def_Stmt))
+        # Component Procedure
+        candidates.extend(
+            subject.find_all(Fortran2003.Proc_Component_Def_Stmt))
+        # Procedure declaration
+        candidates.extend(
+            subject.find_all(Fortran2003.Procedure_Declaration_Stmt))
+        # Variable declaration
+        candidates.extend(
+            subject.find_all(Fortran2003.Type_Declaration_Stmt))
+
+        return_variable = ''
+        for candidate in candidates:
+            if isinstance(candidate,  # Is variable
+                          (Fortran2003.Data_Component_Def_Stmt,
+                           Fortran2003.Type_Declaration_Stmt)):
+                if isinstance(candidate.parent.parent,
+                              Fortran2003.Function_Subprogram):
+                    # Is contained in a function
+                    function_block: Fortran2003.Function_Subprogram \
+                        = candidate.parent.parent
+                    statement = None
+                    for thing in function_block.children:
+                        if isinstance(thing, Fortran2003.Function_Stmt):
+                            statement = thing
+                            break
+                    if statement is None:
+                        message = "Malformed parse tree: Function subprogram" \
+                                  " without Function statement"
+                        raise Exception(message)
+                    suffix = statement.items[3]
+                    if isinstance(suffix, Fortran2003.Suffix):
+                        if isinstance(suffix.items[0], Fortran2003.Name):
+                            # Is return variable
+                            return_variable = str(suffix.items[0])
+
+            problem = '{line}: Declaration of pointer ' \
+                      '"{name}" without initialisation.'
+
+            attributes = candidate.items[1]
+            if attributes is None:
+                continue
+            cannon_attr = list(str(item).lower().replace(' ', '')
+                               for item in attributes.items)
+            argument_def = 'intent(in)' in cannon_attr \
+                           or 'intent(out)' in cannon_attr \
+                           or 'intent(inout)' in cannon_attr
+            if 'pointer' in cannon_attr and not argument_def:
+                for variable in candidate.items[2].items:
+                    if isinstance(candidate,  # Is variable
+                                  (Fortran2003.Data_Component_Def_Stmt,
+                                   Fortran2003.Type_Declaration_Stmt)):
+                        name = str(variable.items[0])
+                        if name == return_variable:
+                            continue  # Return variables cannot be initialised.
+                        init = variable.items[3]
+                        if init is None:
+                            line_number = candidate.item.span[0]
+                            message = problem.format(line=line_number,
+                                                     name=name)
+                            issues.append(Issue(message))
+                    elif isinstance(candidate,  # Is procedure
+                                    (Fortran2003.Proc_Component_Def_Stmt,
+                                     Fortran2003.Procedure_Declaration_Stmt)):
+                        name = str(variable)
+                        if isinstance(variable, Fortran2003.Name):
+                            line_number = candidate.item.span[0]
+                            message = problem.format(line=line_number,
+                                                     name=name)
+                            issues.append(Issue(message))
+                    else:
+                        message \
+                            = f"Unexpected source element: {repr(statement)}"
+                        raise Exception(message)
 
+        issues.sort()
         return issues
diff --git a/source/stylist/issue.py b/source/stylist/issue.py
index a79e70b..b558932 100644
--- a/source/stylist/issue.py
+++ b/source/stylist/issue.py
@@ -33,6 +33,9 @@ class Issue(object):
                              line=self._line,
                              description=self._description)
 
+    def __lt__(self, other):
+        return str(self) < str(other)
+
     def set_filename(self, filename: str) -> None:
         '''
         Attaches a filename to this issue.
diff --git a/source/stylist/source.py b/source/stylist/source.py
index 6fcad2e..7d7e3ad 100644
--- a/source/stylist/source.py
+++ b/source/stylist/source.py
@@ -81,7 +81,7 @@ class TextProcessor(SourceText, metaclass=ABCMeta):
         self._source = source
 
 
-class MetaCPreProcessor(TextProcessor.__class__):
+class MetaCPreProcessor(ABCMeta):
     def __str__(self):
         return "C preprocessor"
 
@@ -111,7 +111,7 @@ class CPreProcessor(TextProcessor, metaclass=MetaCPreProcessor):
         return text
 
 
-class MetaFortranPreProcessor(TextProcessor.__class__):
+class MetaFortranPreProcessor(ABCMeta):
     def __str__(self):
         return "Fortran preprocessor"
 
@@ -141,7 +141,7 @@ class FortranPreProcessor(TextProcessor, metaclass=MetaFortranPreProcessor):
         return text
 
 
-class MetaPFUnitProcessor(TextProcessor.__class__):
+class MetaPFUnitProcessor(ABCMeta):
     def __str__(self):
         return "pFUnit preprocessor"
 
@@ -195,7 +195,7 @@ class SourceTree(object, metaclass=ABCMeta):
         return self._text.get_text()
 
 
-class MetaFortranSource(SourceTree.__class__):
+class MetaFortranSource(ABCMeta):
     def __str__(self):
         return 'Fortran source'
 
@@ -380,7 +380,7 @@ class FortranSource(SourceTree, metaclass=MetaFortranSource):
                 FortranSource.print_tree(child.items, indent+1)
 
 
-class MetaCSource(SourceTree.__class__):
+class MetaCSource(ABCMeta):
     def __str__(self):
         return "C source"
 
diff --git a/source/stylist/style.py b/source/stylist/style.py
index 7107b97..13fc18a 100644
--- a/source/stylist/style.py
+++ b/source/stylist/style.py
@@ -59,5 +59,6 @@ class LFRicStyle(Style):
         rules = [stylist.fortran.FortranCharacterset(),
                  stylist.rule.TrailingWhitespace(),
                  stylist.fortran.MissingImplicit('none'),
-                 stylist.fortran.MissingOnly(ignore=['pfunit_mod', 'xios'])]
+                 stylist.fortran.MissingOnly(ignore=['pfunit_mod', 'xios']),
+                 stylist.fortran.MissingPointerInit()]
         super(LFRicStyle, self).__init__(rules)


Test patch:
diff --git a/tests/fortran_missing_implicit_test.py b/tests/fortran_missing_implicit_test.py
index 8818050..8e72774 100644
--- a/tests/fortran_missing_implicit_test.py
+++ b/tests/fortran_missing_implicit_test.py
@@ -193,7 +193,9 @@ class TestMissingImplicit(object):
         reader = SourceStringReader(empty_program_unit_implicit[0])
         source = FortranSource(reader)
 
-        expectation = empty_program_unit_implicit[1]
+        expectation = []
+        for thing in empty_program_unit_implicit[1]:
+            expectation.append(f"1: {thing}")
 
         unit_under_test = stylist.fortran.MissingImplicit('none')
         issues = unit_under_test.examine(source)
@@ -208,15 +210,26 @@ class TestMissingImplicit(object):
         '''
         # pylint: disable=no-self-use
         procedure = '\n'.join([subprogram_implicit[0],
-                               second_subprogram_implicit[0]])
+                               second_subprogram_implicit[0]]).strip()
         text = containing_program_unit[0].format(procedure=procedure)
         reader = SourceStringReader(text)
         source = FortranSource(reader)
 
+        insert_point = containing_program_unit[0].find('{procedure}')
+        insert_line = containing_program_unit[0].count('\n',
+                                                       0,
+                                                       insert_point) + 1
+        first_len = subprogram_implicit[0].count('\n')
+        if first_len > 0:
+            first_len += 1
+
         expectation = []
-        expectation.extend(containing_program_unit[1])
-        expectation.extend(subprogram_implicit[1])
-        expectation.extend(second_subprogram_implicit[1])
+        for thing in containing_program_unit[1]:
+            expectation.append(f"1: {thing}")
+        for thing in subprogram_implicit[1]:
+            expectation.append(f"{insert_line}: {thing}")
+        for thing in second_subprogram_implicit[1]:
+            expectation.append(f"{insert_line + first_len}: {thing}")
 
         unit_under_test = stylist.fortran.MissingImplicit('none')
         issues = unit_under_test.examine(source)
diff --git a/tests/fortran_missing_only_test.py b/tests/fortran_missing_only_test.py
index 814be24..07a209f 100644
--- a/tests/fortran_missing_only_test.py
+++ b/tests/fortran_missing_only_test.py
@@ -5,7 +5,7 @@
 # under which the code may be used.
 ##############################################################################
 '''
-Tests of the rule for missing "only" clauses.
+Test of the rule for missing "only" clauses.
 '''
 
 import pytest
@@ -64,11 +64,10 @@ class TestMissingOnly(object):
         yield request.param
 
     def test_use(self, unit_type, unit_usage, procedure_usage, ignorance):
-        '''
+        """
         Checks that the rule reports missing "use" clauses correctly.
-        '''
-        # pylint: disable=no-self-use
-        def prepare(params):
+        """
+        def prepare(line_number: int, params):
             usage = []
             expectations = []
             for details in params:
@@ -77,15 +76,16 @@ class TestMissingOnly(object):
                     line = 'use {0}'.format(details[0])
                     if details[1]:
                         line += ', only : {0}'.format(', '.join(details[1]))
+                        line_number += 1
                     elif details[0] not in ignorance:
-                        message = 'Usage of "{0}" without "only" clause.'
-                        expectations.append(message.format(details[0]))
+                        message = f'{line_number}: Usage of "{details[0]}" ' \
+                                  f'without "only" clause.'
+                        expectations.append(message)
+                        line_number += 1
                 if line:
                     usage.append(line)
-            return usage, expectations
+            return usage, expectations, line_number
 
-        unit_lines, unit_expects = prepare(unit_usage)
-        proc_lines, proc_expects = prepare(procedure_usage)
         text = '''{type} test
                    {uusage}
                    implicit none
@@ -95,7 +95,11 @@ class TestMissingOnly(object):
                      implicit none
                    end subroutine foo
                  end {type} test
-                      '''
+               '''
+        unit_lines, unit_expects, last_line = prepare(2, unit_usage)
+        if len(unit_lines) == 0:
+            last_line += 1
+        proc_lines, proc_expects, _ = prepare(last_line + 3, procedure_usage)
         reader = SourceStringReader(text.format(type=unit_type,
                                                 uusage='\n'.join(unit_lines),
                                                 pusage='\n'.join(proc_lines)))
@@ -103,7 +107,10 @@ class TestMissingOnly(object):
 
         expectation = list(unit_expects)
         expectation.extend(proc_expects)
-
+        print(text.format(type=unit_type,
+                          uusage='\n'.join(unit_lines),
+                          pusage='\n'.join(proc_lines)))
+        print(expectation)
         if ignorance:
             unit_under_test = stylist.fortran.MissingOnly(ignore=ignorance)
         else:
diff --git a/tests/fortran_pointer_init_test.py b/tests/fortran_pointer_init_test.py
new file mode 100644
index 0000000..9a1c3ea
--- /dev/null
+++ b/tests/fortran_pointer_init_test.py
@@ -0,0 +1,169 @@
+#!/usr/bin/env python
+##############################################################################
+# (c) Crown copyright 2018 Met Office. All rights reserved.
+# The file LICENCE, distributed with this code, contains details of the terms
+# under which the code may be used.
+##############################################################################
+"""
+Test of the rule for missing pointer initialisation.
+"""
+from typing import List
+
+import pytest
+import stylist.fortran
+from stylist.source import FortranSource, SourceStringReader
+
+
+class TestMissingPointerInit(object):
+    """
+    Tests the checker of missing pointer initialisation.
+    """
+    @pytest.fixture(scope='class',
+                    params=['program', 'module'])
+    def prog_unit(self, request):
+        """
+        Parameter fixture giving program unit types.
+        """
+        # pylint: disable=no-self-use
+        yield request.param
+
+    @pytest.fixture(scope='class',
+                    params=['', 'pointer', 'nullpointer'])
+    def type_pointer(self, request):
+        """
+        Parameter fixture giving pointer type for program unit.
+        """
+        # pylint: disable=no-self-use
+        yield request.param
+
+    @pytest.fixture(scope='class',
+                    params=['', 'pointer', 'nullpointer'])
+    def unit_pointer(self, request):
+        """
+        Parameter fixture giving pointer type for program unit.
+        """
+        # pylint: disable=no-self-use
+        yield request.param
+
+    @pytest.fixture(scope='class',
+                    params=['', 'pointer', 'nullpointer'])
+    def proc_pointer(self, request):
+        """
+        Parameter fixture giving pointer type for procedure.
+        """
+        # pylint: disable=no-self-use
+        yield request.param
+
+    _ATTR_MAP = {'': '',
+                 'pointer': ', pointer',
+                 'nullpointer': ', pointer'}
+    _DECL_MAP = {'': 'normal',
+                 'pointer': 'bare',
+                 'nullpointer': 'null => null()'}
+
+    def test_pointer_init(self,
+                          prog_unit,
+                          type_pointer,
+                          unit_pointer,
+                          proc_pointer):
+        """
+        Checks that the rule reports missing pointer initialisation correctly.
+        """
+        template = '''
+{prog_unit} test
+  implicit none
+  private
+  type foo_type
+    private
+    integer{type_attr} :: type_{type_decl}, second_type_{type_decl}
+    ! Procedure pointers have to be pointers.
+    procedure(some_if), nopass, pointer :: type_proc_pointer_{type_decl}, &
+                                           other_type_proc_pointer_{type_decl}
+  end type foo_type
+  type(foo_type){unit_attr} :: unit_{unit_decl}
+  procedure(some_if){unit_attr} :: unit_proc_{unit_decl}
+contains
+  ! A comment for the function
+  !
+  function qux() result(answer)
+    implicit none
+    ! It isn't possible to initialise function return variables
+    real, pointer :: answer(:)
+  end function qux
+  !
+  ! Comment for the subroutine
+  !
+  subroutine bar()
+    implicit none
+    type(foo_type){proc_attr} :: proc_{proc_decl}
+    procedure(some_if){proc_attr} :: proc_proc_{proc_decl}
+    integer{proc_attr} :: proc_var_{proc_decl}
+  end subroutine bar
+  logical function whatev()
+    ! Functions may also be declared this way.
+    implicit none
+    whatev = .true.
+  end function whatev
+  subroutine baz( thing, thang, thong, bong )
+    implicit none
+    ! Intent in arguments aren't initialised
+    type(foo_type), intent(in), pointer :: thing
+    procedure(some_if), intent(in), pointer :: thang
+    ! Oddly intent out arguments can't be initialised either
+    logical, intent(out), pointer :: bong
+    !Intent inout arguments aren't initialised
+    real, intent(in out), pointer :: thong(:)
+  end subroutine baz
+end {prog_unit} test
+'''
+
+        expectation: List[str] = []
+        message = '{line}: Declaration of pointer ' \
+                  '"{name}" without initialisation.'
+        if type_pointer != 'nullpointer':
+            suffix = self._DECL_MAP[type_pointer].partition(' ')[0]
+            expectation.extend([
+                message.format(line=9,
+                               name=f'other_type_proc_pointer_{suffix}'),
+                message.format(line=9,
+                               name=f'type_proc_pointer_{suffix}')
+            ])
+        if type_pointer == 'pointer':
+            expectation.extend([message.format(line=7,
+                                               name='second_type_bare'),
+                                message.format(line=7,
+                                               name='type_bare')])
+        if proc_pointer == 'pointer':
+            expectation.extend([message.format(line=27,
+                                               name='proc_bare'),
+                                message.format(line=28,
+                                               name='proc_proc_bare'),
+                                message.format(line=29,
+                                               name='proc_var_bare')])
+        if unit_pointer == 'pointer':
+            expectation.extend([message.format(line=12,
+                                               name='unit_bare'),
+                                message.format(line=13,
+                                               name='unit_proc_bare')])
+        expectation.sort(key=lambda x: (int(x.split(':', 1)[0]),
+                                        x.split(':', 1)))
+
+        text = template.format(
+            prog_unit=prog_unit,
+            type_attr=self._ATTR_MAP[type_pointer],
+            type_decl=self._DECL_MAP[type_pointer],
+            unit_attr=self._ATTR_MAP[unit_pointer],
+            unit_decl=self._DECL_MAP[unit_pointer],
+            proc_attr=self._ATTR_MAP[proc_pointer],
+            proc_decl=self._DECL_MAP[proc_pointer])
+        print(text)  # Shows up in failure reports, for debugging
+        reader = SourceStringReader(text)
+        source = FortranSource(reader)
+        unit_under_test = stylist.fortran.MissingPointerInit()
+        issues = unit_under_test.examine(source)
+        issue_descriptions = [str(issue) for issue in issues]
+        issue_descriptions.sort(key=lambda x: (int(x.split(':', 1)[0]),
+                                               x.split(':', 1)))
+        print(issue_descriptions)
+        print(expectation)
+        assert issue_descriptions == expectation
diff --git a/tests/style_test.py b/tests/style_test.py
index 25b145d..54bb306 100644
--- a/tests/style_test.py
+++ b/tests/style_test.py
@@ -107,4 +107,5 @@ class TestLFRicStyle(object):
         assert unit_under_test.list_rules() == ['FortranCharacterset',
                                                 'TrailingWhitespace',
                                                 'MissingImplicit',
-                                                'MissingOnly']
+                                                'MissingOnly',
+                                                'MissingPointerInit']