docs: ADR 006 — Phase 2b interprocedural method-value propagation#30
Merged
Merged
Conversation
Closes the loop from ADR 005, which deferred issue #26 (a method value captured in one function, passed as an argument, invoked from inside a different callee — the real, field-tested e2b-dev/infra case) as "Phase 2b" without committing to an approach. Documents a wrinkle found while scoping this: the flag key is *also* interprocedural, not just the method value's identity — it's a plain call-site literal, invisible from inside the callee's own body — which rules out simply propagating a binding into the callee and ruled out a naive first instinct for how this might be solved. Presents two approaches and accepts the narrower one: a "forwarding function" pattern (recognize a callee that forwards its function-valued parameter to a call using one of its own parameters as the key, resolving identity at the call site rather than inside the callee — no go/ssa/go/callgraph needed, reuses existing infrastructure) over general data-flow via go/ssa/go/callgraph (generalizes further but is a meaningfully larger, riskier engineering investment with its own precision/soundness tradeoffs). Matches how #16 played out: a narrower mechanism than expected turned out to be sufficient once shipped. Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds ADR 006, documenting the decision to implement interprocedural method-value propagation via a forwarding-function pattern (approach A), with a broader ChangesADR Documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the loop from ADR 005, which deferred issue #26 (method value crossing a function boundary — the real, field-tested e2b-dev/infra case) as "Phase 2b" without committing to an approach.
Documents a wrinkle found while scoping this: the flag key is also interprocedural, not just the method value's identity — it's a plain call-site literal, invisible from inside the callee's own body — which rules out a naive "propagate the binding into the callee" fix.
Accepts a narrow "forwarding function" pattern (recognize a callee that forwards its function-valued parameter to a call using one of its own parameters as the key, resolving identity at the call site) over general
go/ssa/go/callgraphdata-flow — discussed with and approved by the user before writing this up.Test plan
Summary by CodeRabbit