Check Fortran imports to locate unknown DDT by gold2718 · Pull Request #684 · NCAR/ccpp-framework

gold2718 · 2025-09-15T14:55:47Z

Added UseStatement type, test for unknown DDT
Add space between --verbose inputs
Check for imported type before raising an invalid Fortran type error

User interface changes?: No

Testing:

unit tests: PASS
system tests: PASS
- added inline DDT plus DDT from another file as undocumented (no metadata) member variables of vmr_type in capgen_test.

Added UseStatement type, test for unknown DDT Add space between --verbose inputs Check for imported type before raising an invalid Fortran type error

gold2718 · 2025-09-15T15:01:10Z

Alternative to #673

climbfuji · 2025-09-22T11:54:34Z

scripts/fortran_tools/parse_fortran_file.py

                    else:
                        scheme_args = list()
-                    # End if
+                    # end if


Given that the changes in this file are mostly unrelated to this PR, I am wondering if we should do one pass of the codebase to convert all these "end block" comments to lowercase in its own PR.

climbfuji · 2025-09-22T12:00:05Z

scripts/metavar.py

-            emsg = "{}: {}"
-            raise ParseSyntaxError(emsg.format(lname, cperr),
-                                   context=self.context) from cperr
+            # Raise this error unless it represents an imported DDT type


Do I understand correctly that this will allow us to import DDTs from some other module if we use

use mpi_f08, only : mpi_comm

but it will still fail if we do

use mpi_f08

?

my understanding is that

use mpi_f08

is still allowed, but if you're trying to do something like:

use ddt_library type(ty_ddt_from_ddt_library) :: ddt_from_ddt_library

you'll get a parsing error because the framework has no way of knowing if "ty_ddt_from_ddt_library" is defined within "ddt_library"

peverwhee

one small request

scripts/fortran_tools/parse_fortran.py

peverwhee · 2025-09-30T17:38:51Z

scripts/metavar.py

-            emsg = "{}: {}"
-            raise ParseSyntaxError(emsg.format(lname, cperr),
-                                   context=self.context) from cperr
+            # Raise this error unless it represents an imported DDT type


my understanding is that

use mpi_f08

is still allowed, but if you're trying to do something like:

use ddt_library type(ty_ddt_from_ddt_library) :: ddt_from_ddt_library

you'll get a parsing error because the framework has no way of knowing if "ty_ddt_from_ddt_library" is defined within "ddt_library"

climbfuji · 2025-09-30T19:09:20Z

I am wondering if a different approach altogether would be better, not just but also given the limitations that we can't use use mpi_f08 etc without listing all the symbols. Do you think the following would work?

In the metadata header for a scheme:

...
[ccpp-table-properties]
  name = mp_thompson
  type = scheme
  dependencies = machine.F,module_mp_radar.F90,module_mp_thompson.F90,module_mp_thompson_make_number_concentrations.F90
  external_types = my_type_a, my_type_b
...

gold2718 · 2025-09-30T19:53:37Z

I am wondering if a different approach altogether would be better, not just but also given the limitations that we can't use use mpi_f08 etc without listing all the symbols. Do you think the following would work?

In the metadata header for a scheme:
...
[ccpp-table-properties]
  name = mp_thompson
  type = scheme
  dependencies = machine.F,module_mp_radar.F90,module_mp_thompson.F90,module_mp_thompson_make_number_concentrations.F90
  external_types = my_type_a, my_type_b
...

This could be an additional method for models that allow naked use statements. CESM/CAM coding standards and NorESM coding standards do not allow naked use statements.

climbfuji · 2025-09-30T20:05:37Z

I am wondering if a different approach altogether would be better, not just but also given the limitations that we can't use use mpi_f08 etc without listing all the symbols. Do you think the following would work?
In the metadata header for a scheme:
...
[ccpp-table-properties]
  name = mp_thompson
  type = scheme
  dependencies = machine.F,module_mp_radar.F90,module_mp_thompson.F90,module_mp_thompson_make_number_concentrations.F90
  external_types = my_type_a, my_type_b
...
This could be an additional method for models that allow naked use statements. CESM/CAM coding standards and NorESM coding standards do not allow naked use statements.

I would be ok with requiring this, but I didn't expect CESM/CAM to be on board with that.

Some models end up with long use statements, though. Real world example:

  use mpi_f08, only : mpi_comm, mpi_comm_null, mpi_info, mpi_datatype, mpi_request, &
                      mpi_status, mpi_max_error_string, mpi_logical, mpi_character, &
                      mpi_sum, mpi_integer, mpi_integer8, mpi_byte, mpi_min, mpi_max, &
                      mpi_real, mpi_2real, mpi_minloc, mpi_maxloc, mpi_thread_funneled, &
                      mpi_comm_world, mpi_success, mpi_info_null, mpi_errors_return, &
                      mpi_datatype_null

gold2718 · 2025-09-30T20:17:47Z

I am wondering if a different approach altogether would be better, not just but also given the limitations that we can't use use mpi_f08 etc without listing all the symbols. Do you think the following would work?
In the metadata header for a scheme:
...
[ccpp-table-properties]
  name = mp_thompson
  type = scheme
  dependencies = machine.F,module_mp_radar.F90,module_mp_thompson.F90,module_mp_thompson_make_number_concentrations.F90
  external_types = my_type_a, my_type_b
