Add Fortran/Fypp static analysis linter

[sbryngelson]

Summary

Adds a new Python-based static analysis linter (toolchain/mfc/lint_source.py) that catches patterns known to cause silent bugs in Fortran/Fypp source code. Integrated into both the local precheck hook and CI.

Checks

Check	What it catches	Example
Fypp list duplicates	Duplicate entries in #:for VAR in [...] lists	bc_x%ve2 listed twice in MPI broadcast, silently skipping bc_x%ve3
Duplicate consecutive lines	Identical adjacent non-trivial lines (≥40 chars)	Doubled R3bar accumulation, repeated subroutine argument
Hardcoded byte size	int(8._wp, ...) patterns that assume 8-byte reals	WP_MOK = int(8._wp, MPI_OFFSET_KIND) breaks in single precision

All three checks are motivated by real bugs found in the codebase.

Integration

• toolchain/bootstrap/precheck.sh: Added as step 5/6 (bumped doc check to 6/6), runs during ./mfc.sh precheck and the pre-commit hook
• .github/workflows/lint-source.yml: Added as a new CI step alongside existing grep-based checks
• CMakeLists.txt: Added -Wconversion to GNU debug builds, tracked in cleanliness CI

Dependencies

This PR will fail precheck/CI until the following PRs merge first:

• Fix WP_MOK hardcoded to 8 bytes, use storage_size for portability #1187 — Fixes WP_MOK hardcoded byte size (10 sites) and dq_prim_dy_vf→dq_prim_dz_vf duplicate line bug
• Fix 6 low-risk pre-process bugs (batch) #1241 — Fixes duplicate R3bar line in m_assign_variables.fpp

The source bug fixes were intentionally moved out of this PR and into their respective dedicated fix PRs. This PR now contains only the linter infrastructure. Once #1187 and #1241 are merged, this PR will pass cleanly.

Test plan

• Merge Fix WP_MOK hardcoded to 8 bytes, use storage_size for portability #1187 and Fix 6 low-risk pre-process bugs (batch) #1241 first
• Run python3 toolchain/mfc/lint_source.py — verify clean (no issues)
• Run ./mfc.sh precheck — verify Fortran/Fypp analysis appears as step 5/6
• Verify no false positives (all findings correspond to real bugs)

🤖 Generated with Claude Code

Closes #1211

[codeant-ai]

Nitpicks 🔍

🔒 No security issues identified

⚡ Recommended areas for review File I/O robustness The code calls Path.read_text(encoding="utf-8") without handling decoding errors. A non-UTF-8 file (or an unexpected byte sequence) will raise UnicodeDecodeError and crash the linter (causing precheck/CI to fail). Consider defensively handling read errors or falling back to a safe decoding strategy.

[Copilot]

Pull request overview

This PR adds a Python-based static analysis linter (lint_source.py) to detect common Fortran/Fypp code issues that cause silent bugs. The linter checks for duplicate entries in Fypp loop lists, duplicate consecutive source lines, and hardcoded byte size assumptions that break single-precision builds.

Changes:

• Added toolchain/mfc/lint_source.py with three static analysis checks for Fortran/Fypp code
• Integrated the linter into the precheck hook as step 5/6
• Added CI workflow step to run Fortran/Fypp static analysis

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
toolchain/mfc/lint_source.py	New static analysis linter with checks for Fypp list duplicates, duplicate consecutive lines, and hardcoded byte sizes
toolchain/bootstrap/precheck.sh	Updated step numbering to 6 total steps and added Fortran/Fypp analysis as step 5
.github/workflows/lint-source.yml	Added new CI step to run the static analysis linter

[cubic-dev-ai]

1 issue found across 3 files

Confidence score: 5/5

• Overall low risk since the only issue is a low-severity linting edge case, not a runtime or user-facing change.
• Most notable concern: toolchain/mfc/lint_source.py only checks quoted list entries, so duplicate non-string items in #:for ... in [...] lists can slip through, reducing lint coverage.
• Pay close attention to toolchain/mfc/lint_source.py - duplicate detection ignores non-string list items.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="toolchain/mfc/lint_source.py">

<violation number="1" location="toolchain/mfc/lint_source.py:62">
P2: This only checks quoted list entries, so duplicate numeric/tuple items in `#:for ... in [...]` lists (e.g., `[1, 2, 3]`) are silently ignored. The linter should parse the full list so duplicates in non-string entries are also detected.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

