Skip to content

(Closes #457) Fix bug with backslash in strings #462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

arporter
Copy link
Member

@arporter arporter commented Jun 4, 2025

Weirdly, fparser was interpreting backslash "\" as an escape character. This is not a standard Fortran feature. i.e. if you want to escape a quotation mark in a Fortran string you simply repeat it: "''" means "'".

@arporter arporter self-assigned this Jun 4, 2025
@arporter arporter marked this pull request as draft June 4, 2025 15:57
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.15%. Comparing base (370884f) to head (a7f029e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   92.15%   92.15%   -0.01%     
==========================================
  Files          86       86              
  Lines       13734    13733       -1     
==========================================
- Hits        12656    12655       -1     
  Misses       1078     1078              

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

@arporter
Copy link
Member Author

arporter commented Jun 5, 2025

The PSyclone test suite is happy with this branch of fparser so I declare it ready for review. It is a small but quite fundamental change in behaviour that affects both fparser1 and fparser2. However, since interpreting a \ as an escape character is non-standard, I think this is OK.

@arporter arporter marked this pull request as ready for review June 5, 2025 17:12
Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this. The loop looks a lot simpler that what it was before, that's great. I do have the feeling that there is a bug though, please check the comments (and if I am wrong, it needs some tests :) ).

@hiker hiker added bug reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jun 25, 2025
@arporter arporter added in progress and removed reviewed with actions PR has been reviewed and is back with developer labels Jun 25, 2025
Copy link
Member Author

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I've accidentally started a review while attempting to reply to your comments. Anyhow, I've given up on AI and gone back to NI to write a new implementation. I think it's easier to understand now.

@arporter
Copy link
Member Author

OK, ready for another look now @hiker.

Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to be merged, thanks!

@hiker
Copy link
Collaborator

hiker commented Jun 30, 2025

Dang, I need to take that "ready to be merged" back. One line was not covered by your new tests, and that line is actually wrong:
Test (continued line if no closing quote character is found). This adds two tests, but I believe they are actually the same (but the second tests again the handling of 'the other' quote character :) )

fparser/src/fparser/common/tests$ git diff ./test_splitline.py
diff --git a/src/fparser/common/tests/test_splitline.py b/src/fparser/common/tests/test_splitline.py
index cc155c5..68c2612 100644
--- a/src/fparser/common/tests/test_splitline.py
+++ b/src/fparser/common/tests/test_splitline.py
@@ -261,6 +261,8 @@ def test_splitquote(input_line, expected_parts, expected_unterm):
     "input_line, expected_parts, expected_unterm, stopchar, lower",
     [
         ("this is STILL a quote'", ["this is STILL a quote'"], None, "'", True),
+        ("this is STILL a quote", ["this is STILL a quote"], "'", "'", True),
+        ("this is STILL a quote\"", ["this is STILL a quote\""], "'", "'", True),
         ("'' STILL a quote'", ["'' STILL a quote'"], None, "'", True),
         ("'' STILL a', Quote", ["'' STILL a'", ", quote"], None, "'", True),
         ("'' STILL a', Quote", ["'' STILL a'", ", Quote"], None, "'", False),

I believe this is the fix:

fparser/src/fparser$ git diff common/splitline.py
diff --git a/src/fparser/common/splitline.py b/src/fparser/common/splitline.py
index f5667fa..bdcf55b 100644
--- a/src/fparser/common/splitline.py
+++ b/src/fparser/common/splitline.py
@@ -324,7 +324,7 @@ def splitquote(
             pos = end + 1
         else:
             # Didn't find a closing quotation char.
-            return String(line), stopchar
+            return [String(line)], stopchar
 
     while pos < n:
         start = _next_quote(line, start=pos)

@hiker hiker added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jun 30, 2025
@arporter
Copy link
Member Author

Thanks for finding that bug. I didn't understand your suggested change to the test so have made my own. Hope it's in-line with what you had in mind. Weirdly, I couldn't find any report saying that line wasn't covered - how did you find it?

Ready for another look now.

@arporter arporter added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants