Skip to content

Throw clearer error message when integer dimension indices are present; remove known ddt case sensitivity #662

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.

Sign up for GitHub

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

Merged
mkavulich merged 6 commits into from
Jul 9, 2025
Merged

Throw clearer error message when integer dimension indices are present; remove known ddt case sensitivity #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d1761c
fix two bugs
peverwhee Jun 25, 2025
f28b5d6
add error handling for integer dimension indices
peverwhee Jun 25, 2025
6e7098d
back out line length fix and address review comments
peverwhee Jun 26, 2025
faff223
another stab at fixing line length cusp bug
peverwhee Jun 27, 2025
4f7230d
Add test for writing long strings
Jun 27, 2025
17442fe
Merge pull request #17 from gold2718/small-bugfixes
peverwhee Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions scripts/fortran_tools/fortran_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,12 @@ def write(self, statement, indent_level, continue_line=False):
if len(outstr) > best:
if self._in_quote(outstr[0:best+1]):
line_continue = '&'
elif not outstr[best+1:].lstrip():
# If the next line is empty, the current line is done
# and is equal to the max line length. Do not use
# continue and set best to line_max (best+1)
line_continue = False
best = best+1
else:
# If next line is just comment, do not use continue
line_continue = outstr[best+1:].lstrip()[0] != '!'
Expand Down
10 changes: 5 additions & 5 deletions scripts/metadata_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def parse_metadata_file(filename, known_ddts, run_env, skip_ddt_check=False):
meta_tables.append(new_table)
table_titles.append(ntitle)
if new_table.table_type == 'ddt':
known_ddts.append(ntitle)
known_ddts.append(ntitle.lower())
# end if
else:
errmsg = 'Duplicate metadata table, {}, at {}:{}'
Expand Down Expand Up @@ -540,7 +540,7 @@ def __init_from_file(self, known_ddts, run_env, skip_ddt_check=False):
raise CCPPError(self.__pobj.error_message)
# end if
if self.table_type == "ddt":
known_ddts.append(self.table_name)
known_ddts.append(self.table_name.lower())
# end if
if self.__dependencies is None:
self.__dependencies = []
Expand Down Expand Up @@ -882,7 +882,7 @@ def __init_from_file(self, table_name, table_type, known_ddts, module_name,
self.title, start_ctx))
# end if
if self.header_type == "ddt":
known_ddts.append(self.title)
known_ddts.append(self.title.lower())
# end if
# Initialize our ParseSource parent
super().__init__(self.title, self.header_type, self.__pobj)
Expand Down Expand Up @@ -954,11 +954,11 @@ def parse_variable(self, curr_line, known_ddts, skip_ddt_check=False):
pval_str = prop[1]
if ((pname == 'type') and
(not check_fortran_intrinsic(pval_str, error=False))):
if skip_ddt_check or pval_str in known_ddts:
if skip_ddt_check or pval_str.lower() in known_ddts:
if skip_ddt_check:
register_fortran_ddt_name(pval_str)
# end if
pval = pval_str
pval = pval_str.lower()
pname = 'ddt_type'
else:
errmsg = "Unknown DDT type, {}".format(pval_str)
Expand Down
30 changes: 27 additions & 3 deletions scripts/parse_tools/parse_checkers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,13 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
>>> check_dimensions("hi_mom", None, True) #doctest: +IGNORE_EXCEPTION_DETAIL
Traceback (most recent call last):
CCPPError: 'hi_mom' is invalid; not a list
>>> check_dimensions(["1:dim1", "dim2name"], None, True) #doctest: +IGNORE_EXCEPTION_DETAIL
Traceback (most recent call last):
CCPPError: '1:dim1 is an invalid dimension name; integer dimension indices not supported
>>> check_dimensions(["ccpp_constant_one:1", "dim2name"], None, True)
['ccpp_constant_one:1', 'dim2name']
Copy link
Copy Markdown
Collaborator

@gold2718 gold2718 Jun 26, 2025

Choose a reason for hiding this comment

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

Why is this okay? Isn't 1 an integer dimension index?

Copy link
Copy Markdown
Collaborator Author

@peverwhee peverwhee Jun 26, 2025

Choose a reason for hiding this comment

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