toolchain/mfc/lint_source.py (1)

42-43: _check_single_fypp_list — quote-matching regex allows mixed delimiters.

r"['\"]([^'\"]*)['\"]" matches any opening quote paired with any closing quote, so 'foo" is a valid match. In the MFC Fypp codebase, list entries are uniformly 'single-quoted', so no false positives have been seen. Consider using two separate patterns or a back-reference to enforce consistent quoting:

♻️ Proposed fix using alternation with back-reference

-    entries = re.findall(r"['\"]([^'\"]*)['\"]", list_content)
+    entries = re.findall(r"'([^']*)'|\"([^\"]*)\"", list_content)
+    entries = [a or b for a, b in entries]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@toolchain/mfc/lint_source.py` around lines 42 - 43, The regex in
_check_single_fypp_list that builds entries using re.findall currently allows
mismatched opening/closing quotes (e.g., "'foo\""); update the pattern used to
extract list_content entries to enforce matching delimiters — either restrict to
single quotes (since Fypp uses single-quoted entries) or use a
back-reference/alternation so the opening quote is the same as the closing
quote; modify the entries assignment (the variable entries derived from
list_content in _check_single_fypp_list) to use the corrected regex and ensure
subsequent logic that iterates over entries continues to work with the new
extraction.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@toolchain/mfc/lint_source.py`:
- Around line 126-152: The check_hardcoded_byte_size function is flagging
patterns inside inline Fortran comments because byte_re is run against the whole
stripped line; to fix, before testing byte_re.search, remove the inline comment
portion by taking the substring up to the first unquoted '!' (i.e., split on '!'
and use the left part) so only actual code is scanned; update the loop in
check_hardcoded_byte_size to compute a code_only variable from line (respecting
full-line comments via _is_comment_or_blank) and run byte_re.search on that
code_only string (references: check_hardcoded_byte_size, _is_comment_or_blank,
byte_re, lines loop).

---

Nitpick comments:
In `@toolchain/mfc/lint_source.py`:
- Around line 42-43: The regex in _check_single_fypp_list that builds entries
using re.findall currently allows mismatched opening/closing quotes (e.g.,
"'foo\""); update the pattern used to extract list_content entries to enforce
matching delimiters — either restrict to single quotes (since Fypp uses
single-quoted entries) or use a back-reference/alternation so the opening quote
is the same as the closing quote; modify the entries assignment (the variable
entries derived from list_content in _check_single_fypp_list) to use the
corrected regex and ensure subsequent logic that iterates over entries continues
to work with the new extraction.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.04%. Comparing base (ab5082e) to head (bd96007).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1193   +/-   ##
=======================================
  Coverage   44.04%   44.04%           
=======================================
  Files          70       70           
  Lines       20499    20499           
  Branches     1993     1993           
=======================================
  Hits         9028     9028           
  Misses      10330    10330           
  Partials     1141     1141           

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

[cubic-dev-ai]

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="toolchain/mfc/lint_source.py">

<violation number="1" location="toolchain/mfc/lint_source.py:145">
P2: The inline comment stripping uses `split("!")`, which also cuts `!` inside string literals. That can hide real `int(8._wp, ...)` usages on lines that include a string with `!`, leading to false negatives. Use a comment-stripping approach that ignores `!` inside quotes.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[github-actions]

Claude Code Review

Head SHA: bd96007
Files changed: 7

• .claude/rules/common-pitfalls.md
• .github/workflows/cleanliness.yml
• .github/workflows/lint-source.yml
• CLAUDE.md
• CMakeLists.txt
• toolchain/bootstrap/precheck.sh
• toolchain/mfc/lint_source.py (new)

---

Summary

• Adds a Python static-analysis linter (lint_source.py) that catches three real classes of silent bugs: Fypp list duplicates, duplicate consecutive source lines, and hardcoded int(8._wp, ...) byte-size assumptions.
• Integrates the linter as step 5/6 in precheck.sh and as a new CI step in lint-source.yml.
• Adds -Wconversion to GNU debug builds and tracks it in the cleanliness workflow.
• Counter and documentation updates (CLAUDE.md, common-pitfalls.md) correctly reflect the new 6-check count.

---

Findings

