(Closes #505) by removing blank line comments being generated as Comment objects.

[LonelyCat124]

@sergisiso This is ready for reivew if tests pass (though they may not, but they did locally so I will investigate if they don't).

I also think I'd like to test this doesn't break anything in PSyclone, but I need to think about how best to do that.

[LonelyCat124]

I can't replicate the failing example, I have only python 3.12, but locally I get

(fparser_env) aidan@DIN-43888:~/fparser/example$ ./make_public.py test_files/make_public.f90 | diff test_files/make_public_correct.f90 -

I will attempt to do the same test on rupert which has 3.14.5.

[LonelyCat124]

It also worked for me on rupert with 3.14.5 so its a big mystery to me. Is there a known issue with this test at all?

[sergisiso]

I also think I'd like to test this doesn't break anything in PSyclone, but I need to think about how best to do that.

Feel free to use my branch in stfc/PSyclone#3440

You just need to update psyclone submodule to the commit you want to test and submit the integration tests.

I was not planning to merge it until we had a new fparser release.

[sergisiso]

@LonelyCat124 This example also fails for me (are you sure you don't have a preinstalled fparser in the environment that those examples would take), anyway the problem is that the linebreaks are gone:

./make_public.py test_files/make_public.f90
MODULE a_mod
  ! Access_Stmt private will be removed:
  ! Attr_Spec with 0 and 1 additional attribute, the protected will be remobed
  REAL :: planet_radius = 123
  REAL, PARAMETER :: planet_radius_constant = 123
  LOGICAL :: public_protected = .FALSE.
  LOGICAL :: only_protected = .FALSE.
  LOGICAL :: private_protected = .FALSE.
  ! Access_stmt with public, this will be unmodified
  PUBLIC :: public_protected
  ! Protected_Stmt - the whole statement will be removed
  ! Access_stmt with private - the whole statement will be removed
  TYPE :: my_type
    ! Private_Components_Stmt in a type will be removed
    INTEGER :: a, b
    CONTAINS
    ! This private will also be removed.
  END TYPE my_type
  ! Access_Spec - the `private` will be removed
  TYPE(my_type), PUBLIC :: my_var
  CONTAINS
  SUBROUTINE sub_a
  END SUBROUTINE sub_a
END MODULE a_mod

❯ cat test_files/make_public_correct.f90
MODULE a_mod

  ! Access_Stmt private will be removed:

  ! Attr_Spec with 0 and 1 additional attribute, the protected will be remobed
  REAL :: planet_radius = 123
  REAL, PARAMETER :: planet_radius_constant = 123

  LOGICAL :: public_protected = .FALSE.
  LOGICAL :: only_protected = .FALSE.
  LOGICAL :: private_protected = .FALSE.

  ! Access_stmt with public, this will be unmodified
  PUBLIC :: public_protected
  ! Protected_Stmt - the whole statement will be removed
  ! Access_stmt with private - the whole statement will be removed

  TYPE :: my_type
    ! Private_Components_Stmt in a type will be removed
    INTEGER :: a, b
    CONTAINS
    ! This private will also be removed.

  END TYPE my_type

  ! Access_Spec - the `private` will be removed
  TYPE(my_type), PUBLIC :: my_var

  CONTAINS
  SUBROUTINE sub_a
  END SUBROUTINE sub_a
END MODULE a_mod

[LonelyCat124]

@LonelyCat124 This example also fails for me (are you sure you don't have a preinstalled fparser in the environment that those examples would take), anyway the problem is that the linebreaks are gone:

./make_public.py test_files/make_public.f90
MODULE a_mod
  ! Access_Stmt private will be removed:
  ! Attr_Spec with 0 and 1 additional attribute, the protected will be remobed
  REAL :: planet_radius = 123
  REAL, PARAMETER :: planet_radius_constant = 123
  LOGICAL :: public_protected = .FALSE.
  LOGICAL :: only_protected = .FALSE.
  LOGICAL :: private_protected = .FALSE.
  ! Access_stmt with public, this will be unmodified
  PUBLIC :: public_protected
  ! Protected_Stmt - the whole statement will be removed
  ! Access_stmt with private - the whole statement will be removed
  TYPE :: my_type
    ! Private_Components_Stmt in a type will be removed
    INTEGER :: a, b
    CONTAINS
    ! This private will also be removed.
  END TYPE my_type
  ! Access_Spec - the `private` will be removed
  TYPE(my_type), PUBLIC :: my_var
  CONTAINS
  SUBROUTINE sub_a
  END SUBROUTINE sub_a
END MODULE a_mod

❯ cat test_files/make_public_correct.f90
MODULE a_mod

  ! Access_Stmt private will be removed:

  ! Attr_Spec with 0 and 1 additional attribute, the protected will be remobed
  REAL :: planet_radius = 123
  REAL, PARAMETER :: planet_radius_constant = 123

  LOGICAL :: public_protected = .FALSE.
  LOGICAL :: only_protected = .FALSE.
  LOGICAL :: private_protected = .FALSE.

  ! Access_stmt with public, this will be unmodified
  PUBLIC :: public_protected
  ! Protected_Stmt - the whole statement will be removed
  ! Access_stmt with private - the whole statement will be removed

  TYPE :: my_type
    ! Private_Components_Stmt in a type will be removed
    INTEGER :: a, b
    CONTAINS
    ! This private will also be removed.

  END TYPE my_type

  ! Access_Spec - the `private` will be removed
  TYPE(my_type), PUBLIC :: my_var

  CONTAINS
  SUBROUTINE sub_a
  END SUBROUTINE sub_a
END MODULE a_mod

Ok apparently I ended up testing from the wrong branch.

Anyway these extra line breaks are wrong, and are introduced by ignore_comments=False (you can't test this by setting ignore_comments=True and both the comments and line breaks are lost) so I will update the output.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.23%. Comparing base (e557461) to head (a4fbd65).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #509   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files          88       88           
  Lines       13863    13861    -2     
=======================================
- Hits        12786    12785    -1     
+ Misses       1077     1076    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[LonelyCat124]

@sergisiso updated as requested. I also think the failing test was wrong rather than this PR, so I've fixed the expected output (it didn't match ignore_comments=True). The changes seem to be working correctly on PSyclone-mirror so I'm happy enough with these changes.

[sergisiso]

@LonelyCat124 I am ok with this implementation, but before approving let me ask if any of the comments below can improve things.

[LonelyCat124]

@sergisiso back to you - I think I don't understand some of the code enough to have a conclusion, but I think the linebreaks we were adding were a bug I introduced as opposed to something that fparser is meant to be able to output. I think we'd need some sort of special class to keep whitespace which I'm not sure how to do without replicating the issue fixed here.

I did swap to using the deque, we can see if it makes any difference performance wise.

[sergisiso]

@LonelyCat124 If in Rupert I do:
$ fparser2 --task=repr /archive/psyclone-tests/latest-run/slow_files/ukca_aero_ctl.F90
It still have the same problem (in this case only parses the top comments before the module, but not the actual module).

[LonelyCat124]

I'll take a look next then, thats odd but maybe some issue with comments more generally I've not considered.

[LonelyCat124]

There's no syntax error in this file right? It looks like this file works with master so I've broken it somehow with this PR? I'll try to check why

[LonelyCat124]

Ok I've worked out the problem and the fix is rough.

The problem comes from returning None from Comment.__new__ when there is a blank line, because eventually in utils.py::BlockBase.match we hit:

if not had_match or endcls and not found_end:
    # We did not get a match from any of the subclasses or

which returns None to the original source of the issue here:

            result = BlockBase.match(Main_Program0, [], None, reader)
            if not result and comments:
                # This program only contains comments.
                return (content,)
            else:
                return result

in Program.match.

I'm going to try one other change, which is to see if we get an empty Comment at

                # We got a match for this class
                had_match = True
                content.append(obj)

in BaseBlock.match if I just don't append a comment with an empty content to the content list, but I'm not sure if that will work or not.

[LonelyCat124]

Thats still not the right place to filter out empty comments, I need to keep looking up the stack trace. This is turning into a lot of a mess...

[LonelyCat124]

I've changed this to a much simpler fix, but we get the empty lines again for now that means that the issue in PSyclone with enabling comments changing the order of the fparser tree (empty lines introduce Comment('') in some CodeBlock (or potentially frontend related) places, but thats not necessarily an fparser problem.

[LonelyCat124]

@sergisiso I've changed the implementation here, removing blank lines is not currently successful, it would need more rethinking, I think thats a problem for later. Feel free to test it on more repos as you feel would be worthwhile, both my syntax failure test and the file you said before now work though.

[sergisiso]

@LonelyCat124 I confirm it now works with my examples as well (both with fparser2 and through psyclone). Unfortunately the deque didn't make any difference, but I still think it is a more logical choice for a FIFO, so let's keep it.

[sergisiso]

@LonelyCat124 Everything looks good now, this is approved for merging.

[arporter]

A bit late now but did you try filtering them out in the FortranReader?

[LonelyCat124]

A bit late now but did you try filtering them out in the FortranReader?

I didn't, I can take a look perhaps, the current behaviour is we get the FortranStringReader (or whichever) readfortran.Comment objects, I'm not sure how that thing is built, maybe its possible to just not add them (since they're ignored anyway with comments not kept).

[LonelyCat124]

Blank comments can be removed by adding:

        if len(self.comment) == 0:
            return True

into readfortran.Comment.isempty