a valid question! The problem I ran into is that sometimes the parse checker is called after the "ccpp_constant_one:" is prepended to the integer dimension (which I believe we're allowing) and sometimes it's called before.

So, the metadata could specify dimensions = (2) and we'd end up calling check_dimensions on ccpp_constant_one:2

I thought about diving deeper into figuring out what the specifics were of when we're calling what, but decided that ccpp_constant_one:1 is the same as saying 1 and left it at that.

Copy link
Copy Markdown
Collaborator

@gold2718 gold2718 Jun 26, 2025

Choose a reason for hiding this comment

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

Here is what is confusing: If "integer dimension indices not supported", how is the 1, an integer okay? Also, when did we decide it is okay to use integers for array extents?

Copy link
Copy Markdown
Collaborator Author

@peverwhee peverwhee Jun 26, 2025

Choose a reason for hiding this comment

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

basically, it's only ok if you do ccpp_constant_one:1. any other use of integer indices isn't ok. And that's because of my earlier comment - to appease the checker.

As for when we started allowing integer sizes, i believe @dustinswales brought that in recently for his work in SCM - #652

"""
info_msg = None
if not isinstance(test_val, list):
if error:
raise CCPPError("'{}' is invalid; not a list".format(test_val))
Expand All @@ -123,10 +129,24 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
# end if
# Check possible dim styles (a, a:b, a:, :b, :, ::, a:b:c, a::c)
tdims = [x.strip() for x in isplit if len(x) > 0]
starts_at_one = False
if len(tdims) > 0 and tdims[0] == 'ccpp_constant_one':
starts_at_one = True
# end if
is_int = False
for tdim in tdims:
# Check numeric value first
try:
valid = isinstance(int(tdim), int)
is_int = isinstance(int(tdim), int)
# Allow integer dimensions, but not indices
if is_int:
valid = starts_at_one or len(tdims) == 1
if not valid:
info_msg = 'integer dimension indices not supported'
# end if
else:
valid = False
# end if
except ValueError as ve:
# Not an integer, try a Fortran ID
valid = check_fortran_id(tdim, None,
Expand All @@ -147,8 +167,12 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
# End try
if not valid:
if error:
errmsg = "'{}' is an invalid dimension name"
raise CCPPError(errmsg.format(item))
if info_msg:
errmsg = f"'{item}' is an invalid dimension name; {info_msg}"
else:
errmsg = f"'{item}' is an invalid dimension name"
# end if
raise CCPPError(errmsg)
else:
test_val = None
# end if
Expand Down
111 changes: 111 additions & 0 deletions test/unit_tests/sample_files/fortran_files/long_string_test.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
!
! This work (Common Community Physics Package Framework), identified by
! NOAA, NCAR, CU/CIRES, is free of known copyright restrictions and is
! placed in the public domain.
!
! THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
! IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
! FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
! THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
! IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
! CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.


!>
!! @brief Auto-generated Test of long string breaking for FortranWriter
!!
!
module long_string_test

foo100 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'

foo101 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'

foo102 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901'

foo103 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012'

foo104 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123'

foo105 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234'

foo106 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345'

foo107 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456'

foo108 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567'

foo109 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678'

foo110 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'

foo111 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'

foo112 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901'

foo113 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012'

foo114 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123'

foo115 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234'

foo116 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345'

foo117 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456'

foo118 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567'

foo119 = &
'01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678'

foo120 = &
'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'

foo121 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&'
foo122 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&1'
foo123 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&12'
foo124 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&123'
foo125 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&1234'
foo126 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&12345'
foo127 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&123456'
foo128 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&1234567'
foo129 = &
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890&
&12345678'

end module long_string_test
28 changes: 27 additions & 1 deletion test/unit_tests/test_fortran_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,32 @@ def test_good_comments(self):
amsg = f"{generate} does not match {compare}"
self.assertTrue(filecmp.cmp(generate, compare, shallow=False), msg=amsg)

def test_long_strings(self):
"""Test breaking of long strings"""
# Setup
testname = "long_string_test"
compare = os.path.join(_SAMPLE_FILES_DIR, f"{testname}.F90")
generate = os.path.join(_TMP_DIR, f"{testname}.F90")
# Exercise
header = "Test of long string breaking for FortranWriter"
foostr = ''.join(['0123456789']*10)
nxtchr = ord('0')
with FortranWriter(generate, 'w', header, f"{testname}") as gen:
while len(foostr) < 130:
gen.write(f"foo{len(foostr)} = '{foostr}'", 1)
foostr += chr(nxtchr)
nxtchr += 1
if nxtchr > ord('9'):
nxtchr = ord('0')
# end if
# end while
# end with

# Check that file was generated
amsg = f"{generate} does not exist"
self.assertTrue(os.path.exists(generate), msg=amsg)
amsg = f"{generate} does not match {compare}"
self.assertTrue(filecmp.cmp(generate, compare, shallow=False), msg=amsg)

if __name__ == "__main__":
unittest.main()