From 943512539b04b80cbc3e008e8b9858a08831abf6 Mon Sep 17 00:00:00 2001 From: Haipeng Lin Date: Wed, 22 Jul 2026 21:05:41 -0400 Subject: [PATCH] Fix local name collisions between group cap dummy arguments and locals Two schemes in the same suite group using the same Fortran local name for different standard names generated an uncompilable group cap: a constituent dummy argument keeps the local name of the scheme that first requested it, while a scheme-supplied interstitial declares a group local with its own scheme local name. The Group call list and the Group dictionary each enforced local name uniqueness internally (#631) but never against each other, so the group subroutine declared the same name twice. The same collision occurred between a host variable dummy argument and an interstitial local. Fixes: - After a Group's analysis, rename any group-local variable whose local name matches a dummy argument (call list) or suite module variable (Group.ensure_unique_local_names). Renaming group locals is safe because generated code resolves variables by standard name at write time and locals are invisible outside the group subroutine. - VarDictionary.new_internal_variable_name accepts a reserved collection of additional names to avoid and compares candidate names case insensitively (local name use is case insensitive). - Add VarDictionary.known_local_names accessor. - Fix a latent KeyError in Group.write when a duplicate local name was first registered as a scalar or optional variable rather than an allocatable. Test: the advection test gains cld_shadow, a scheme reusing the cloud ice constituent local name (cld_ice_array) and the host horizontal dimension local name (ncols) for its own interstitials; the generated group cap must rename its locals for the test to compile and run. Seen in CAM-SIMA cam7: nucleate_ice_ccpp's constituent 'ni' collided with the gravity wave drag schemes' interstitial 'ni' (worked around in atmospheric_physics by renaming the scheme argument). Assisted-by: claude-fable:5 --- scripts/metavar.py | 20 ++++++-- scripts/suite_objects.py | 48 ++++++++++++++++++- test/advection_test/CMakeLists.txt | 2 +- test/advection_test/README.md | 3 ++ test/advection_test/advection_test_reports.py | 3 +- test/advection_test/cld_shadow.F90 | 41 ++++++++++++++++ test/advection_test/cld_shadow.meta | 46 ++++++++++++++++++ test/advection_test/cld_suite.xml | 1 + 8 files changed, 157 insertions(+), 7 deletions(-) create mode 100644 test/advection_test/cld_shadow.F90 create mode 100644 test/advection_test/cld_shadow.meta diff --git a/scripts/metavar.py b/scripts/metavar.py index cafdbf9f..b64e043f 100755 --- a/scripts/metavar.py +++ b/scripts/metavar.py @@ -1863,6 +1863,12 @@ def find_local_name(self, local_name, any_scope=False): # end if return pvar + def known_local_names(self): + """Return a list of the local names in use in this dictionary. + The returned names are lowercase because local name use is + case insensitive.""" + return list(self.__local_names.keys()) + def find_error_variables(self, any_scope=False, clone_as_out=False): """Find and return a consistent set of error variables in this dictionary. @@ -2098,11 +2104,14 @@ def var_call_string(self, var, loop_vars=None): """ return var.call_string(self, loop_vars=loop_vars) - def new_internal_variable_name(self, prefix=None, max_len=63): + def new_internal_variable_name(self, prefix=None, max_len=63, + reserved=None): """Find a new local variable name for this dictionary. The new name begins with _ or with (where is this VarDictionary's name) if is None. The new variable name is kept to a maximum length of . + If is passed, it is a collection of local names (e.g., + from a related dictionary) which the new name must also avoid. """ index = 0 if prefix is None: @@ -2110,7 +2119,12 @@ def new_internal_variable_name(self, prefix=None, max_len=63): else: var_prefix = '{}'.format(prefix) # end if - varlist = [x for x in self.__local_names.keys() if var_prefix in x] + # Local names are used case insensitively, compare in lowercase + varlist = [x for x in self.__local_names.keys() + if var_prefix.lower() in x] + if reserved: + varlist.extend([x.lower() for x in reserved]) + # end if newvar = None while newvar is None: if index == 0: @@ -2122,7 +2136,7 @@ def new_internal_variable_name(self, prefix=None, max_len=63): if len(newvar) > max_len: var_prefix = var_prefix[:-1] newvar = None - elif newvar in varlist: + elif newvar.lower() in varlist: newvar = None # end if # end while diff --git a/scripts/suite_objects.py b/scripts/suite_objects.py index 3c846ed5..fd5d9bf5 100755 --- a/scripts/suite_objects.py +++ b/scripts/suite_objects.py @@ -1632,6 +1632,38 @@ def manage_variable(self, newvar): raise CCPPError(emsg) # end if + def ensure_unique_local_names(self): + """Rename any group-local variable (this Group's dictionary) whose + local name is also the local name of one of this Group's dummy + arguments (call list) or of a suite module variable. + The group subroutine declares its dummy arguments and its local + variables in the same scope, so a shared name is a Fortran error + (e.g., a constituent dummy argument that keeps the local name of the + scheme which first requested it while another scheme uses the same + local name for a scheme-supplied interstitial). A group local would + also shadow a same-named suite module variable, silently redirecting + any reference to that suite variable in this group's subroutine. + Renaming a group local is safe because generated code looks + variables up by standard name at write time and group locals are + invisible outside the group subroutine.""" + reserved = self.call_list.known_local_names() + reserved.extend(self.parent.known_local_names()) + rset = set(reserved) + for var in list(self.variable_list()): + lname = var.get_prop_value('local_name') + if lname.lower() in rset: + stdname = var.get_prop_value('standard_name') + new_lname = self.new_internal_variable_name(prefix=lname, + reserved=reserved) + self.remove_variable(stdname) + # Add directly to this Group's dictionary (bypassing the + # SuiteObject dummy-argument adjustments already applied to + # the original variable) + VarDictionary.add_variable(self, var.clone(new_lname), + self.run_env, exists_ok=True) + # end if + # end for + def analyze(self, phase, suite_vars, scheme_library, ddt_library, check_suite_state, set_suite_state): """Analyze the Group's interface to prepare for writing""" @@ -1653,6 +1685,9 @@ def analyze(self, phase, suite_vars, scheme_library, ddt_library, # end for self._phase_check_stmts = check_suite_state self._set_state = set_suite_state + # All of this Group's dummy arguments and local variables are now + # known; resolve any local name collisions between them + self.ensure_unique_local_names() if (self.run_env.logger and self.run_env.logger.isEnabledFor(logging.DEBUG)): self.run_env.logger.debug("{}".format(self)) @@ -1736,8 +1771,17 @@ def write(self, outfile, host_arglist, indent, const_mod, for var in item.declarations(): lname = var.get_prop_value('local_name') sname = var.get_prop_value('standard_name') - if (lname in subpart_allocate_vars) or (lname in subpart_optional_vars) or (lname in subpart_scalar_vars): - if subpart_allocate_vars[lname][0].compatible(var, self.run_env): + if lname in subpart_allocate_vars: + prev_var = subpart_allocate_vars[lname][0] + elif lname in subpart_optional_vars: + prev_var = subpart_optional_vars[lname][0] + elif lname in subpart_scalar_vars: + prev_var = subpart_scalar_vars[lname][0] + else: + prev_var = None + # end if + if prev_var is not None: + if prev_var.compatible(var, self.run_env): pass # We already are going to declare this variable else: errmsg = "Duplicate Group variable, {}" diff --git a/test/advection_test/CMakeLists.txt b/test/advection_test/CMakeLists.txt index 4c20835b..f21cdcc6 100644 --- a/test/advection_test/CMakeLists.txt +++ b/test/advection_test/CMakeLists.txt @@ -1,5 +1,5 @@ # Create list of SCHEME_FILES, HOST_FILES, and SUITE_FILES -set(SCHEME_FILES "cld_liq" "cld_ice" "apply_constituent_tendencies" "const_indices") +set(SCHEME_FILES "cld_liq" "cld_ice" "apply_constituent_tendencies" "const_indices" "cld_shadow") set(SCHEME_FILES_ERROR "cld_liq" "cld_ice" "dlc_liq") set(HOST_FILES "test_host_data" "test_host_mod") set(SUITE_FILES "cld_suite.xml") diff --git a/test/advection_test/README.md b/test/advection_test/README.md index 6dd53e9f..01743dba 100644 --- a/test/advection_test/README.md +++ b/test/advection_test/README.md @@ -8,6 +8,9 @@ Contains tests to exercise the capabilities of the constituents object, includin - Accessing and modifying a constituent tendency variable - Passing around the constituent tendency array - Dimensions are case-insensitive +- Reuse of a constituent or host variable local name by a scheme + interstitial with a different standard name (cld_shadow); the group cap + must rename its local variables to avoid duplicate declarations ## Building/Running diff --git a/test/advection_test/advection_test_reports.py b/test/advection_test/advection_test_reports.py index 4fbe8e68..16bafca8 100644 --- a/test/advection_test/advection_test_reports.py +++ b/test/advection_test/advection_test_reports.py @@ -35,7 +35,8 @@ _CCPP_FILES = _UTILITY_FILES + _HOST_FILES + _SUITE_FILES _DEPENDENCIES = [""] _PROCESS_LIST = [""] -_MODULE_LIST = ["cld_ice", "cld_liq", "const_indices", "apply_constituent_tendencies"] +_MODULE_LIST = ["cld_ice", "cld_liq", "cld_shadow", "const_indices", + "apply_constituent_tendencies"] _SUITE_LIST = ["cld_suite"] _REQUIRED_VARS_CLD = ["ccpp_error_code", "ccpp_error_message", "horizontal_loop_begin", "horizontal_loop_end", diff --git a/test/advection_test/cld_shadow.F90 b/test/advection_test/cld_shadow.F90 new file mode 100644 index 00000000..c1ac438a --- /dev/null +++ b/test/advection_test/cld_shadow.F90 @@ -0,0 +1,41 @@ +! Test parameterization whose local names collide with other identifiers +! in the group cap: is also the local name of the cloud +! ice constituent (see cld_ice) and is also the local name of the +! host model horizontal dimension. Both name a scheme-supplied +! interstitial here, so capgen must rename the group-cap locals. +! + +module cld_shadow + + use ccpp_kinds, only: kind_phys + + implicit none + private + + public :: cld_shadow_run + +contains + + !> \section arg_table_cld_shadow_run Argument Table + !! \htmlinclude arg_table_cld_shadow_run.html + !! + subroutine cld_shadow_run(ncol, timestep, cld_ice_array, ncols, & + errmsg, errflg) + + integer, intent(in) :: ncol + real(kind_phys), intent(in) :: timestep + real(kind_phys), intent(out) :: cld_ice_array(:,:) + real(kind_phys), intent(out) :: ncols(:) + character(len=512), intent(out) :: errmsg + integer, intent(out) :: errflg + !---------------------------------------------------------------- + + errmsg = '' + errflg = 0 + + cld_ice_array(:ncol,:) = timestep + ncols(:ncol) = real(ncol, kind_phys) + + end subroutine cld_shadow_run + +end module cld_shadow diff --git a/test/advection_test/cld_shadow.meta b/test/advection_test/cld_shadow.meta new file mode 100644 index 00000000..2df6d28f --- /dev/null +++ b/test/advection_test/cld_shadow.meta @@ -0,0 +1,46 @@ +[ccpp-table-properties] + name = cld_shadow + type = scheme +[ccpp-arg-table] + name = cld_shadow_run + type = scheme +[ ncol ] + standard_name = horizontal_loop_extent + type = integer + units = count + dimensions = () + intent = in +[ timestep ] + standard_name = time_step_for_physics + long_name = time step + units = s + dimensions = () + type = real | kind = kind_phys + intent = in +[ cld_ice_array ] + standard_name = cld_shadow_scratch_array + units = s + type = real | kind = kind_phys + dimensions = (horizontal_loop_extent, vertical_layer_dimension) + intent = out +[ ncols ] + standard_name = cld_shadow_column_scratch + units = count + type = real | kind = kind_phys + dimensions = (horizontal_loop_extent) + intent = out +[ errmsg ] + standard_name = ccpp_error_message + long_name = Error message for error handling in CCPP + units = none + dimensions = () + type = character + kind = len=512 + intent = out +[ errflg ] + standard_name = ccpp_error_code + long_name = Error flag for error handling in CCPP + units = 1 + dimensions = () + type = integer + intent = out diff --git a/test/advection_test/cld_suite.xml b/test/advection_test/cld_suite.xml index fac613e8..d5760416 100644 --- a/test/advection_test/cld_suite.xml +++ b/test/advection_test/cld_suite.xml @@ -7,5 +7,6 @@ apply_constituent_tendencies cld_ice apply_constituent_tendencies + cld_shadow