390 ignore linemarkers

[hiker]

Fixes #390 by supporting linemarkers (# 123 "test.f90" - indicating line number and file).

I have used the existing preprocessor directive support, since linemarkers looks pretty much like a preprocessor directive (starting with #). But I was unable to get it to verify properly out of the box - I feel like the existing implementation assumes that the value is constant (e.g. #ifdef), while in the case of a linemarker we don't have this (since it would need the line number and filename to verify correctness, and I couldn't work out how to do this). While it worked this way, it was unable to detect invalid linemarkers (# abc "xx.f90" or # 123 'no_single_quote_allowed.f90'). So, instead I added a simple regex check at the beginning, and only if that passes, it will use the established path.

I am happy to reimplement (I know there is pattern_tools.digit_string and file_name ... I just couldn't figure out how to plug this all together.

[hiker]

I could also not find an official syntax for a linemaker, but looked at https://git.cels.anl.gov/ksalapenades/llvm-project/-/blob/develop/clang/test/Preprocessor/line-directive.c (and did some compiler tests to verify what gets rejected)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.24%. Comparing base (73e31b5) to head (ddb78ac).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #498   +/-   ##
=======================================
  Coverage   92.23%   92.24%           
=======================================
  Files          88       88           
  Lines       13852    13866   +14     
=======================================
+ Hits        12776    12790   +14     
  Misses       1076     1076           

☔ 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.

[hiker]

Checking the git log, it appears this code was contributed by @reuterbal - I wonder if you would have time to provide some feedback? Thanks!

[reuterbal]

Happy to provide my 2ct here. I agree that linemarkers should be handled like preprocessor directives and the implementation itself looks good to me.

As for the need for the additional regex validation: I believe you only need to encode the full linemarker specification as a regex pattern to use in WORDClsBase.match - either manually or using the building blocks in pattern_tools - e.g., like here:

fparser/src/fparser/two/pattern_tools.py

Line 416 in 5e124e5

part_ref = name + ~((r"[(]" + name + r"[)]"))

Without testing it, I would expect this would maybe need to look like something like this:

_pattern = r"^\s*#\s+" +  digit_string + "\s+" + file_name

[hiker]

Hi Balthasar,

thanks a lot for the quick review, really appreciated.

Happy to provide my 2ct here. I agree that linemarkers should be handled like preprocessor directives and the implementation itself looks good to me.

I am glad that I made the right decision here :)

As for the need for the additional regex validation: I believe you only need to encode the full linemarker specification as a regex pattern to use in WORDClsBase.match - either manually or using the building blocks in pattern_tools - e.g., like here:

fparser/src/fparser/two/pattern_tools.py

Line 416 in 5e124e5

part_ref = name + ~((r"[(]" + name + r"[)]"))

Without testing it, I would expect this would maybe need to look like something like this:

_pattern = r"^\s*#\s+" +  digit_string + "\s+" + file_name

I tried that, and thanks a lot for the example. Looking at the +digit_string..., made me realise that Pattern overwrites __add_-, but that doesn't work here since the first expression is a string. But I could use digit_string.pattern etc. But then file_name.pattern started with ^, so I couldn't use this. And since digit_string.pattern was pretty long with no apparent advantage over \d+, I actually just removed it and used a simple regex instead.

But then I am stuck with my original problem: I am using:

    _pattern = pattern.Pattern("<linemarker>", r"^\s*#\s+\d+\s+\".*\"")

But if I do this, the matching in WORDClsBase uses _pattern.value ... which is None. _pattern.value is a string, so I can set it to # (which I did in the state that you reviewed it), but then I don't get the rest of the match: The match function basically does:

           my_match = keyword.match(string)
            if my_match is None:
                return None
            line = string[len(my_match.group()) :]
            pattern_value = keyword.value
...

Basically, the rest of the match is discarded (line number and filename) :(

I tried to fix that using:

           my_match = keyword.match(string)
            if my_match is None:
                return None
            line = string[len(my_match.group()) :]
            if keyword.value:
                pattern_value = keyword.value
            else:
                pattern_value = my_match.group()

So, I provide value=None for the linemarker class, and that then triggers returning the fully matched string, including line numbers and filename.

This passed nearly all tests, except it would not remove spaces between "#" and line number and line number and file name. Therefore, I adjusted one linemarker test (but the test still checks that leading spaces before the # are removed, which is consistent with other code in C99_preprocessor). Small changes to __str__ were required.

I am not sure if this is the right solution tbh :(

Maybe it should not be based on WORDClsBase, but instead use the full grammar? But that also feels like an total overkill.

@reuterbal , @arporter , @sergisiso - feedback appreciated! The modification of the match function seems to have no other side effect, all tests were passing. But I am not keen on changing something that might be quite fundamental ;)

[reuterbal]

Nice work, the details of the keyword/value returns were lost on me and your proposal seems sensible to me.
Overall I think that's indeed much better now, no further comments!

[arporter]

Looks good to me Joerg, thanks. I could do with a bit of help understanding some of the changes - probably because I know pretty much nothing about the Pattern class.
I think it would also be good to test the new functionality in context - i.e. provide some Fortran containing some linemarkers and check that things work OK.

[hiker]

I've resolved Balthasar's comments, and addressed all new issues. Back for next review.

[hiker]

@arporter I just remembered that pylint flagged one issue, which looks wrong, but I just wanted to make sure it's not done on purpose before fixing. In test_c99preprocessor.py there is:

    [
...
        "#definex",
        "#define 2a" "#define fail(...,test) test",
        "#define",
        "#define fail(...,...)",
    ],
...
test_c99preprocessor.py:369:0: W1404: Implicit string concatenation found in list (implicit-str-concat)

Adding a , between the two strings works (as in: all tests are passing). It just looks wrong to me :)

The original (pre-black) line was:

@pytest.mark.parametrize('line', [
-    None, '', ' ', '#def', '#defnie', '#definex', '#define 2a'
-    '#define fail(...,test) test', '#define', '#define fail(...,...)'])

Which comes from d44ad584:

@@ -259,7 +259,7 @@ def test_macro_stmt_with_whitespace(line, ref):
 
 @pytest.mark.usefixtures("f2003_create")
 @pytest.mark.parametrize('line', [
-    None, '', ' ', '#def', '#defnie', '#definex',
+    None, '', ' ', '#def', '#defnie', '#definex', '#define 2a'
     '#define fail(...,test) test', '#define', '#define fail(...,...)'])
 def test_incorrect_macro_stmt(line):
     '''Test that incorrectly formed #define statements raise exception'''

Is it ok if I add the missing ,?

[reuterbal]

Nice catch, that was definitely a mistake when I expanded the test parameterisation. I'd say please add the missing ,.

[hiker]

Thanks for the confirmation. Updated to current master, ready for review.

[arporter]

Thanks Joerg, pretty much there now - just a couple of minor things to tidy up and then this can go on.

[hiker]

All issues addressed, thanks! Ready for next review.

[sergisiso]

Since this could be the last PR before a new release, I triggered the psyclone integration tests with this last fparser commit.

[arporter]

Thanks Joerg. CodeCov is happy now and so am I :-)