...
This could be an additional method for models that allow naked use statements. CESM/CAM coding standards and NorESM coding standards do not allow naked use statements.
I would be ok with requiring this, but I didn't expect CESM/CAM to be on board with that.

Some models end up with long use statements, though. Real world example:
  use mpi_f08, only : mpi_comm, mpi_comm_null, mpi_info, mpi_datatype, mpi_request, &
                      mpi_status, mpi_max_error_string, mpi_logical, mpi_character, &
                      mpi_sum, mpi_integer, mpi_integer8, mpi_byte, mpi_min, mpi_max, &
                      mpi_real, mpi_2real, mpi_minloc, mpi_maxloc, mpi_thread_funneled, &
                      mpi_comm_world, mpi_success, mpi_info_null, mpi_errors_return, &
                      mpi_datatype_null

Some times the long statements are a pain but in CAM, another SHOULD rule (see the same link) tries to help: "use statements should be brought in at the smallest scope possible (e.g. inside individual subroutines instead of at the module level)".

climbfuji · 2025-09-30T20:19:28Z

I am wondering if a different approach altogether would be better, not just but also given the limitations that we can't use use mpi_f08 etc without listing all the symbols. Do you think the following would work?
In the metadata header for a scheme:
...
[ccpp-table-properties]
  name = mp_thompson
  type = scheme
  dependencies = machine.F,module_mp_radar.F90,module_mp_thompson.F90,module_mp_thompson_make_number_concentrations.F90
  external_types = my_type_a, my_type_b
...
This could be an additional method for models that allow naked use statements. CESM/CAM coding standards and NorESM coding standards do not allow naked use statements.
I would be ok with requiring this, but I didn't expect CESM/CAM to be on board with that.
Some models end up with long use statements, though. Real world example:
  use mpi_f08, only : mpi_comm, mpi_comm_null, mpi_info, mpi_datatype, mpi_request, &
                      mpi_status, mpi_max_error_string, mpi_logical, mpi_character, &
                      mpi_sum, mpi_integer, mpi_integer8, mpi_byte, mpi_min, mpi_max, &
                      mpi_real, mpi_2real, mpi_minloc, mpi_maxloc, mpi_thread_funneled, &
                      mpi_comm_world, mpi_success, mpi_info_null, mpi_errors_return, &
                      mpi_datatype_null
Some times the long statements are a pain but in CAM, another SHOULD rule (see the same link) tries to help: "use statements should be brought in at the smallest scope possible (e.g. inside individual subroutines instead of at the module level)".

Not possible in this case, unfortunately. Support for Fortran submodules is limited with certain compilers.

climbfuji · 2025-10-02T00:04:18Z

Coming back to this, if we decide that we require named module use statements for imported DDTs to work, then we need to update the documentation accordingly. I prefer to not add an alternative solution as I suggested previously in this case. We can do that if the need arises. @mkavulich FYI (you are our doc master)

peverwhee · 2025-10-14T17:30:25Z

@mkavulich @climbfuji can this be merged or do we need to update the documentation first?

mkavulich

I'll go ahead and merge this PR...the documentation/rule changes will need some more discussion (i.e. I'll need help from more knowledgeable people) since, from a brief review of the ccpp-physics code, there are many examples of these bare use statements, not just use mpi_f08.

Check Fortran imports to locate unknown DDT

92f4eb9

Added UseStatement type, test for unknown DDT Add space between --verbose inputs Check for imported type before raising an invalid Fortran type error

gold2718 requested review from climbfuji, dustinswales, grantfirl, mkavulich, mwaxmonsky and peverwhee as code owners September 15, 2025 14:55

gold2718 added the bugfix Fix for issue with 'bug' label. label Sep 15, 2025

Steve Goldhaber added 3 commits September 20, 2025 21:25

Fix merge conflicts

3e77301

Fix merge goof

c08fd2c

Fix test-reports tests

481b5f9

climbfuji reviewed Sep 22, 2025

View reviewed changes

peverwhee mentioned this pull request Sep 30, 2025

DRAFT: Skip variable parse checks during fortran parsing #673

Closed

peverwhee requested changes Sep 30, 2025

View reviewed changes

Add doctests from review

db8dafa

Remove repeat doctest

86d1a26

peverwhee self-requested a review October 1, 2025 17:37

peverwhee approved these changes Oct 1, 2025

View reviewed changes

Merge branch 'develop' into check_use_statements

fa79188

climbfuji approved these changes Oct 9, 2025

View reviewed changes

mkavulich approved these changes Oct 15, 2025

View reviewed changes

mkavulich merged commit 37db348 into NCAR:develop Oct 15, 2025
19 checks passed

mkavulich mentioned this pull request Oct 22, 2025

Improve parsing of Fortran array specifications #687

Merged

gold2718 deleted the check_use_statements branch December 29, 2025 12:12

Conversation

gold2718 commented Sep 15, 2025

Uh oh!

gold2718 commented Sep 15, 2025

Uh oh!

climbfuji Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

climbfuji Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

peverwhee Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

peverwhee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peverwhee Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

climbfuji commented Sep 30, 2025

Uh oh!

gold2718 commented Sep 30, 2025

Uh oh!

climbfuji commented Sep 30, 2025

Uh oh!

gold2718 commented Sep 30, 2025

Uh oh!

climbfuji commented Sep 30, 2025

Uh oh!

climbfuji commented Oct 2, 2025

Uh oh!

peverwhee commented Oct 14, 2025

Uh oh!

mkavulich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants