diff --git a/capgen/generator/group_cap.py b/capgen/generator/group_cap.py index 9dc8acea..a6e0069d 100644 --- a/capgen/generator/group_cap.py +++ b/capgen/generator/group_cap.py @@ -25,7 +25,7 @@ import os from typing import Dict, List, Optional, Set, Tuple -from metadata.parse_tools import FORTRAN_CONDITIONAL_REGEX, open_if_changed +from metadata.parse_tools import CCPPError, FORTRAN_CONDITIONAL_REGEX, open_if_changed from metadata.variable_resolver import HostVarEntry from generator.suite_types import _ptr_type_name_for_arg from generator.suite_resolver import ( @@ -1187,6 +1187,68 @@ def _generate_state_dealloc(suite_name: str, group_name: str) -> List[str]: ] +def _check_host_control_local_collisions( + suite_name: str, + group_name: str, + resolved_group: ResolvedGroup, + host_dict, +) -> None: + """Reject a host variable whose Fortran local name collides with a control variable. + + In a generated group cap, host variables are use-associated at *module* + scope while every control variable is a *subroutine argument*. When a + host variable and a control variable share a Fortran local name, the dummy + silently shadows the host import, i.e. is used instead of the host import. + Since we cannot rename the variables (they are defined in metadata), an + error is thrown when two host variables have the same local name. + + Suite-owned variables cannot collide this way -- they are always prefixed + with the suite-data DDT (``ccpp_suite_data(:)%``), and generator + locals (transformation temporaries, subcycle counters) are uniquified + separately. + """ + if host_dict is None: + return + control_by_name: Dict[str, HostVarEntry] = {} + for entry in _ctrl_entries_for_signature( + host_dict, exclude={'suite_name', 'group_name'} + ): + control_by_name[entry.local_name.lower()] = entry + if not control_by_name: + return + + def _raise(host_std: str, host_local: str) -> None: + ctrl = control_by_name[host_local.lower()] + raise CCPPError( + "Local name collision in group '{grp}' of suite '{suite}': host " + "variable '{hstd}' and control variable '{cstd}' both use the " + "Fortran local name '{ln}'. Rename the local_name " + "of one of them in the host metadata.".format( + grp=group_name, suite=suite_name, + hstd=host_std, cstd=ctrl.standard_name, ln=ctrl.local_name, + ) + ) + + for items in resolved_group.phase_calls.values(): + for call in iter_phase_calls(items): + for arg in call.args: + # Direct host argument (bare use-associated module symbol). + if (arg.source == 'host' and arg.module_name is not None + and arg.root_symbol.lower() in control_by_name): + _raise(arg.standard_name, arg.root_symbol) + # Dimension-helper and active-condition host variables are + # use-associated too, so they can shadow a control dummy. + dim_and_active = set(arg.used_dim_std_names) + dim_and_active.update(_active_std_names(arg.active)) + for std in dim_and_active: + entry = host_dict.get(std) + if (entry is not None and not entry.is_control + and entry.module_name is not None): + root = _root_symbol(entry.access_path) + if root.lower() in control_by_name: + _raise(std, root) + + def _generate_group_cap( suite_name: str, group_name: str, @@ -1209,6 +1271,12 @@ def _generate_group_cap( ------- list of str (without trailing newlines) """ + # Fail loudly on a host/control local-name collision before emitting a + # cap that would silently read the wrong variable (issue #774). + _check_host_control_local_collisions( + suite_name, group_name, resolved_group, host_dict + ) + mod_name = 'ccpp_{}_{}_{}'.format(suite_name, group_name, 'cap') # Short Fortran symbols for the state-management subroutines; the # module name carries ``ccpp___`` and keeps the mangled diff --git a/doc/followups.md b/doc/followups.md index 09ed8cf0..994523e7 100644 --- a/doc/followups.md +++ b/doc/followups.md @@ -47,6 +47,7 @@ Status values: `open`, `in progress`, `blocked`, `closed`. | FU-029 | Decide `timestep_init` / `timestep_final` phase-call-count semantics | framework | 2026-06-10 | open | For a scheme that appears multiple times in a suite, original capgen calls its `timestep_init`/`final` **once per appearance**; capgen-ng calls it **once per group** (measured cam4: `qneg_timestep_final` 2 vs 12). Benign for cam4 (the affected phases are idempotent/guarded) but a latent b4b/correctness hazard the moment such a phase is stateful (accumulates, zeroes a buffer). CCPP intent is once-per-timestep; neither matches strictly when a scheme spans groups. Decide the intended semantics and make capgen-ng's behaviour intentional + documented. Reproduce via the standalone-capgen driver, diffing `_timestep_(init|final)` call counts. | | FU-030 | Deterministic + documented constituent registration order in the generator | framework | 2026-06-11 | open | Root cause of the cam4 FWAUT b4b diff (the framework side of FU-018): capgen-ng registers water species alphabetically ([cloud_ice, cloud_liquid, water_vapor]) vs original's declaration order ([cloud_liquid, cloud_ice, water_vapor]), and trace gases differ too, so `air_composition`'s `thermodynamic_active_species_idx` order → `get_hydrostatic_energy` water-sum FP order → energy fixer → pervasive roundoff. Proven b4b by a flag-guarded reorder hack. **Decision (Dom): RE-BASELINE** — give capgen-ng a deterministic, documented order (qv first; an understandable rule for how constituents land in the array), then CAM-SIMA re-baselines against the original-capgen reference; not match-the-old-order. Levers: `host_constituents.py` / the legacy-auto-clone path (FU-012) / `ccpp_register_constituents` emission; intersects the constituents overhaul (FU-020). Analysis: `doc/cam4_fwaut_constituent_order.md`. | | FU-031 | Long-term redesign of the `ccpp_static_api.F90` runtime listings | framework | 2026-05-14 | open | The suite-variable / suite-host-data listings made the introspection module ~33k lines (`-O3` effectively hangs). Immediate pressure is off — `--no-host-introspection` stubs them (→ ~800 lines) — so this is **no longer blocking**, but the long-term redesign stays open for team discussion: move the listings to a runtime read of `datatable.xml` (preferred — no recompile when listings change), or a separate `-O0` file, or static string `data` tables, or lazy-emit only the routines the host calls. Do not redesign unilaterally. | +| FU-032 | Generator-owned locals can silently shadow a host import — auto-uniquify | framework | 2026-08-07 | open | `_check_host_control_local_collisions` (`capgen/generator/group_cap.py`) now hard-errors when a host variable's local name collides with a control-variable dummy (issue #774 — the silent wrong-value case, closed by that check + `unit-tests/test_suite_resolver.py::TestHostControlLocalNameCollision`). Two other subroutine-scope locals can shadow a use-associated host import the same way but are **generator-owned**, so the right fix is to rename *them*, not error: transformation temporaries (`_l` / `_p`) and subcycle loop counters (`ccpp_loop_counter*`). Seed the temp uniquifier (`used_local_names_phase`, `suite_resolver.py:2446`) with the group's host-import symbols + control-dummy names so `_local_name_conflict` renames generator locals away from them. Rare in practice (suffixed/reserved names) but closes the class. Deliberately deferred out of the #774 fix (Step 2, 2026-08-07). | --- @@ -145,6 +146,7 @@ file, per the procedure in the repository's `CLAUDE.md`. | Machine | Last reconciled | By | |---------|-----------------|-----| +| `dutchman` | 2026-08-07 | folded the issue #772 / #774 session: added FU-032 (generator-local shadow follow-up). #772 shown to be a non-issue in v1 (cld_shadow e2e reproducer) and #774 detect-and-error landed in `group_cap.py` — both tracked in GitHub, not restated here | | `dutchman` | 2026-08-06 | first sweep of this machine; folded its auto-memory investigation notes into new rows FU-025…FU-031, added Codee `use…only:` detail to FU-005, cross-linked FU-018↔FU-030 | | `ip-10-0-0-98.ec2.internal` | 2026-07-29 | swept on adding FU-024; local stores unchanged since the previous sweep, nothing new to fold in | | `ip-10-0-0-98.ec2.internal` | 2026-07-28 | initial migration — merged `migration.md` §8, `briefing.md` §7.1, `redesign_prompt.md` "Still deferred", plus open items from this machine's auto-memory | diff --git a/end-to-end-tests/advection/CMakeLists.txt b/end-to-end-tests/advection/CMakeLists.txt index 2b69dda0..f3108146 100644 --- a/end-to-end-tests/advection/CMakeLists.txt +++ b/end-to-end-tests/advection/CMakeLists.txt @@ -5,7 +5,7 @@ # #------------------------------------------------------------------------------ -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(HOST_FILES "test_host_data" "test_host_mod" "test_host") set(SUITE_FILES "cld_suite.xml") set(HOST "test_host") diff --git a/end-to-end-tests/advection/README.md b/end-to-end-tests/advection/README.md index c460e13a..a7fd8d01 100644 --- a/end-to-end-tests/advection/README.md +++ b/end-to-end-tests/advection/README.md @@ -8,3 +8,8 @@ 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); capgen v1 + routes each interstitial through the suite-data DDT + (`ccpp_suite_data(:)%...`), so the group cap compiles and runs without + any local-name collision (see GitHub issues #772 / #774) diff --git a/end-to-end-tests/advection/cld_shadow.F90 b/end-to-end-tests/advection/cld_shadow.F90 new file mode 100644 index 00000000..c1ac438a --- /dev/null +++ b/end-to-end-tests/advection/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/end-to-end-tests/advection/cld_shadow.meta b/end-to-end-tests/advection/cld_shadow.meta new file mode 100644 index 00000000..ffbe0a81 --- /dev/null +++ b/end-to-end-tests/advection/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_dimension + 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_dimension, vertical_layer_dimension) + intent = out +[ ncols ] + standard_name = cld_shadow_column_scratch + units = count + type = real | kind = kind_phys + dimensions = (horizontal_dimension) + 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/end-to-end-tests/advection/cld_suite.xml b/end-to-end-tests/advection/cld_suite.xml index fac613e8..d5760416 100644 --- a/end-to-end-tests/advection/cld_suite.xml +++ b/end-to-end-tests/advection/cld_suite.xml @@ -7,5 +7,6 @@ apply_constituent_tendencies cld_ice apply_constituent_tendencies + cld_shadow diff --git a/unit-tests/test_suite_resolver.py b/unit-tests/test_suite_resolver.py index 8a506d3a..e648b4fa 100644 --- a/unit-tests/test_suite_resolver.py +++ b/unit-tests/test_suite_resolver.py @@ -5477,6 +5477,110 @@ def test_range_dimension_token_checked(self): validate_init_dimensions(sr) +######################################################################## +# Host/control local-name collision detection (GitHub issue #774). +# A host variable and a control variable that share a Fortran local name +# (with different standard names) must be rejected: in the group cap the +# host variable is use-associated at module scope while the control +# variable is a subroutine dummy argument that silently shadows it, so a +# scheme requesting the host variable would receive the control value. +######################################################################## + +_SHADOW_HOST = ''' +[ccpp-table-properties] + name = shadow_host + type = host +[ccpp-arg-table] + name = shadow_host + type = host +[ nthreads ] + standard_name = test_host_thread_count + long_name = a host quantity that happens to share the thread-count local name + units = count + type = integer + dimensions = () +''' + +# Same host table, but the colliding variable is renamed so there is no +# clash with the control ``nthreads`` (number_of_threads). +_NOSHADOW_HOST = _SHADOW_HOST.replace('[ nthreads ]', '[ nthr_host ]') + +_SHADOW_SCHEME = ''' +[ccpp-table-properties] + name = thr_use + type = scheme +[ccpp-arg-table] + name = thr_use_run + type = scheme +[ ni ] + standard_name = test_host_thread_count + units = count + type = integer + dimensions = () + intent = in +[ errmsg ] + standard_name = ccpp_error_message + units = none + dimensions = () + type = character + kind = len=512 + intent = out +[ errflg ] + standard_name = ccpp_error_code + units = 1 + dimensions = () + type = integer + intent = out +''' + +_SHADOW_SUITE = ( + '\n' + '\n' + ' \n' + ' thr_use\n' + ' \n' + '\n' +) + + +class TestHostControlLocalNameCollision(unittest.TestCase): + """A host var and a control var sharing a Fortran local name (different + standard names) must be rejected before a cap that silently reads the + wrong variable is written (GitHub issue #774). ``control_full.meta`` + already declares ``number_of_threads`` with local name ``nthreads``.""" + + def _resolve(self, host_src): + import logging + from generator.suite_xml import parse_suite_xml + host_tbls = _parse(host_src) + ctrl_tbls = parse_metadata_file(_sf('control_full.meta')) + hd = build_flat_host_dict(host_tbls, ctrl_tbls, []) + store = SchemeStore.build_from(_parse(_SHADOW_SCHEME)) + with tempfile.TemporaryDirectory() as tmp: + sx = os.path.join(tmp, 'suite_shadow.xml') + with open(sx, 'w') as fh: + fh.write(_SHADOW_SUITE) + suite = parse_suite_xml(sx, tmp, logging.getLogger('test'), + skip_validation=True) + return resolve_suite(suite, store, hd), hd + + def test_collision_raises(self): + sr, hd = self._resolve(_SHADOW_HOST) + with self.assertRaises(CCPPError) as cm: + _generate_group_cap('shadow', 'phys', sr.groups[0], hd) + msg = str(cm.exception) + self.assertIn('test_host_thread_count', msg) + self.assertIn('number_of_threads', msg) + self.assertIn('nthreads', msg) + + def test_no_collision_when_local_names_differ(self): + # Same host quantity, but its local name no longer matches the + # control ``nthreads`` -- generation must succeed. + sr, hd = self._resolve(_NOSHADOW_HOST) + lines = _generate_group_cap('shadow', 'phys', sr.groups[0], hd) + self.assertTrue(any('thr_use_run' in ln for ln in lines)) + + def load_tests(loader, tests, ignore): import generator.suite_resolver as suite_resolution import generator.group_cap as gc