Repository: stfc/fparser

Issue:
fparser2: generate correct OPEN call when unit number argument is not named
When parsing the following OPEN call:

    OPEN( idrst, FILE = TRIM(cdname), FORM = 'unformatted', ACCESS = 'direct'   &
            &       , RECL = 8, STATUS = 'old', ACTION = 'read', IOSTAT = ios, ERR = 987 )

fparser2 generates:

    OPEN(UNIT = <class 'fparser.Fortran2003.File_Unit_Number'>, FILE = TRIM(cdname), FORM = 'unformatted', ACCESS = 'direct', RECL = 8, STATUS = 'old', ACTION = 'read', IOSTAT = ios, ERR = 987)

i.e. the fact that the unit number argument isn't named in the original call appears to cause problems.

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/13772596/badge)](https://coveralls.io/builds/13772596)

Coverage increased (+0.04%) to 82.144% when pulling **39d093df3ba0182d045aea2f651a512d9e8f59aa on open_wo_named_unit** into **3cc5c52f00654edb58b6a7f8bfba6e7225818045 on master**.

arporter: I've just tested this branch with the ORCA2_LIM configuration of NEMO. Modulo the problem with INCLUDE statements (they get removed), fparser2 successfully parses and re-generates the whole code base which I've then compiled (with the Intel compiler).
arporter: In trying to get test coverage for the missed lines highlighted in the review I discovered that the parser failed if a keyword is misspelt (e.g "aunit=23" instead of "unit=23"). I've fixed that bug and obtained coverage of the modified lines. It's probable that this bug will exist in other places in the code but I haven't checked. This possibly points to the need to have a KeywordValueList base class that would cut down on code duplication.

Patch:
diff --git a/src/fparser/Fortran2003.py b/src/fparser/Fortran2003.py
index 429ca80..20f3dca 100644
--- a/src/fparser/Fortran2003.py
+++ b/src/fparser/Fortran2003.py
@@ -161,8 +161,16 @@ class Base(ComparableMixin):
     subclasses = {}
 
     @show_result
-    def __new__(cls, string, parent_cls = None):
+    def __new__(cls, string, parent_cls=None):
         """
+        Create a new instance of this object.
+
+        :param cls: the class of object to create
+        :type cls: :py:type:`type`
+        :param string: (source of) Fortran string to parse
+        :type string: str or :py:class:`FortranReaderBase`
+        :param parent_cls: the parent class of this object
+        :type parent_cls: :py:type:`type`
         """
         if parent_cls is None:
             parent_cls = [cls]
@@ -218,12 +226,9 @@ class Base(ComparableMixin):
                     obj = None
                 if obj is not None:
                     return obj
-
         else:
             raise AssertionError(repr(result))
         errmsg = '%s: %r' % (cls.__name__, string)
-        #if isinstance(string, FortranReaderBase) and string.fifo_item:
-        #    errmsg += ' while reaching %s' % (string.fifo_item[-1])
         raise NoMatchError(errmsg)
 
 ##     def restore_reader(self):
@@ -5379,6 +5384,7 @@ class Internal_File_Variable(Base): # R903
     """
     subclass_names = ['Char_Variable']
 
+
 class Open_Stmt(StmtBase, CALLBase): # R904
     """
     <open-stmt> = OPEN ( <connect-spec-list> )
@@ -5387,10 +5393,15 @@ class Open_Stmt(StmtBase, CALLBase): # R904
     use_names = ['Connect_Spec_List']
     @staticmethod
     def match(string):
-        return CALLBase.match('OPEN', Connect_Spec_List, string, require_rhs=True)
+        # The Connect_Spec_List class is generated automatically
+        # by code at the end of this module
+        return CALLBase.match('OPEN', Connect_Spec_List, string,
+                              require_rhs=True)
 
-class Connect_Spec(KeywordValueBase): # R905
+
+class Connect_Spec(KeywordValueBase):
     """
+    R905
     <connect-spec> = [ UNIT = ] <file-unit-number>
                      | ACCESS = <scalar-default-char-expr>
                      | ACTION = <scalar-default-char-expr>
@@ -5412,26 +5423,40 @@ class Connect_Spec(KeywordValueBase): # R905
                      | STATUS = <scalar-default-char-expr>
     """
     subclass_names = []
-    use_names = ['File_Unit_Number', 'Scalar_Default_Char_Expr', 'Label', 'File_Name_Expr', 'Iomsg_Variable',
+    use_names = ['File_Unit_Number', 'Scalar_Default_Char_Expr', 'Label',
+                 'File_Name_Expr', 'Iomsg_Variable',
                  'Scalar_Int_Expr', 'Scalar_Int_Variable']
+
+    @staticmethod
     def match(string):
-        for (k,v) in [\
-            (['ACCESS','ACTION','ASYNCHRONOUS','BLANK','DECIMAL','DELIM','ENCODING',
-              'FORM','PAD','POSITION','ROUND','SIGN','STATUS'], Scalar_Default_Char_Expr),
-            ('ERR', Label),
-            ('FILE',File_Name_Expr),
-            ('IOSTAT', Scalar_Int_Variable),
-            ('IOMSG', Iomsg_Variable),
-            ('RECL', Scalar_Int_Expr),
-            ('UNIT', File_Unit_Number),
-            ]:
+        '''
+        :param str string: Fortran code to check for a match
+        :return: 2-tuple containing the keyword and value or None if the
+                 supplied string is not a match
+        :rtype: 2-tuple containing keyword (e.g. "UNIT") and associated value
+        '''
+        if "=" not in string:
+            # The only argument which need not be named is the unit number
+            return 'UNIT', File_Unit_Number(string)
+        # We have a keyword-value pair. Check whether it is valid...
+        for (keyword, value) in [
+                (['ACCESS', 'ACTION', 'ASYNCHRONOUS', 'BLANK', 'DECIMAL',
+                  'DELIM', 'ENCODING', 'FORM', 'PAD', 'POSITION', 'ROUND',
+                  'SIGN', 'STATUS'], Scalar_Default_Char_Expr),
+                ('ERR', Label),
+                ('FILE', File_Name_Expr),
+                ('IOSTAT', Scalar_Int_Variable),
+                ('IOMSG', Iomsg_Variable),
+                ('RECL', Scalar_Int_Expr),
+                ('UNIT', File_Unit_Number)]:
             try:
-                obj = KeywordValueBase.match(k, v, string, upper_lhs = True)
+                obj = KeywordValueBase.match(keyword, value, string,
+                                             upper_lhs=True)
             except NoMatchError:
                 obj = None
-            if obj is not None: return obj
-        return 'UNIT', File_Unit_Number
-    match = staticmethod(match)
+            if obj is not None:
+                return obj
+        return None
 
 
 class File_Name_Expr(Base): # R906
@@ -6027,7 +6052,7 @@ items : (Inquire_Spec_List, Scalar_Int_Variable, Output_Item_List)
 
 class Inquire_Spec(KeywordValueBase):  # R930
     """
-:F03R:`930`::
+    :F03R:`930`::
     <inquire-spec> = [ UNIT = ] <file-unit-number>
                      | FILE = <file-name-expr>
                      | ACCESS = <scalar-default-char-variable>
@@ -6065,9 +6090,9 @@ class Inquire_Spec(KeywordValueBase):  # R930
                      | UNFORMATTED = <scalar-default-char-variable>
                      | WRITE = <scalar-default-char-variable>
 
-Attributes
-----------
-items : (str, instance)
+    Attributes
+    ----------
+    items : (str, instance)
     """
     subclass_names = []
     use_names = ['File_Unit_Number', 'File_Name_Expr',
@@ -6077,6 +6102,18 @@ items : (str, instance)
 
     @staticmethod
     def match(string):
+        '''
+        :param str string: The string to check for conformance with an
+                           Inquire_Spec
+        :return: 2-tuple of name (e.g. "UNIT") and value or None if
+                 string is not a valid Inquire_Spec
+        :rtype: 2-tuple where first object represents the name and the
+                second the value.
+        '''
+        if "=" not in string:
+            # The only argument which need not be named is the unit number
+            return 'UNIT', File_Unit_Number(string)
+        # We have a keyword-value pair. Check whether it is valid...
         for (keyword, value) in [
                 (['ACCESS', 'ACTION', 'ASYNCHRONOUS', 'BLANK', 'DECIMAL',
                   'DELIM', 'DIRECT', 'ENCODING', 'FORM', 'NAME', 'PAD',
@@ -6092,11 +6129,14 @@ items : (str, instance)
                 ('IOMSG', Iomsg_Variable),
                 ('FILE', File_Name_Expr),
                 ('UNIT', File_Unit_Number)]:
-            obj = KeywordValueBase.match(keyword, value, string,
-                                         upper_lhs=True)
+            try:
+                obj = KeywordValueBase.match(keyword, value, string,
+                                             upper_lhs=True)
+            except NoMatchError:
+                obj = None
             if obj is not None:
                 return obj
-        return 'UNIT', File_Unit_Number(string)
+        return None
 
 ###############################################################################
 ############################### SECTION 10 ####################################
@@ -7561,14 +7601,16 @@ ClassType = type(Base)
 _names = dir()
 for clsname in _names:
     cls = eval(clsname)
-    if not (isinstance(cls, ClassType) and issubclass(cls, Base) and not cls.__name__.endswith('Base')): continue
+    if not (isinstance(cls, ClassType) and issubclass(cls, Base) and
+            not cls.__name__.endswith('Base')):
+        continue
     names = getattr(cls, 'subclass_names', []) + getattr(cls, 'use_names', [])
     for n in names:
         if n in _names: continue
         if n.endswith('_List'):
             _names.append(n)
             n = n[:-5]
-            #print 'Generating %s_List' % (n)
+            # Generate 'list' class
             exec('''\
 class %s_List(SequenceBase):
     subclass_names = [\'%s\']


Test patch:
diff --git a/src/fparser/tests/fparser2/test_Fortran2003.py b/src/fparser/tests/fparser2/test_Fortran2003.py
index 9b33289..e373b81 100644
--- a/src/fparser/tests/fparser2/test_Fortran2003.py
+++ b/src/fparser/tests/fparser2/test_Fortran2003.py
@@ -2811,7 +2811,7 @@ def test_Inquire_Stmt():  # R929
 
 def test_Inquire_Spec():  # R930
     ''' Test that we recognise the various possible forms of
-    inquire list '''
+    entries in an inquire list '''
     cls = Inquire_Spec
     obj = cls('1')
     assert isinstance(obj, cls), repr(obj)
@@ -2837,6 +2837,128 @@ def test_Inquire_Spec():  # R930
     assert_equal(str(obj), 'DIRECT = a')
 
 
+def test_Inquire_Spec_List():  # pylint: disable=invalid-name
+    ''' Test that we recognise the various possible forms of
+    inquire list - R930
+    '''
+    # Inquire_Spec_List is generated at runtime in Fortran2003.py
+    cls = Inquire_Spec_List
+
+    obj = cls('unit=23, file="a_file.dat"')
+    assert isinstance(obj, cls)
+    assert str(obj) == 'UNIT = 23, FILE = "a_file.dat"'
+
+    # Invalid list (afile= instead of file=)
+    with pytest.raises(NoMatchError) as excinfo:
+        _ = cls('unit=23, afile="a_file.dat"')
+    assert "NoMatchError: Inquire_Spec_List: 'unit=23, afile=" in str(excinfo)
+
+
+def test_Open_Stmt():
+    ''' Check that we correctly parse and re-generate the various forms
+    of OPEN statement (R904)'''
+    cls = Open_Stmt
+    obj = cls("open(23, file='some_file.txt')")
+    assert isinstance(obj, cls)
+    assert str(obj) == "OPEN(UNIT = 23, FILE = 'some_file.txt')"
+    obj = cls("open(unit=23, file='some_file.txt')")
+    assert isinstance(obj, cls)
+    assert str(obj) == "OPEN(UNIT = 23, FILE = 'some_file.txt')"
+
+
+def test_Connect_Spec():
+    ''' Tests for individual elements of Connect_Spec (R905) '''
+    cls = Connect_Spec
+    # Incorrect name for a member of the list
+    with pytest.raises(NoMatchError) as excinfo:
+        _ = cls("afile='a_file.dat'")
+    assert 'NoMatchError: Connect_Spec: "afile=' in str(excinfo)
+
+
+def test_Connect_Spec_List():  # pylint: disable=invalid-name
+    '''
+    Check that we correctly parse the various valid forms of
+    connect specification (R905)
+    '''
+    cls = Connect_Spec_List
+    obj = cls("22, access='direct'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, ACCESS = 'direct'"
+
+    obj = cls("22, action='read'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, ACTION = 'read'"
+
+    obj = cls("22, asynchronous='YES'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, ASYNCHRONOUS = 'YES'"
+
+    obj = cls("22, blank='NULL'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, BLANK = 'NULL'"
+
+    obj = cls("22, decimal='COMMA'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, DECIMAL = 'COMMA'"
+
+    obj = cls("22, delim='APOSTROPHE'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, DELIM = 'APOSTROPHE'"
+
+    obj = cls("22, err=109")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, ERR = 109"
+
+    obj = cls("22, encoding='DEFAULT'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, ENCODING = 'DEFAULT'"
+
+    obj = cls("22, file='a_file.dat'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat'"
+
+    obj = cls("22, file='a_file.dat', form='FORMATTED'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', FORM = 'FORMATTED'"
+
+    obj = cls("22, file='a_file.dat', iomsg=my_string")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', IOMSG = my_string"
+
+    obj = cls("22, file='a_file.dat', iostat=ierr")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', IOSTAT = ierr"
+
+    obj = cls("22, file='a_file.dat', pad='YES'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', PAD = 'YES'"
+
+    obj = cls("22, file='a_file.dat', position='APPEND'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', POSITION = 'APPEND'"
+
+    obj = cls("22, file='a_file.dat', recl=100")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', RECL = 100"
+
+    obj = cls("22, file='a_file.dat', round='UP'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', ROUND = 'UP'"
+
+    obj = cls("22, file='a_file.dat', sign='PLUS'")
+    assert isinstance(obj, cls)
+    assert str(obj) == "UNIT = 22, FILE = 'a_file.dat', SIGN = 'PLUS'"
+
+    obj = cls("22, file='a_file.dat', sign='PLUS', status='OLD'")
+    assert isinstance(obj, cls)
+    assert str(obj) == ("UNIT = 22, FILE = 'a_file.dat', SIGN = 'PLUS', "
+                        "STATUS = 'OLD'")
+
+    # Incorrect name for a member of the list
+    with pytest.raises(NoMatchError) as excinfo:
+        _ = cls("unit=22, afile='a_file.dat', sign='PLUS', status='OLD'")
+    assert 'NoMatchError: Connect_Spec_List: "unit=22, afile=' in str(excinfo)
+
 
 ###############################################################################
 ############################### SECTION 10 ####################################
@@ -3664,42 +3786,43 @@ def test_Contains(): # R1237
 
 
 if 0:
-    nof_needed_tests = 0
-    nof_needed_match = 0
-    total_needs = 0
-    total_classes = 0
-    for name in dir():
-        obj = eval(name)
-        if not isinstance(obj, ClassType): continue
-        if not issubclass(obj, Base): continue
-        clsname = obj.__name__
-        if clsname.endswith('Base'): continue
-        total_classes += 1
-        subclass_names = obj.__dict__.get('subclass_names',None)
-        use_names = obj.__dict__.get('use_names',None)
-        if not use_names: continue
-        match = obj.__dict__.get('match',None)
+    NOF_NEEDED_TESTS = 0
+    NOF_NEEDED_MATCH = 0
+    TOTAL_NEEDS = 0
+    TOTAL_CLASSES = 0
+    for NAME in dir():
+        OBJ = eval(NAME)
+        if not isinstance(OBJ, ClassType): continue
+        if not issubclass(OBJ, Base): continue
+        CLSNAME = OBJ.__name__
+        if CLSNAME.endswith('Base'): continue
+        TOTAL_CLASSES += 1
+        SUBCLASS_NAMES = OBJ.__dict__.get('subclass_names', None)
+        USE_NAMES = OBJ.__dict__.get('use_names', None)
+        if not USE_NAMES: continue
+        MATCH = OBJ.__dict__.get('match', None)
         try:
-            test_cls = eval('test_%s' % (clsname))
+            TEST_CLS = eval('test_{0}'.format(CLSNAME))
         except NameError:
-            test_cls = None
-        total_needs += 1
-        if match is None:
-            if test_cls is None:
-                print('Needs tests:', clsname)
-                print('Needs match implementation:', clsname)
-                nof_needed_tests += 1
-                nof_needed_match += 1
+            TEST_CLS = None
+        TOTAL_NEEDS += 1
+        if MATCH is None:
+            if TEST_CLS is None:
+                print('Needs tests:', CLSNAME)
+                print('Needs match implementation:', CLSNAME)
+                NOF_NEEDED_TESTS += 1
+                NOF_NEEDED_MATCH += 1
             else:
-                print('Needs match implementation:', clsname)
-                nof_needed_match += 1
+                print('Needs match implementation:', CLSNAME)
+                NOF_NEEDED_MATCH += 1
         else:
-            if test_cls is None:
-                print('Needs tests:', clsname)
-                nof_needed_tests += 1
+            if TEST_CLS is None:
+                print('Needs tests:', CLSNAME)
+                NOF_NEEDED_TESTS += 1
         continue
     print('-----')
-    print('Nof match implementation needs:',nof_needed_match,'out of',total_needs)
-    print('Nof tests needs:',nof_needed_tests,'out of',total_needs)
-    print('Total number of classes:',total_classes)
+    print('Nof match implementation needs:', NOF_NEEDED_MATCH,
+          'out of', TOTAL_NEEDS)
+    print('Nof tests needs:', NOF_NEEDED_TESTS, 'out of', TOTAL_NEEDS)
+    print('Total number of classes:', TOTAL_CLASSES)
     print('-----')