Skip to content

toolchain: lint_source.py checks for integer(wp) and signed d-exponent literals (addresses #1485) - #1725

Open
BakaOverflow wants to merge 3 commits into
MFlowCode:masterfrom
BakaOverflow:fix-issue-1485
Open

toolchain: lint_source.py checks for integer(wp) and signed d-exponent literals (addresses #1485)#1725
BakaOverflow wants to merge 3 commits into
MFlowCode:masterfrom
BakaOverflow:fix-issue-1485

Conversation

@BakaOverflow

Copy link
Copy Markdown

Description

I'm a CodePath AI301 student contributing to MFC as part of an open-source capstone project.

Why. MFC enforces several source conventions only through code review, so violations keep
landing in src/, and their failure modes are silent: a d-exponent literal such as 5.0d-11
hard-codes double precision and defeats MFC's build-time wp/stp precision switching, and
integer(wp) declares an integer with a real kind (a copy-paste slip). lint_source.py
had too narrow a regex for the first and no check for the second, so neither was caught.

What. This PR covers two of the three gaps in #1485 and fixes the violations they surface:

Linter (toolchain/mfc/lint_source.py):

  1. Broadened check_double_precision's d-literal regex from [0-9]d0 to catch signed /
    multi-digit exponents (5.0d-11, 2.5d+3, 1.0d12). I constrained it with identifier
    boundaries — (?<![A-Za-z0-9_])[0-9]\.?[0-9]*[dD][-+]?[0-9]+(?![A-Za-z0-9_]) — because the
    bare pattern in the issue also matches the 2d12 inside identifiers like cart2d12_coords.
  2. Added check_integer_wp — flags integer(wp), suggests plain integer.

Both registered in main(), with unit tests in test_lint_source.py.

Source fixes:

  • m_bubbles_EE.fpp: integer(wp)integer
  • m_bubbles_EL.fpp: 5.0d-115.0e-11_wp

Deferred: the third gap — check_stop_statements — is intentionally left out of this PR. The
existing stops can't simply become s_mpi_abort (that routine is in m_mpi_common, which
already uses the modules the stops live in, so the import would be a circular dependency),
which is why the stop cleanup is tracked in #1483. I'll add the stop check in a follow-up
paired with #1483 so the check and the fixes land together.

@sbryngelson — two things I wanted to check with you. First, I tightened the d-literal regex from
the exact pattern in the issue by adding identifier boundaries, because the bare version also
matches names like cart2d12_coords; I believe that only removes false positives, but wanted to
flag the deviation. Second, I scoped this PR to the integer(wp) and d-literal checks and left the
stop/error stop check for a follow-up alongside #1483 — fixing those stops cleanly runs into
the module-dependency cycle that issue already owns. Happy to pull the stop check into this PR
instead if you'd prefer.

Addresses #1485. (Delivers two of the three requested checks; the stop/error stop check is deferred to #1483 — see "Deferred" above.)

Type of change

  • New feature (linter checks)
  • Refactor (source convention fixes)

Testing

Local (rebased branch, gfortran 13.3.0, CPU):

  • python3 -m pytest toolchain/368 passed (8 warnings, 4 subtests passed).
  • python3 toolchain/mfc/lint_source.py → exit 0 (clean tree).
  • ./mfc.sh precheck → all 7 local gates pass.
  • ./mfc.sh build → exit 0 (all four targets).

Full CI — intra-fork dry-run on the narrowed commit (62a16ff2): 60 checks passed, 1 failed.

  • ✅ CPU test matrix green — every NVHPC-cpu and Github (ubuntu/macos, GNU/Intel) job passed,
    Lagrange bubble cases included (the family an earlier full-scope attempt broke, now clean).
  • Coverage Test on CodeCov green; ✅ NVHPC GPU jobs green; ✅ lint / spell / convergence /
    FP-stability all green.
  • ❌ Only red: Build & Verify, at its Linkcheck - Lychee step — a [403] on a StackOverflow URL
    in docs/documentation/docker.md, a file this PR does not touch and which is unchanged on
    master (pre-existing; not caused by this PR).

Before / after — the gap this PR closes (verbatim linter output):

# BEFORE: on master, the linter reports success despite the violations
$ python3 toolchain/mfc/lint_source.py; echo exit=$?
exit=0

# WITH THE NEW CHECKS (this PR), before fixing the sources — they are caught:
$ python3 toolchain/mfc/lint_source.py
Source lint failed:
  src/simulation/m_bubbles_EL.fpp:1799 double-precision intrinsic '5.0d-11'. Fix: use generic intrinsics and wp/stp kind parameters
  src/simulation/m_bubbles_EE.fpp:73 'integer(wp)' uses a floating-point kind. Fix: use plain 'integer'

# AFTER: sources fixed in this PR, tree is clean again
$ python3 toolchain/mfc/lint_source.py; echo exit=$?
exit=0

Checklist

  • I added or updated tests for new behavior
  • I updated documentation if user-facing behavior changed (no user-facing behavior changed)
GPU changes

integer-kind + literal-form changes only; no GPU logic; flagging for the CI matrix.

@BakaOverflow

Copy link
Copy Markdown
Author

@claude full review

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.21%. Comparing base (a4a5d3c) to head (90d738d).

Files with missing lines Patch % Lines
src/simulation/m_bubbles_EL.fpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1725   +/-   ##
=======================================
  Coverage   61.21%   61.21%           
=======================================
  Files          84       84           
  Lines       21603    21603           
  Branches     3189     3189           
=======================================
  Hits        13224    13224           
  Misses       6209     6209           
  Partials     2170     2170           

☔ View full report in Codecov by Harness.
📢 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.

@sbryngelson

Copy link
Copy Markdown
Member

Thanks for this, and for flagging the two deviations explicitly in the description. Both calls were right; see the end of this comment.

I verified the behavior rather than just reading the regexes. On current master, the old [0-9]d0 pattern gets 0 hits across src/, and the new pattern gets exactly 1, the 5.0d-11 you fixed, with no false positives anywhere in the tree. Both source fixes are correct: 5.0e-11_wp is representable in single precision, and the integer(wp) -> integer counters in s_comp_alpha_from_n only span grid extents, so the int64 to int32 narrowing in double builds does not change behavior.

Two things to fix before this merges.

1. The mantissa only accepts one digit before the decimal point

toolchain/mfc/lint_source.py:197

The d-literal alternative is [0-9]\.?[0-9]*[dD][-+]?[0-9]+. Only a single [0-9] is allowed ahead of the optional decimal point, so any literal spelled <multi-digit>.d<exp> is missed. Match attempts at interior digits are blocked by the (?<![A-Za-z0-9_]) lookbehind, and the pattern cannot start at the . itself.

input current regex proposed regex
p0 = 101325.d0 no match 101325.d0
x = 100.d0 no match 100.d0
x = 1013.d3 no match 1013.d3
x = 12.d-3 no match 12.d-3
p0 = 101325.0d0 '0d0' 101325.0d0
x = 1013.25d3 '25d3' 1013.25d3
x = 5.0d-11 5.0d-11 5.0d-11
x = .5d0 '5d0' .5d0
cart2d12_coords no match no match

There are two distinct defects here.

The first is a false negative. 101325.d0 is an ordinary way to write a hardcoded double, and it still passes precheck and CI silently. That is the exact failure mode this PR sets out to close.

The second is a misleading diagnostic. When the mantissa has several digits before the point, the match starts mid-number, so the error quotes a fragment. 101325.0d0 is reported as double-precision intrinsic '0d0', and 1013.25d3 as '25d3'. Neither string exists as a token in the file, so grepping for the reported text finds nothing.

Both are fixed by allowing a full mantissa and adding . to the boundaries:

r"(?<![A-Za-z0-9_.])(?:[0-9]+\.?[0-9]*|\.[0-9]+)[dD][-+]?[0-9]+(?![A-Za-z0-9_.])"

I ran this variant over the whole src/ tree: still exactly one hit, m_bubbles_EL.fpp:1798, still rejects cart2d12_coords, still accepts .5d0, 1.0d0, and 5.0d-11.

Adding . to the trailing boundary also closes a latent false positive in the current version. write(*,'(1D12.4)') x matches as 1D12 today, because a D edit descriptor with a repeat count looks exactly like a d-literal. No format string in src/ uses a D descriptor right now, so this is not reachable, but the linter is a blocking gate and a future formatted write would fail lint for no reason. The proposed regex does not match it.

Please add 101325.d0 to test_double_precision_flags_signed_d_exponent.

2. check_integer_wp misses integer(kind=wp)

toolchain/mfc/lint_source.py:281

\binteger\s*\(\s*wp\s*\) does not match the explicit-keyword spelling. That is the identical bug, an integer declared with a real kind, and it is the likelier copy-paste slip, since it mirrors real(kind=wp), which appears 75 times in src/. A contributor writing integer(kind=wp) :: i, j, k, l reintroduces exactly what this check exists to prevent, and the linter stays green.

r"\binteger\s*\(\s*(?:kind\s*=\s*)?wp\s*\)"

I confirmed this leaves the legitimate integer(KIND=MPI_OFFSET_KIND) declarations in m_data_input.f90 and m_data_output.fpp alone. Please add a test case for the kind= spelling.

On your two questions

The identifier-boundary tightening is strictly an improvement, so keep it. I confirmed the bare pattern from #1485 matches inside cart2d12_coords, which is a real identifier in this codebase, so the untightened version would have produced false positives on day one. test_double_precision_clean_cases pins that regression correctly.

Deferring the stop check to a follow-up alongside #1483 is the right call. The dependency reasoning checks out: m_mpi_common already uses the modules the stops live in, so the import would cycle. Landing the check and the fixes together in that issue is cleaner than splitting them.

Otherwise this is in good shape. The test additions are well targeted, and the _write_proxy to _write_src refactor is a clean generalization that does not disturb the existing broadcast tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants