Code Review - PR #243: CI Weeder Check

Summary

This PR adds weeder (unused code detection) to the CI pipeline. The implementation is solid overall, with good caching strategy and proper artifact management.

✅ Strengths

• Good separation of concerns: Weeder job properly depends on build job
• Efficient caching: Weeder binary cached with appropriate key (weeder-2.9.0-ghc-9.12.2)
• HIE files generated: Correct use of -fwrite-ide-info -hiedir=.hiefiles
• Artifact retention: 1-day retention is appropriate for PR artifacts
• Config exists: config/weeder.toml already configured

🔧 Suggested Improvements

1. Reduce artifact size (Performance)

Uploading entire dist-newstyle (~hundreds of MB) is excessive. Weeder only needs specific files:

- name: Upload build artifacts for weeder
  uses: actions/upload-artifact@v4
  with:
    name: weeder-artifacts
    path: |
      .hiefiles
      dist-newstyle/**/*.conf
    retention-days: 1

Then combine downloads:

- name: Download build artifacts
  uses: actions/download-artifact@v4
  with:
    name: weeder-artifacts

Impact: Faster upload/download, less storage, reduced CI time.

2. Consolidate GHC options (Code Quality)

Line 72 duplicates GHC options from line 66. Use single build command - the -O0 -j8 from configure already applies globally.

3. Add validation (Reliability)

Add check before running weeder:

- name: Run weeder
  run: |
    if [ ! -d ".hiefiles" ]; then
      echo "Error: HIE files not found"
      exit 1
    fi
    ~/.local/bin/weeder --config config/weeder.toml

4. Minor: PATH vs absolute path (Code Style)

Instead of ~/.local/bin/weeder, add to PATH for cleaner code:

- name: Run weeder
  run: |
    export PATH="$HOME/.local/bin:$PATH"
    weeder --config config/weeder.toml

📊 Test Coverage

• ✅ Weeder config exists and properly configured
• ⚠️ Consider adding a test case with known dead code to verify weeder actually runs

🔒 Security

• ✅ No security concerns
• ✅ Artifacts have appropriate short retention
• ✅ No secrets exposed

📝 Other Notes

• PR description is empty - please add context about why weeder is being added
• Checklist items not completed

Overall: Good implementation. Main suggestion is reducing artifact size for better performance. Ready to merge with or without suggested improvements.

Code Review: Weeder CI Integration

Summary

This PR adds weeder integration to detect unused code in CI. Good initiative for code quality.

Issues & Recommendations

Critical: Artifact Upload Size 🔴

Lines 74-81: Uploading entire dist-newstyle/**/*.conf will create massive artifacts (potentially GBs).

• Weeder only needs package configuration, not all build artifacts
• Fix: Use specific path: dist-newstyle/packagedb/ghc-9.12.2/*.conf or similar

Redundant Validation ⚠️

Lines 235-238: The HIE files check is unnecessary:

• download-artifact already fails if artifact missing
• Extra validation adds no value
• Remove: Delete the conditional check, let the action fail naturally

Missing Error Handling 🔴

Line 239: Weeder execution has no output capture or interpretation:

• Weeder failures should be clear and actionable
• Add: || { echo "::error::Weeder found unused code"; exit 1; }

GHC Options Duplication ⚠️

Lines 66 + 72: GHC options specified in both configure and build:

• --ghc-options already set in configure (line 66)
• Simplify: Remove from line 72, use: cabal build all -j4
• Then add HIE-specific options only: --ghc-options="-fwrite-ide-info -hiedir=.hiefiles"

Optimization Opportunity 💡

Lines 214-226: Weeder installation could be faster:

• Consider using pre-built binary from GitHub releases
• Or add to project's tool dependencies in cabal.project
• Current approach rebuilds weeder on cache miss

Code Succinctness

The yaml could be more concise:

# Lines 233-239 current:
- name: Run weeder
  run: |
    if [ ! -d ".hiefiles" ]; then
      echo "Error: HIE files not found"
      exit 1
    fi
    ~/.local/bin/weeder --config config/weeder.toml

# Suggested:
- name: Run weeder
  run: ~/.local/bin/weeder --config config/weeder.toml

Testing Recommendations

• Verify artifact size after fixing path (should be <10MB)
• Test weeder with intentionally unused code to ensure it fails CI
• Document in weeder.toml what code patterns are considered roots

Security

✅ No security concerns. Using official GitHub actions and specific weeder version.

Performance

• HIE file generation adds ~5-10% to build time (acceptable)
• Artifact upload/download could be slow if not fixed (see above)
• Weeder execution itself is fast (<30s typically)

Priority: Fix artifact upload path and remove redundant validation before merging.

Code Review

Overview

Adds weeder dead-code detection to CI and revamps config/weeder.toml to use correct module-name regex patterns instead of file paths (the old config's root-modules entries were file paths, which weeder doesn't accept — so this is also a correctness fix).

Concerns

1. No version pin on cabal install weeder (workflow)

cabal install weeder --install-method=copy ...

Without a version constraint this will silently pick up a new major release if one ships, potentially changing which declarations are flagged or breaking the invocation. Pin it:

cabal install weeder-2.9.0 --install-method=copy ...

2. Cold-install cost on every CI run

cabal install weeder compiles weeder from source, which takes several minutes when the cache is cold. Consider either:

• caching /usr/local/bin/weeder in its own actions/cache step keyed on the pinned version, or
• using a pre-built binary if the project already has a Nix shell / nixpkgs overlay available (weeder ships as a nixpkg).

3. ^System\.Logging$ / ^System\.Tracing$ don't cover submodules

^Data\.Effectful\. (trailing dot, no $) correctly wildcards the whole subtree. But:

"^System\\.Logging$",
"^System\\.Tracing$",

are anchored with $, so System.Logging.Internal (if it exists) would still be flagged. Either add both patterns or use the open suffix like the Data.Effectful entry:

"^System\\.Logging",
"^System\\.Tracing",

4. --hie-directory dist-newstyle passes the whole build tree

Weeder will walk every .hie file it finds under dist-newstyle, including deps if any end up there. The conventional approach is to pass the project-source subdirectory, e.g. dist-newstyle/build or use a glob. Low risk in practice but worth double-checking if weeder starts emitting noisy hits from vendored/generated code.

Minor / Nitpicks

• The ^Spec\.spec$ root is harmless but only useful if there's actually a Spec module exposing a spec symbol — confirm this matches the test runner entry point.
• The inline comment # package.yaml already passes -fwrite-ide-info is useful, but could mention which component or stanza sets it, so a future reader doesn't have to grep to verify.
• continue-on-error: true with the flip-once-triaged note is good pragmatism — just make sure a tracking issue/ticket exists so it doesn't stay true indefinitely.

Summary

Good addition overall — the root-modules migration from file paths to module-name regexes is a real fix, and the new roots (^Paths_, ^Spec\., ^Proto\.) are sensible. Main things to address before the step becomes blocking: pin the weeder version and decide on a caching strategy to keep CI fast.