1. PR description–diff mismatch (toolchain/mfc/lint_source.py)
The PR body states "Companion Fortran fixes included — fixes for the 13 issues the linter finds on master today" (bc_x%ve2→ve3, dq_prim_dy→dz, WP_MOK hardcoded size, duplicate R3bar line). However, the actual diff contains no Fortran source changes — only 7 non-Fortran files. If the linter is supposed to pass precheck on this branch, either (a) those fixes are already on master, or (b) the linter CI step will fail green only because the Fortran files aren't checked out. The description should be updated to accurately reflect what is or isn't in this PR; otherwise reviewers may assume bug-fixes are present when they are not.

2. Comment-stripping heuristic (toolchain/mfc/lint_source.py:155)

if byte_re.search(stripped.split("!")[0]):

Splitting on the first ! to strip inline comments will incorrectly truncate lines where ! appears inside a string literal (e.g., call foo("msg!here")). For the specific pattern being checked (int(8._wp), this is a harmless false-negative rather than a false-positive, but it is a fragile approach. A regex-based comment stripper (skip ! inside quotes) would be more robust if this heuristic is ever reused by future checks.

3. Duplicate-line check: line.strip() vs comment-aware strip (lint_source.py:98–111)
prev_stripped and stripped include inline comments verbatim. Two lines that are logically identical but differ only in a trailing ! comment won't be flagged — this is the correct, safe behavior. However, two lines with the same trailing comment (common in generated/template-style code) will be flagged even if they are intentionally identical. The 40-char floor mitigates most noise, but teams should be aware of this if they hit false positives in repetitive initialisation blocks.

4. precheck.sh step 5 suppresses nothing on success (toolchain/bootstrap/precheck.sh:130–135)
Every other step uses > /dev/null 2>&1 to suppress output on success, then prints only the ok/error banner. The new step 5 does not suppress lint_source.py output:

if python3 toolchain/mfc/lint_source.py 2>&1; then
    ok "Fortran/Fypp analysis passed."

When the check passes the script prints nothing, so this works fine today. If lint_source.py ever gains verbose progress output, the terminal will become noisy on clean runs. Low risk now, worth noting for consistency.

---

Improvement opportunities

• Unit tests for the linter: A small pytest fixture with synthetic .fpp snippets would make it easy to verify the three checks without scanning the full src/ tree. This would also guard against regressions if thresholds (e.g., MIN_DUP_LINE_LEN) are changed.
• _check_single_fypp_list — & stripping scope: list_content.replace("&", "") strips all ampersands from the extracted list content. A continuation & at the end of a raw line segment will already be present inside the accumulated full string before the bracket is sliced out, so this is currently correct. A targeted strip (only trailing & per segment) would be more defensive if the function is extended to handle other list shapes.

[github-actions]

Claude Code Review

Head SHA: fae51a9
Files changed: 7 — .claude/rules/common-pitfalls.md, .github/workflows/cleanliness.yml, .github/workflows/lint-source.yml, CLAUDE.md, CMakeLists.txt, toolchain/bootstrap/precheck.sh, toolchain/mfc/lint_source.py

Summary

• Adds toolchain/mfc/lint_source.py — a Python static linter catching Fypp list duplicates, consecutive identical lines, and hardcoded 8-byte real size assumptions
• Integrates the linter as precheck step 5/6 (bumping doc check to 6/6) and adds it to lint-source.yml CI
• Adds -Wconversion to GNU debug builds and tracks it in cleanliness.yml
• Updates all user-facing count references (5→6 checks) in CLAUDE.md and .claude/rules/common-pitfalls.md
• PR is intentionally gated on Fix WP_MOK hardcoded to 8 bytes, use storage_size for portability #1187 and Fix 6 low-risk pre-process bugs (batch) #1241 to avoid false positives on existing bugs

---

Findings

toolchain/mfc/lint_source.py — minor issues only

1. check_duplicate_lines skips blank lines but resets prev_stripped to "" (line ~105).

   prev_stripped = ""
   for i, line in enumerate(lines):
       stripped = line.strip()
       if (stripped == prev_stripped and ...):  # blank lines compare equal to ""

   The _is_comment_or_blank guard inside the if prevents false positives for blank lines, but prev_stripped is still updated to "" for blank lines. This means that after a blank line, a long non-trivial line will never be flagged as a duplicate of the line before the blank, which is correct semantically. No bug here, but a comment clarifying the intent would help future maintainers.

2. _check_single_fypp_list: multi-line continuation accumulation strips & from content (line ~40).
   list_content.replace("&", "") strips every & from the list body, including any & that might appear legitimately inside a quoted string (e.g. a variable name like q_prim&_vf). This is extremely unlikely in practice but worth noting as a potential source of false negatives if Fypp list entries ever contain &.

3. check_hardcoded_byte_size strips inline comments with stripped.split("!")[0] (line ~138).
   This is correct for Fortran but will also strip the ! inside string literals like 'foo!bar'. Again, very unlikely to matter in practice but is a known limitation of the simple approach.

4. precheck.sh step 5 exit-code check (line ~137 of the diff):

   if python3 toolchain/mfc/lint_source.py 2>&1; then

   The 2>&1 redirect here sends stderr to stdout but the output goes to the terminal uncaptured (not to a variable or file). This is consistent with the lint_docs.py step immediately below, so it is intentional — no bug.

5. cleanliness.yml — continue-on-error: true on the new Conversion Warnings Diff step (line ~89).
   All existing warning-diff steps also use continue-on-error: true, so this is consistent. However, the summary step (which echoes counts) does not gate on conversion warnings being non-regressive. This matches the pattern for all other warning types and is presumably intentional.

---

No blocking issues found.

The linter logic is sound and motivated by real bugs. The three checks are well-scoped and unlikely to produce false positives. The precheck and CI integration is consistent with the existing pattern. The PR description honestly documents the dependency order required for a clean run.

Suggestions (non-blocking):

• Consider adding a # noqa: lint-source-dup or # lint: ok escape hatch mechanism for cases where truly identical adjacent lines are intentional (e.g. two end if statements each ≥40 chars — currently guarded by MIN_DUP_LINE_LEN=40, which is a reasonable proxy).
• The MIN_DUP_LINE_LEN = 40 constant is not explained in a comment; a brief note like # short lines (end if, end do, etc.) are common and not bugs would help.

🤖 Generated with Claude Code

[github-actions]

Claude Code Review

Head SHA: f9ad60b

Files changed: 7

• toolchain/mfc/lint_source.py (new, +171)
• toolchain/bootstrap/precheck.sh (+15/-6)
• .github/workflows/cleanliness.yml (+9)
• .github/workflows/lint-source.yml (+3)
• CMakeLists.txt (+1)
• CLAUDE.md (+2/-2)
• .claude/rules/common-pitfalls.md (+1/-1)

---

Summary

• Adds a new Python static-analysis linter (lint_source.py) with three well-motivated checks: Fypp list duplicates, duplicate consecutive lines, and hardcoded 8-byte real size.
• Cleanly integrated into both precheck.sh (step 5/6) and lint-source.yml CI.
• Adds -Wconversion to GNU debug builds and tracks it in cleanliness.yml (non-blocking diff).
• Documentation updates are consistent and accurate.

---

Findings

toolchain/mfc/lint_source.py:86 — prev_stripped not reset across blank lines

check_duplicate_lines updates prev_stripped = stripped unconditionally, including for blank and comment lines. This means a blank-line-separated duplicate pair is never caught:

    some_long_accumulation_statement_here += val

    some_long_accumulation_statement_here += val   ! ← not flagged

Given the PR motivation (duplicate R3bar accumulation lines), this is a real pattern to consider. The fix would be to only update prev_stripped from non-trivial lines:

if not _is_comment_or_blank(stripped):
    prev_stripped = stripped
else:
    prev_stripped = ""   # reset across gaps

Severity: low — the existing check still catches adjacent duplicates with no gap, which covers most copy-paste bugs.

toolchain/mfc/lint_source.py:48 — & stripping is overly broad

list_content.replace("&", "") removes all & characters from the parsed list content. If a quoted variable name ever contained a & (e.g., an operator or unusual identifier), this would silently corrupt the match. Not a real risk for current MFC variable name patterns, but re.sub(r"\s*&\s*", " ", full) on the full accumulated line (before extracting list_content) would be safer. Cosmetic / future-proof.

toolchain/mfc/lint_source.py:44 — multi-line Fypp lists without & not handled

Fypp allows line continuation via Python-style open brackets with no trailing &:

#:for VAR in [
  'a', 'b',
  'a'
]

