Fix: raise error when namelist group is not closed before the start of a new group

[dhohn]

Hi, I hope this is okay to submit — I ran into a bug where a missing / terminator on a namelist group would silently cause the following group to be skipped entirely. For example, in a file like:

  &open_group
  a = 1

  &closed_group
  b = 2
  /

closed_group would be silently dropped from the parsed result, which can be pretty hard to debug. I've looked at the bevahiour of gfortran and ifx for this case. gfortran errors and exits. ifx reads both groups without warning. I prefer an error personally so I made the fix to be an error.

The fix peeks at the next non-whitespace token when & is seen as a group terminator. If it's not a recognised F77-style terminator (&end, standalone &, or $), a ValueError is raised pointing to the offending group. itertools.tee is used to peek without consuming from the token stream, which felt consistent with how lookahead is already handled elsewhere in the parser.

This might also relate to this issue: #160

Would be happy to adjust anything if this doesn't fit. Thanks for considering it!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22b3d95a57

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[marshallward]

Thanks, this looks very valuable. As you say, the behavior can vary depending on the compiler, regardless of what is in the language standard. So it might be a good idea to introduce a flag to control this behavior.

For example, the namelist above could be interpreted as follows.

  &open_group   ! <-- start a new group
  a = 1
  &  ! <-- legacy termination token (from &closed_group)
    
  ! Not formally a namelist group, so excluded from data transfer?
  closed_group
  b = 2
  /
  
  ! Continue searching for the next & or EOF.

This might still be a formally correct namelist, even some compilers raise an error. I've never been entirely sure myself. (I think it depends on what defines a "data transfer".)

I agree that this is a frustratingly subtle error, so it makes sense for this to raise an error on default, with an option to disable it.

I'll try to look over the lookahead a bit later today. Thanks for putting this together!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.03%. Comparing base (420c780) to head (22b3d95).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   97.21%   97.03%   -0.18%     
==========================================
  Files           9        9              
  Lines        2152     2160       +8     
  Branches      358      361       +3     
==========================================
+ Hits         2092     2096       +4     
- Misses         35       39       +4     
  Partials       25       25              

Flag	Coverage Δ
python2.7	?
python3.5	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

[marshallward]

Should this test for both '&' and '$' ?

For what it's worth, NVIDIA appears to happily match &grp with $end and every combination therein.

[dhohn]

Yes. Oh I didnt even consider mixed opening and closing tokens. That would become really tricky with multiple groups with different conventions in one file. I would hope such files just dont exists (but you never know with fortran ;)

[dhohn]

Hi @marshallward ,

Thanks for having a look and commenting. I have taken up both your suggestions above and added tests for those cases as well.

Regarding the mixed open/close token usage, thats probably an edge case of an edge case if at all. Do you want to do anything about that? The current code also allows any combination of tokens to open and close.