Skip to content

Support dune instrumentation backend (#50)#102

Merged
lukstafi merged 2 commits intomainfrom
ludics/gh-ppx-minidebug-50-s1/root
Mar 25, 2026
Merged

Support dune instrumentation backend (#50)#102
lukstafi merged 2 commits intomainfrom
ludics/gh-ppx-minidebug-50-s1/root

Conversation

@lukstafi
Copy link
Copy Markdown
Owner

Summary

  • Register ppx_minidebug as a dune instrumentation backend ((instrumentation.backend (ppx ppx_minidebug)))
  • Add --auto flag and auto_instrument_impl hook to auto-instrument unannotated top-level functions with entry/exit tracing
  • Inject module Debug_runtime = (val Minidebug_db.debug_db_file "debugger_<filename>") automatically when needed
  • Add [%%ppx_minidebug_disable] extension for per-file opt-out
  • Prevent double instrumentation of already-annotated let%debug_* bindings
  • Strip type annotations in auto-mode to avoid generating unexpandable [%show:]/[%sexp_of:] extensions (impl hook runs after context-free rules)

Test plan

  • PPX transformation tests: test_auto_instrument, test_auto_disable, test_auto_no_functions, test_auto_mixed
  • Runtime behavior test: test_auto_runtime (generates DB, verified via minidebug_view)
  • Integration fixture: test/instrumentation_fixture/ with run_instrumentation_test.sh
  • No regressions: dune runtest passes all existing tests

Closes #50

🤖 Generated with Claude Code

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a1753ccdf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2090 to +2094
vb with
pvb_expr = strip_expr vb.pvb_expr;
pvb_pat = strip_pat vb.pvb_pat;
pvb_constraint = None;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve type constraints during auto instrumentation

Dropping pvb_constraint in auto mode changes the user’s typing contract and can make previously valid functions fail to type-check (for example functions relying on locally abstract types, GADT refinements, polymorphic recursion, or coercions). Because --auto is meant to add tracing, not alter type semantics, clearing this field can break builds or silently change inferred types when instrumentation is enabled.

Useful? React with 👍 / 👎.

lukstafi and others added 2 commits March 25, 2026 20:39
Register ppx_minidebug as a dune instrumentation backend so users can
enable auto-instrumentation of all top-level functions via
`(instrumentation (backend ppx_minidebug --auto))` in their dune files.

- Add `(instrumentation.backend (ppx ppx_minidebug))` to library stanza
- Add `--auto`, `--auto-mode`, `--auto-log-value`, `--auto-db-base-name` CLI args
- Implement `auto_instrument_impl` as ppxlib `impl` hook for whole-file transform
- Inject `module Debug_runtime = (val Minidebug_db.debug_db_file ...)` when needed
- Add `[%%ppx_minidebug_disable]` extension for per-file opt-out
- Strip type annotations in auto-mode to avoid unexpandable PPX extensions
- Detect already-instrumented bindings to prevent double instrumentation
- Add PPX transformation tests, runtime behavior test, and integration fixture

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restore the original pvb_constraint on the output of debug_binding
after stripping it for the input. This preserves locally abstract types,
GADT refinements, polymorphic recursion, and coercions while still
suppressing value logging (which would generate unexpandable [%show:]
extensions in the impl hook).

Add test_auto_poly.ml to verify polymorphic type constraints are preserved.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lukstafi lukstafi force-pushed the ludics/gh-ppx-minidebug-50-s1/root branch from e55e096 to cd3faad Compare March 25, 2026 19:39
@lukstafi lukstafi merged commit 84dc9fa into main Mar 25, 2026
0 of 6 checks passed
@lukstafi
Copy link
Copy Markdown
Owner Author

Suggested Refactors & Lessons for Next Time

1. Extract eligibility predicate

The "is this binding eligible for auto-instrumentation?" check is duplicated in three places: needs_runtime_injection, any_eligible, and the inner List.map filter. Extract a single is_auto_eligible vb predicate and reuse it everywhere.

2. Consider a dedicated auto_wrap_binding instead of reusing debug_binding

Using debug_binding with strip_annotations_for_auto + restoring pvb_constraint is a workaround for the fundamental tension between the impl hook (runs after context-free rules) and PPX extension generation. A purpose-built auto_wrap_binding that directly generates open_log/close_log without touching types would be simpler, more robust, and wouldn't need the strip-and-restore dance. It would also make it easier to add auto-mode-specific features (e.g., logging function arity, module path) without coupling to debug_binding's complex type extraction logic.

3. Move is_already_instrumented closer to its logical peers

Currently is_already_instrumented is defined early in the file (near needs_runtime_injection) because it's used by that function. But conceptually it belongs with the auto-instrumentation code. Consider restructuring so the auto-instrumentation helpers form a cohesive section, possibly with a comment banner.

4. The instrumentation.backend flags limitation deserves documentation

Dune's (instrumentation.backend ...) stanza does NOT support a flags field — the --auto flag must be passed on the consumer side as (instrumentation (backend ppx_minidebug --auto)). This is a common gotcha that should be documented prominently, perhaps with an example in a README section or the opam description.

5. Future: support value logging in auto-mode

The current approach strips type annotations to avoid generating [%show:]/[%sexp_of:] extensions that can't be expanded in the impl hook. A future improvement could:

  • Generate direct Stdlib.string_of_* calls for primitive types (int, float, string, bool) without needing PPX extensions
  • Or use a two-pass approach where the first pass marks bindings for instrumentation and the second (context-free) pass generates the actual logging code
  • Or register auto-instrumentation as a context-free rule on Pstr_value items rather than an impl hook, so generated extensions can still be expanded by other PPXes

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support dune instrumentation backend functionality

1 participant