The accumulation loop only fires when a line ends with &, so this form is silently skipped. Not a current MFC pattern but worth a comment noting the limitation.

toolchain/bootstrap/precheck.sh:131 — 2>&1 without capturing stdout

if python3 toolchain/mfc/lint_source.py 2>&1; then

This merges stderr into stdout but neither stream is redirected; the linter output always appears in the terminal regardless of pass/fail. This is consistent with the lint_docs.py step immediately below it, so it's not a regression — just noting this is intentional behavior (errors visible on failure, but also on success if any warnings were ever added).

---

Improvement Opportunities

1. Consider adding a # type: ignore or a --no-type-check note if this script is ever picked up by mypy/pyright — the list[str] return type annotations require Python 3.9+. If the CI runner uses Python 3.8 this will fail at import. A from __future__ import annotations at the top would fix this for 3.7+.

2. The check_hardcoded_byte_size error message says '8._wp' but the actual matched pattern includes int( — a more precise fix suggestion like 'int(8._wp, ...) → int(storage_size(0._stp)/8, ...)' would be more actionable.

3. The linter currently only scans src/. The toolchain/ directory doesn't have Fortran, but if examples/ ever gets .fpp files the scanner would miss them. The scope is appropriate for now.

---

Overall: Clean implementation, well-motivated checks, good integration. No blocking issues. The blank-line edge case in check_duplicate_lines is the only finding worth acting on before merge.

[github-actions]

Claude Code Review

Head SHA: d5ea340
Files changed: 7 — .claude/rules/common-pitfalls.md, .github/workflows/cleanliness.yml, .github/workflows/lint-source.yml, CLAUDE.md, CMakeLists.txt, toolchain/bootstrap/precheck.sh, toolchain/mfc/lint_source.py

Summary

• Adds toolchain/mfc/lint_source.py: a Python static analysis linter catching Fypp list duplicates, consecutive duplicate lines, and hardcoded 8-byte real sizes.
• Integrates the linter as step 5/6 in precheck.sh and in the lint-source.yml CI workflow.
• Adds -Wconversion to GNU debug builds and tracks it in the cleanliness workflow.
• Documentation updated to reflect 6 precheck steps (CLAUDE.md, common-pitfalls.md).

---

Findings

1. — lint output suppressed on failure (significant)

File: toolchain/bootstrap/precheck.sh, lines ~105–110

The Fortran/Fypp analysis subprocess discards all stdout/stderr:

if python3 toolchain/mfc/lint_source.py > /dev/null 2>&1; then

When this step fails, the user only sees:

Fortran/Fypp analysis failed. Run python3 toolchain/mfc/lint_source.py for details.

Every other precheck step either displays errors inline or points to structured output. This inconsistency makes the failure harder to debug in CI where re-running interactively isn't always practical. Suggest capturing output to a tmp file and printing it on failure:

python3 toolchain/mfc/lint_source.py > "$TMPDIR_PC/fortran_out" 2>&1
# then on failure: cat "$TMPDIR_PC/fortran_out"

2. — reset by blank/comment lines (minor)

File: toolchain/mfc/lint_source.py, lines ~78–88

prev_stripped is unconditionally updated on every line, including blank lines and comments. Two identical long lines separated by a single blank line or comment will not be detected. This may be intentional (only consecutive duplicates), but it is worth documenting. Real examples (like the R3bar bug mentioned in the PR) tend to be truly adjacent, so this is unlikely to miss actual bugs in practice.

3. — multi-line accumulation strips twice (cosmetic)

File: toolchain/mfc/lint_source.py, lines ~52–62 and line ~38

The outer accumulator appends continuation lines after stripping them (already removes & characters), then _check_single_fypp_list also does list_content.replace("&", ""). Double-stripping is harmless but the redundancy could be removed.

---

Improvement opportunities

1. No unit tests. The linter has no automated tests (e.g., a small fixture .fpp with known violations). This is consistent with current toolchain style, but as the linter grows, false-positive/negative regressions will be harder to catch. Even a minimal tests/test_lint_source.py would help.

2. _fortran_fpp_files yields both .f90 and .fpp — if a build ever places generated .f90 files under src/, the duplicate-line checker could flag auto-generated code. A comment noting that only hand-written source is expected there would clarify the assumption.

3. check_hardcoded_byte_size regex — the fix hint in the error message says storage_size(0._stp)/8 but this formula itself uses a byte calculation. A brief comment in the code explaining why storage_size is the correct portable alternative would reinforce the intent.

---

Overall this is a well-motivated, focused PR. The linter logic is sound and the three checks are each motivated by a real codebase bug. The main actionable issue is suppressing output in precheck.sh — the rest are minor observations.