Codecov Report

Merging #182 into master will increase coverage by 0.6%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #182     +/-   ##
=========================================
+ Coverage   89.16%   89.76%   +0.6%     
=========================================
  Files          33       34      +1     
  Lines       11728    11842    +114     
=========================================
+ Hits        10457    10630    +173     
+ Misses       1271     1212     -59

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e279d5d...e62b3cd. Read the comment docs.

although I must admit I am unsure whether the order in the specification indicates a suggested matching order

The answer is that the order of subclass_names is important. https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/utils.py#L437 iterates through them, and only increments the iteration if there is no match with the previous type https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/utils.py#L455-L459. The Parenthesis type still causes a recursion error, its just we now never get there because the Array constructor type is matched first.

You don't need to check the recursion error, just have a simple test for each of the options and perhaps one error check for invalid input.

Turns out to have been quite challenging... in a isolated context it isn't actually possible to check all of the subclass_types of Primary - for example, it isn't possible to distinguish a Person (12, 'Jones') structure-constructor with a F( X, Y ) function-reference. There is quite some detail that I'm yet to figure out regarding Part_Refs and how these things are resolved. I've also found it somewhat challenging to find out how things like the Type_Param_Name type is actually defined... 🔎

For this class it involves restructuring the docstring (see other refactored classes)

It wasn't immediately obvious what kind of refactoring was needed wrt. docstrings. I've taken the example of the Program type (https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/Fortran2003.py#L188-L192) as it appears to have a separate test module (https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/tests/fortran2003/test_program_r201.py)

The answer is that the order of subclass_names is important. https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/utils.py#L437 iterates through them, and only increments the iteration if there is no match with the previous type https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/utils.py#L455-L459. The Parenthesis type still causes a recursion error, its just we now never get there because the Array constructor type is matched first.

Thanks @pelson. Yes, I agree that the order that the rules are tested matters in fparser (at least from a tree pruning perspective). What I was questioning was whether there was an inbuilt ordering of the rules in the Fortran specification. I've looked through the (Fortran2003) spec in more detail and can't find any text that states that a earlier clause in an "or" rule should be computed before a later clause. My reading of this is that, from the point of view of the spec, the ordering is undefined i.e. it does not matter which order the rules are tested.

Turns out to have been quite challenging... in a isolated context it isn't actually possible to check all of the subclass_types of Primary - for example, it isn't possible to distinguish a Person (12, 'Jones') structure-constructor with a F( X, Y ) function-reference. There is quite some detail that I'm yet to figure out regarding Part_Refs and how these things are resolved. I've also found it somewhat challenging to find out how things like the Type_Param_Name type is actually defined... 🔎

@pelson, sorry if this is complex. There are currently many rules and sub-rules in fparser2 that have not been looked at, so ambiguities, issues and errors will exist. It is not necessary to fix all of these in a PR, just to document any problems with failing tests (xfail) and/or developer documentation and/or new issues.

It wasn't immediately obvious what kind of refactoring was needed wrt. docstrings. I've taken the example of the Program type (https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/Fortran2003.py#L188-L192) as it appears to have a separate test module (https://github.com/stfc/fparser/blob/0.0.8/src/fparser/two/tests/fortran2003/test_program_r201.py)

Yes @pelson, that is the right thing to do. The docstrings in the new tests are meant to more closely follow the rules in the specification document.

The tests in tests/fortran2XXX/test_yyy.py are meant to (gradually) replace the tests in tests/test_fortran2003.py. As new fparser classes are modified and fully tested there should be a migration from tests in tests/test_fortran2003.py to tests/fortran2XXX/test_YYY.py.

I've looked through the (Fortran2003) spec in more detail and can't find any text that states that a earlier clause in an "or" rule should be computed before a later clause. My reading of this is that, from the point of view of the spec, the ordering is undefined i.e. it does not matter which order the rules are tested.

Seems like a reasonable interpretation which is supported loosely by the statement:

§1.4.4 (2) : "The rules on statement ordering are defined rigorously in the definition of program-unit (R202). Expression hierarchy is described rigorously in the definition of expr (R722)".

This does then beg the question of whether the correct fix here would be to make the Parenthesis type more robust.

@rupertford - given the information you've provided, I think the changes I pushed yesterday are ready for re-review. Thanks!

Ok, I've iterated some more on this and think it is ready for further review. I haven't yet opened issues for the xfails - I'd like to get confirmation from you that they are desirable tests before I do.

Just these last few issues then it can go on master. Back to @pelson.