search

stfc / fparser

This project maintains and develops a Fortran parser called fparser2 written purely in Python which supports Fortran 2003 and some Fortran 2008. A legacy parser fparser1 is also available but is not supported. The parsers were originally part of the f2py project by Pearu Peterson.
https://fparser.readthedocs.io
Other
61 stars 29 forks source link

Bugs in fparser2: procedure binding and write statement failure #106

Closed TeranIvy closed 5 years ago

TeranIvy commented 5 years ago

Parsing the LFRic trunk Fortran files with fparser2 revealed two bugs.

1) class Specific_Binding(StmtBase) produces incorrect Fortran string for statement with :: but no <binding-attr-list>, such as in reference_element_mod.F90 (and iterative_solver_mod.F90) where the original code 

     procedure :: populate_entity_centres => cube_populate_entity_centres
     procedure :: reference_element_init

is parsed as 

     PROCEDURE populate_entity_centres => cube_populate_entity_centres
     PROCEDURE reference_element_init

The first parsed string is invalid Fortran and returns compilation error Error: '::' needed in PROCEDURE binding with explicit target at (1). The second parsed string is valid Fortran, however the behaviour of fparser2 is not correct in this case - it should return original string if it is valid Fortran.

2) When parsing the incorrect Fortran write statement in iter_timestep_alg_mod.x90 for lines

          write( log_scratch_space, '(A,2I3)' ) 'loop indices (o, i): ', &
                                                    , outer, inner

the fparser2 fails as it should (fparser1 does not and corrects the code instead of reporting error and failing, #107). However, it does not report FortranSyntaxError and instead produces long and unwieldy error report. This seems like an issue with Output_Item_List in Write_Stmt. Also, it is worth checking how fparser1 gets around this.

TeranIvy commented 5 years ago

The full failure error from bug 2 (write statement) is

  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 1303, in __new__
    return Base.__new__(cls, string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 1328, in match
    obj = Program_Unit(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 8218, in match
    End_Module_Stmt, reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 8261, in match
    None, reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 9235, in match
    reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 1449, in match
    None, string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 6238, in match
    End_Do_Stmt, reader
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 5637, in match
    enable_if_construct_hook=True)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 6238, in match
    End_Do_Stmt, reader
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 390, in match
    obj = cls(reader)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 207, in __new__
    obj = item.parse_line(cls, parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/common/readfortran.py", line 358, in parse_line
    obj = cls(self.line, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 7003, in match
    Output_Item_List(repmap(line[i+1:].lstrip()))
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "<string>", line 4, in match
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 545, in match
    lst.append(subcls(repmap(p.strip())))
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 244, in __new__
    obj = subcls(string, parent_cls=parent_cls)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 222, in __new__
    result = cls.match(string)
  File "/home/h06/ikavcic/PSyclone_work/fparser/src/fparser/two/Fortran2003.py", line 2089, in match
    if string[-1] not in '"\'':
IndexError: string index out of range
rupertford commented 5 years ago

Hi @TeranIvy. I agree that we should be gradually splitting up the test_fortran2003.py file. If you look in the fparser/two/tests/fortran2003 directory you should see a first example of this for rule 201.

As you say, the current testing is patchy in general and I strongly support the idea of testing all aspects of each rule independently.

I was thinking that whenever we modify/update a rule we should make it one of our coding rules that we should split out the associated test from the fortran2003.py file and add it as a separate file in tests/fortran2003 with full testing of all options. However, I did not get round to discussing this with everyone. It sounds like you are thinking something similar.

In terms of naming and structure for tests I quite like the existing approach of the fortran2003 subdirectory with a separate file for each rule with the name of the rule and then its number added to make it easy to see what each test is doing. However, you might have a different suggestion?

TeranIvy commented 5 years ago

Hi @rupertford, I am happy to move the tests I touched into the fortran2003 test directory. I also think your idea of splitting off f2003 tests to be one of the coding rules is a great one - otherwise we will not move much forward with restructuring.

Regarding naming and structure of tests, the approach you mentioned from fortran2003 subdirectory seems to be the cleanest one. I thought of grouping the tests for related language constructs (such as type-bound procedures and their bindings, R448 to R454) but this introduces developer-dependent definition of grouping. I am happy to update and restructure R448 to R454 tests accordingly in this issue (into separate tests in fortran2003 subdirectory, I mean).

rupertford commented 5 years ago

Thanks @TeranIvy. @arporter agrees with the new coding rule so I've updated the wiki (see review guidelines rule 6).

rupertford commented 5 years ago

pr #131 has been merged to master. Closing this issue.