(Closes #335, #326) add block and critical

[arporter]

This takes on the work of @ZedThree in #335 to add support for block and critical constructs.

[codecov]

Codecov Report

Patch coverage: 95.16% and project coverage change: +0.03 🎉

Comparison is base (95eb99a) 91.69% compared to head (a766ac1) 91.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
+ Coverage   91.69%   91.73%   +0.03%     
==========================================
  Files          37       37              
  Lines       13299    13338      +39     
==========================================
+ Hits        12194    12235      +41     
+ Misses       1105     1103       -2     

Impacted Files	Coverage Δ
src/fparser/two/symbol_table.py	100.00% <ø> (ø)
src/fparser/two/parser.py	92.04% <87.50%> (-0.74%)	⬇️
src/fparser/two/Fortran2003.py	94.41% <100.00%> (ø)
src/fparser/two/Fortran2008.py	99.42% <100.00%> (+0.10%)	⬆️
src/fparser/two/utils.py	96.88% <100.00%> (+0.27%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[arporter]

I've found there's a problem in the parser setup - I get test failures if I do pytest ./test_symbol_table.py fortran2008/test_block.py. i.e. if I run some F2003 tests before the F2008 ones. These failures go away if I just run the test suite on test_block.py. The two failures appear when there's a block inside an If_Construct or a Subroutine. The match method of If_Construct hard-wires in the use of certain classes:

    return BlockBase.match(
        If_Then_Stmt,
        [
            Execution_Part_Construct,
            Else_If_Stmt,
            Execution_Part_Construct,
            Else_Stmt,
            Execution_Part_Construct,
        ],
        End_If_Stmt,
        string,
        match_names=True,  # C801
        strict_match_names=True,  # C801
        match_name_classes=(Else_If_Stmt, Else_Stmt, End_If_Stmt),
        enable_if_construct_hook=True,
    )

In turn, Execution_Part_Construct has Executable_Construct as one of its 'subclass_names' and this class is the one that has been extended to include Block and Critical in its 'subclass_names' in Fortran2008. So, are we getting the wrong Executable_Construct class?

[ZedThree]

Might be #326 or #191 ?

[arporter]

Might be #326 or #191 ?

Ah yes, I think you are absolutely right. You've captured the problem very clearly in #326. I think I'll just go for xfail for now as that's quite a big thing to tackle.

[arporter]

Actually, it's still not clear to me what's going on as the parser constructor explicitly wipes the Fortran2003.Base.subclasses dict. What is clear is that the list of subclass_names in Execution_Part_Construct does not include the F2008-specific classes (when we get to match for an If_Construct).

[arporter]

The _setup method in two.parser modifies the subclass_names properties of the various classes and there's no way of recovering from this - this is modifying information that was originally coded in the class definitions at run time.
Since Executable_Construct does not have a match method, its subclass_names will be promoted up until we get to the first class that also doesn't have a match method which will be Execution_Part_Construct:

Execution_Part[match] -> Execution_Part_Construct[no match] -> Executable_Construct[no match] -> BlockConstruct...

Once this process is complete, every name in subclass_names will be for a class with a match method. Therefore,
when this process is repeated when building the parser a second time, the list of subclass_names will not be modified
because there will (apparently) be no need to walk down the tree of classes. This means that the change to the subclass_names of Executable_Construct will have no effect on the list of subclass_names in Execution_Part_Construct.

[arporter]

Upon reflection, I realised that there's no need to modify the subclass_names list of every class since the results are only ever used to populate the Base.subclasses dictionary. Therefore, I can greatly simplify things by just storing the optimised lists of subclass names in a local dict rather than modify the classes.

[arporter]

I'm pretty happy with the Block implementation now. I need to tidy Critical and then address testing, esp. re ScopingRegionMixin.

[arporter]

This is ready for first review now. One for @sergisiso or @rupertford I think. As well as adding support for the Fortran08 Block and Critical constructs, it also updates the way that scoping regions are identified. This has allowed some code simplification.

[arporter]

Ready for another look now. I've created #397 regarding the issue of named block constructs.

[sergisiso]

@arporter There are some conflicts with merging with master and I think now some pylint inlined statements can be removed. Otherwise it should be ready.

[arporter]

Ready for another look now. In my quest to understand what EndStmtBase.match actually returns I tidied it up and added a docstring. Then I had to add tests...anyway, done now.

[sergisiso]

@arporter All comments have been addressed, PR ready to merge.