Adding strict order statements (closes #353) #358

rupertford · 2022-06-20T21:52:23Z

Some of the rules using blockbase have a strict order. This PR enforces this where it was not already used. The parser runs a bit faster as a result.

However, there is an additional case where we only want blockbase to allow 0 or 1 instances of a particular rule to match. At the moment that is not enforced and we can end up with multiple Specification_Part entries within a Main_Program if, for example, an implicit none is incorrectly added before a use statement. This is not a good outcome as tools might rightly rely on there only being one of these and we end up with strange errors, rather than it being caught by fparser in the first place. Note that on master we only get one Specification_Part but the implicit none and use statements are allowed to be in the wrong order.

rupertford · 2022-06-20T22:31:45Z

I've fixed the 0 or 1 instance problem but now break the addition of comments so I think I'll need to add a comment loop before incrementing the class pointer.

Note, there may be cases where it might be worth having a list of classes that are 0 or 1, as a subset of the classes might require this. I should look at this.

rupertford · 2022-06-21T15:33:08Z

There appears to be a noticable improvement in performance for the example benchmark ...

master: Time taken for parse = 36.27s
Branch: Time taken for parse = 29.06s

…arser into 353_strict_order

codecov · 2022-06-21T21:44:31Z

Codecov Report

Base: 91.48% // Head: 91.25% // Decreases project coverage by -0.22% ⚠️

Coverage data is based on head (9f0dcbc) compared to base (8df46ac).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 9f0dcbc differs from pull request most recent head 512cdfe. Consider uploading reports for the commit 512cdfe to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
- Coverage   91.48%   91.25%   -0.23%     
==========================================
  Files          37       37              
  Lines       13138    13134       -4     
==========================================
- Hits        12019    11986      -33     
- Misses       1119     1148      +29

Impacted Files	Coverage Δ
src/fparser/two/Fortran2003.py	`93.61% <ø> (-0.60%)`	⬇️
src/fparser/two/utils.py	`95.47% <100.00%> (-0.12%)`	⬇️
src/fparser/two/Fortran2008.py	`100.00% <0.00%> (ø)`

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rupertford · 2022-06-24T22:43:21Z

When testing the latest version with LFRic algorithm code I am finding parse errors in code like ...

...
use x
#if defined(UM_PHYSICS)
use y
...

which is not correct (although PSyclone does not promise to support ifdef'ed code).

I guess it is because we are incrementing the class match without dealing with comments and ifdefs. I need to look at how to fix this. This probably related to the change in location of comments that is observed in tests (which I convinced myself was OK).

rupertford · 2022-06-24T22:56:01Z

Interestingly comments between use statements seem to be OK but ifdef's are not. I thought we processed these in the same way. I guess there must be a difference somewhere.

rupertford · 2022-06-24T23:12:12Z

OK, I now realise that comment, include and preprocess rules are added to the general blockbase rules, so if we do them in order they will not be picked up at the right point. I think we need to not add them to the rules and then consume all of them as we go along (without incrementing the class reference), but it needs thinking about.

rupertford · 2022-06-27T00:18:47Z

Comment restructuring seems to be working. All tests pass, LFRic code is being parsed OK with psyclone and psyclone tests pass after finding and fixing another bug in one of the examples.

Annoyingly, running psyclone on all the lfric algorithm codes does not seem to give any performance improvement.

arporter · 2022-08-11T07:28:46Z

Hi @rupertford, do you think you could add in a test that I wrote for #366 into this PR:

def test_do_loop_coments():
    """Check that comments around and within do loops appear in the expected
    places in the tree."""
    source = """\
program test
integer :: arg1
integer :: iterator
!comment_out 1
arg1 = 10
!comment_out 2
do iterator = 0,arg1
    !comment_in
    print *, iterator
end do
!comment_out 3
end program test"""
    reader = get_reader(source, isfree=True, ignore_comments=False)
    obj = Program(reader)
    comments = walk(obj, Comment)
    assert isinstance(comments[1].parent, Execution_Part)
    assert isinstance(comments[2].parent, TODO)

rupertford · 2022-08-11T09:12:31Z

Added test, which seems to work as expected, giving the following (which preserves comment locations) ...

PROGRAM test
  INTEGER :: arg1
  INTEGER :: iterator
  !comment_out 1
  arg1 = 10
  !comment_out 2
  DO iterator = 0, arg1
    !comment_in
    PRINT *, iterator
  END DO
  !comment_out 3
END PROGRAM test

rupertford · 2022-08-11T09:23:50Z

I'm not sure why I didn't make this ready for review before so here we go ...

arporter

All looks good apart from the fact that there seem to be no tests for the new once_only flag - I'd expect to see some tests that we reject code that has more than one implicit none in it for example.
Coverage is fine and Black means I don't have to worry much about style (although see inline for a couple of pylint things).
I don't think that the Dev Guide needs to be updated.

arporter · 2022-08-31T20:25:19Z

src/fparser/two/utils.py

                if not strict_order:
                    # Return to start of classes list now that we've matched.
                    i = 0
+                elif once_only:


Suggest a comment: we had a match for this sub-class and once_only is set so move on to the next sub-class.

Added comment.

arporter · 2022-08-31T20:29:20Z

src/fparser/two/utils.py

@@ -714,9 +714,15 @@ def match(
                    # We've found the enclosing end statement so break out
                    found_end = True
                    break
+


Not strictly yours but since we're here pylint complains that the elif at L701 could be an if. Probably the same goes for L705.

arporter · 2022-08-31T20:33:20Z

src/fparser/two/Fortran2003.py

@@ -10157,6 +10159,7 @@ def match(reader):
            [Specification_Part, Module_Subprogram_Part],
            End_Module_Stmt,
            reader,


Each sub-class can only match once for a valid module? (I initially got worried that this wouldn't work for multiple modules in a file but it's fine.)

Yes, a Module can only contain one Module_Stmt, one Specification_Part, one Module_Subprogram_Part and one End_Module_Stmt. Multiple modules are supported in 'higher level' rules, such as Program_Unit.

arporter · 2022-08-31T20:41:15Z

src/fparser/two/tests/test_comments.py

    #        |--> Execution_Part
    #        |    |--> Write_Stmt
-    #        |    \--> Comment
+    #        |    |--> Comment
    #   .    .
    #   .
    #   |--> Comment
    from fparser.two.Fortran2003 import Main_Program, Write_Stmt, End_Program_Stmt


Please move these imports to the top (and check for any subsequent repeats).

Moved all imports to the top.

arporter · 2022-08-31T20:42:46Z

src/fparser/two/tests/test_comments.py

    assert isinstance(comment, Comment)
    assert "! First comment" in str(comment)
-    comment = spec_part.content[2].content[0]
+    spec_part = fn_unit.children[2]
+    comment = spec_part.content[2]


s/content/children/ since you use that on the previous line?

Good point.

rupertford · 2022-09-15T16:13:44Z

I started adding a test for strict_order and found that the implementation is not working correctly. For example

integer :: a
use my_mod

should not match with Specification_Part as use my_mod should precede integer :: a.

The rule order is ...

[Use_Stmt, Import_Stmt, Implicit_Part, Declaration_Construct]

However, integer :: a is returned as a match with use my_mod being ignored.

issue #353. Adding strict order statements.

f532b42

rupertford added enhancement in progress labels Jun 20, 2022

rupertford self-assigned this Jun 20, 2022

pr #358. Fix for 0 or 1 blockbase matching.

8b90bc0

Merge branch 'master' into 353_strict_order

2c831c3

rupertford added 2 commits June 21, 2022 22:37

pr #358. Updated tests.

c20d9e1

pr #358 Merge branch '353_strict_order' of https://github.com/stfc/fp…

3eb33d4

…arser into 353_strict_order

rupertford added 4 commits June 26, 2022 17:22

pr #358. Fixed comment/include/preprocess problem.

0f7b914

pr #358 Merge branch 'master' into 353_strict_order

77e0b10

pr #358. black changes.

800af82

pr #358. Black changes.

8b371f4

rupertford mentioned this pull request Aug 11, 2022

Comment hierarchy issue #366

Open

pr #358 Merge branch 'master' into 353_strict_order

b7b4f27

rupertford added 2 commits August 11, 2022 10:13

pr #358. Added test for comment placement as requested by @arporter.

e4baa7f

pr #358. Back to black.

e4efa6a

rupertford added ready for review and removed in progress labels Aug 11, 2022

rupertford requested review from arporter and sergisiso August 11, 2022 09:24

arporter added under review and removed ready for review labels Aug 31, 2022

Merge branch 'master' into 353_strict_order

9f0dcbc

arporter requested changes Aug 31, 2022

View reviewed changes

arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Aug 31, 2022

rupertford added 2 commits September 15, 2022 15:18

pr #358 Merge branch 'master' into 353_strict_order

365a80d

pr #358. Addressed some of reviewer's comments.

512cdfe

rupertford added in progress and removed reviewed with actions PR has been reviewed and is back with developer labels Sep 15, 2022

Merge branch 'master' into 353_strict_order

2fea1f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding strict order statements (closes #353) #358

Adding strict order statements (closes #353) #358

rupertford commented Jun 20, 2022 •

edited

Loading

rupertford commented Jun 20, 2022

rupertford commented Jun 21, 2022

codecov bot commented Jun 21, 2022 •

edited

Loading

rupertford commented Jun 24, 2022

rupertford commented Jun 24, 2022

rupertford commented Jun 24, 2022

rupertford commented Jun 27, 2022

arporter commented Aug 11, 2022

rupertford commented Aug 11, 2022

rupertford commented Aug 11, 2022

arporter left a comment

arporter Aug 31, 2022

rupertford Sep 15, 2022

arporter Aug 31, 2022

rupertford Sep 15, 2022

arporter Aug 31, 2022

rupertford Sep 15, 2022

arporter Aug 31, 2022

rupertford Sep 15, 2022

arporter Aug 31, 2022

rupertford Sep 15, 2022

rupertford commented Sep 15, 2022

Adding strict order statements (closes #353) #358

Are you sure you want to change the base?

Adding strict order statements (closes #353) #358

Conversation

rupertford commented Jun 20, 2022 • edited Loading

rupertford commented Jun 20, 2022

rupertford commented Jun 21, 2022

codecov bot commented Jun 21, 2022 • edited Loading

Codecov Report

rupertford commented Jun 24, 2022

rupertford commented Jun 24, 2022

rupertford commented Jun 24, 2022

rupertford commented Jun 27, 2022

arporter commented Aug 11, 2022

rupertford commented Aug 11, 2022

rupertford commented Aug 11, 2022

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupertford commented Sep 15, 2022

rupertford commented Jun 20, 2022 •

edited

Loading

codecov bot commented Jun 21, 2022 •

edited

Loading