SY-4297: Arc Params Refactor (Base) - RFC & Plan#2420
Conversation
2e7fb37 to
4bbcdac
Compare
pjdotson
left a comment
There was a problem hiding this comment.
some other things worth considering:
- what would the syntax look like for a very complicated "function that returns a function that returns a function"? this is useful for thinking about NOT because this is something users would every do, but it gets towards the goal of "express the most with the least syntax"
- what about thinking about the function syntax for a long-term refactor where we might want to remove stateful variables? how would that look?
- would be very good to have a short section that says basically what one-way door move towards with this.
| # 2 - Non-Goals | ||
|
|
||
| - **User-facing Arc syntax.** `name(...)`, `name{...}`, and `wire -> name{...}` parse, | ||
| analyze, and run identically. The grammar is untouched. |
There was a problem hiding this comment.
you might want to change this to be a goal. e.g. what should the long-term syntax be
that is simplest?
There was a problem hiding this comment.
Deliberately keeping this a non-goal rather than promoting it to a goal. Reasoning being the scope boundary itself. The goal of this RFC is parity: one parameter model under the hood, with the surface syntax held exactly as it is today. That keeps the change non-breaking for customers and bounds the test churn to the refactor. If I fold a grammar change in here, every existing program and every test migrates on top of an already massive diff, and a clean internal refactor turns into a breaking syntax change.
That said, I am thinking about the simplest long-term syntax. The direction I like is {}== trigger, () == inputs, which gives this equivalency:
a -> f(b) -> output
{a} -> f(b) -> output
f{a}(b) -> output
I added the sketch to the RFC. But it's a customer-breaking grammar change, so it earns its own RFC after parity lands, rather than being bolted onto this one.
There was a problem hiding this comment.
Why do we want to remove stateful variables?
| `Trigger`, the `ExecWASM` / `ExecFlow` / `ExecBoth` enum no longer selects a param array | ||
| or feeds the trigger decision. It is kept only as a superficial gate, a thin permission | ||
| flag the analyzer consults to decide where a symbol may be called (Section 4.1); every | ||
| symbol is otherwise treated as usable in any context. |
There was a problem hiding this comment.
shouldn't we get rid of the gate entirely in the long term, long-term we want a symbol
to be able to be called anywhere?
There was a problem hiding this comment.
Yes, but for now, it's needed to prevent time.interval and time.wait from being called in a func context. I also want to revisit that rule, but I think it will be a lot easier when it's a flag that we can set, as opposed to opening the flood gates now and introducing potential bugs.
There was a problem hiding this comment.
This seems like something crucial to answer now in this RFC. I feel like a core part of this proposal is to remove the difference between functions called in flow contexts and ones called in the imperative context. Open questions:
How do you decide which node stateful variables bind to?
Are there nodes that just can't be used in the imperative context?
We're also now adding a trigger field while keeping this field? Why?
|
Expanded on the "Future Work" section for the syntax, HOF, and stateful/global variables. |
Phase 2 of the function-call unification (RFC 0039). - Replace CallHook/FlowConfigHook with a single ArgumentsHook over a unified Argument view (symbol.go). - Add Trigger/TriggerBinding with TriggerOnly and TriggerInput helpers to Symbol. - Propagate the Config removal through the type layer: drop the Config comparison in Equal, drop the Config freshen, and point ResolveConfigChannel at Inputs. Intermediate phase: the tree intentionally does not compile until the stack completes (~35 .Config readers remain in later phases).
Introduce a shared `call.Analyze` (new `analyzer/call` package) that validates a call's arguments against the unified `Inputs` list for both surface forms. The two parallel validators are removed: `validateFunctionCall` (parens) and `validateFuncConfig` (brace) now adapt their arguments via `inputArguments` and delegate to `call.Analyze`, which also runs the unified `AnalyzeArguments` hook. Replace the `upstreamIsTrigger` heuristic and the three-branch upstream handling in the flow analyzer with `consultTrigger`, which keys off the symbol's explicit `Trigger`. User-defined functions are assigned `Trigger` in `CollectDeclarations` (the first parens-block param, else `TriggerOnly`). Drop the now-dangling analyzer `.Config` readers (the substitution loop in resolve.go and the function-type clone in constraints.go). The tree does not compile yet by design: downstream consumers (ir, graph, text, compiler) still read `.Config` and are converted in later phases.
pjdotson
left a comment
There was a problem hiding this comment.
Looks good on my end. In the future can you please try to make RFCs like these shorter / easier to read? There isn't really 2000 lines of stuff to talk about here, and some of it is details I don't particularly care about like the implementation plan and stuff like that.
There was a problem hiding this comment.
My main question is: Why are we deciding on this internal refactor before we have a decision on what the long term syntax and semantic model for arc programs is?
It seems to me like that's the actual important question to answer, and everything in this RFC is just talking about implementation details, which is substantially less important.
Benefits we're getting from this RFC:
- Collapse
ConfigandInputsinto one thing. - Make it so that things like
status.sethas one overload rather than two.
Those are benefits, but they seem pretty marginal.
- What does a
status.setfunction look like that operates in both contexts? These are the two set signatures:
func set(key_or_name str, message str, variant str) {}
func set{
key_or_name str
message str
variant str
} () {}
In the future what does the signature look like? Crucially when I do
my_cat -> status.set{} where does my_cat get routed to? The key? the message? the variant?
What are the rules to make sure a function has all of it's input parameters?
Personally I don't really care about how you refactor the internals to work, that's less important. Having one or two fields to specify configuration parameters vs. inputs is not that concerning of a decision.
All of the things you have in the 'non-goals' section to me are what's actually important to answer in an RFC of this length.
Why should this RFC be implemented before we decide on the answers to the non-goals section? It doesn't seem to me like this is blocking anything critical.
I made the decision to separate config and input params carefully, and it seems like this RFC is deciding to collapse the internals (which is fine) without answering the important questions of user facing semantics.
P.S. I don't want to write more essays. I will discuss this verbally from now on.
| `Trigger`, the `ExecWASM` / `ExecFlow` / `ExecBoth` enum no longer selects a param array | ||
| or feeds the trigger decision. It is kept only as a superficial gate, a thin permission | ||
| flag the analyzer consults to decide where a symbol may be called (Section 4.1); every | ||
| symbol is otherwise treated as usable in any context. |
There was a problem hiding this comment.
This seems like something crucial to answer now in this RFC. I feel like a core part of this proposal is to remove the difference between functions called in flow contexts and ones called in the imperative context. Open questions:
How do you decide which node stateful variables bind to?
Are there nodes that just can't be used in the imperative context?
We're also now adding a trigger field while keeping this field? Why?
…r-part-1 SY-4297: Arc Params Refactor (Part 1) - Oracle Changes
…r-part-2 SY-4297: Arc Params Refactor (Part 2) - Unify Symbol Hooks and Add Trigger Binding
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## rc #2420 +/- ##
==========================================
- Coverage 67.68% 67.38% -0.31%
==========================================
Files 2546 2496 -50
Lines 124878 117736 -7142
Branches 9145 8866 -279
==========================================
- Hits 84525 79331 -5194
+ Misses 33820 32182 -1638
+ Partials 6533 6223 -310 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fold the analyzer/call package into expression as AnalyzeCall and add unit tests. flow calls expression.AnalyzeCall; the separate package is removed.
I agree on not wanting to write essays, which is why the points above need no further discussion for this scoped change. The large RFCs have been in direct response to suggested scope creep. I also understand that the decision to separate Config/Inputs was deliberate. After using it and writing new modules, the question I am answering here is "Is there a better model for upstream triggers and function inputs?" It is not to answer how we should handle higher order functions, changing syntax, or changing the user-facing semantics. |
|
@emilbon99 , replying to this comment Two of these are the Exec gate (Section 4.1); one is out of scope. Nodes that can't be used imperatively: yes, that's exactly what Exec gates. Flow-only symbols (time.interval, channel.write, etc.) are restricted to flow, pure-computation ones to func. The check is symbol.Exec.Compatible(contextExec). Keeping Exec and adding Trigger: they answer different questions, and this RFC actually separates two jobs Exec did at once. Exec decides WHERE a symbol can be called (flow vs imperative). Trigger decides WHAT an upstream wire binds its value to once in flow (a param, or nothing). Today Exec is overloaded and also feeds the trigger heuristic, so this strips it down to the context gate and moves the param-binding decision to Trigger. Stateful variable binding: out of scope here. This refactor doesn't change how stateful variables bind or instance, that's a runtime concern untouched by the call-path unification. |
…r-part-3 SY-4297: Arc Params Refactor (Part 3) - Collapse the Analyzer Call Paths
Arc Function-Call Unification
Linear Issue
SY-4297
This is the base branch for RFC 0039. The
-part-Nbranches stack on this one and collapse into it as they land. Once every part has merged in and the tree is green, this branch merges torcas a single complete change, sorcnever sees a broken intermediate state. It also carries the RFC and plan the implementation follows.Parts
Config)TriggerbindingConfigtoInputs)The change
Arc currently splits every function signature across two parameter arrays:
Config(the{...}brace form) andInputs(the(...)parens form). This refactor collapses them into one:Inputsonly. NoConfig, no per-param "configuration vs input" distinction at the type level.Triggerfield onSymbolreplaces theExec == ExecBoth && len(Config) > 0heuristic.TriggerOnlymeans the wire activates;TriggerInput(name)binds the wire's value to a named param.Execdemoted to a gate. No longer selects a parameter array or drives trigger detection. It survives only as a call-site permission flag, consulted viaExec.Compatible(contextExec).call.Analyze; the two analyzer hooks become oneAnalyzeArguments.User-facing syntax is unchanged:
name(...),name{...}, andwire -> name{...}parse, analyze, compile, and run exactly as before.Docs
docs/tech/rfc/0039-260513-arc-function-call-unification.md— design.docs/tech/rfc/0039-260513-plan.md— phasing and stacked-PR strategy.Greptile Summary
This PR is a documentation-only base branch for RFC 0039, landing the design specification and implementation plan for the Arc function-call unification refactor. No code ships here; the nine stacked phase PRs carry the actual changes.
arc-function-call-unification.md, 978 lines) specifies collapsing theConfig/Inputsdichotomy into a singleInputslist, replacing theupstreamIsTriggerheuristic with an explicitTrigger TriggerBindingfield onSymbol, demotingExecto a superficial gate, and merging the two call-validation paths into onecall.Analyzeroutine. Codec migration (Phase 8) handles the wire-format break by concatenating oldConfigbefore oldInputsand deduplicating mirror-trick symbols.plan.md) breaks the work into nine stacked PRs with a design-evaluation gate after Phase 3; it includes per-symbol trigger-assignment tables, a verification checklist, and a risk register.syntax-exploration.md) is scratchwork comparing function-passing (Style A) vs. explicit triggers (Style B) across eight scenarios; it supports the RFC's decision to adopt Style B.Confidence Score: 4/5
Documentation-only PR with no code changes; safe to merge once the branch-naming ambiguity between the plan and the PR description is resolved.
The RFC and implementation plan are well-structured and internally consistent. The main findings are a naming mismatch between -phase-N (plan) and -part-N (PR description) that will confuse implementers creating the first stacked branch, and an underspecified fallback in the Phase 8 deduplication logic that could lead two implementers to make different choices for corrupted-but-plausible IR inputs. Neither introduces code risk in this PR, but both are worth resolving before the phase PRs begin.
docs/tech/rfc/0039-260513-plan.md — branch-naming inconsistency and migration dedup fallback; docs/tech/rfc/0039-260513-arc-function-call-unification.md — same dedup gap in §5.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Arc source: name(...) / name{...} / wire -> name{...}"] B["Parser: unchanged grammar"] A --> B B --> C["Expression analyzer (parens form)"] B --> D["Flow analyzer (brace form + wire)"] subgraph After_RFC_0039 C --> E["call.Analyze"] D --> E E --> F["Unified Inputs list"] E --> G["AnalyzeArguments hook"] D --> H["fn.Trigger.Target consult"] H -->|"Target == ''"| I["TriggerOnly: wire = pure activation"] H -->|"Target == param"| J["TriggerInput: wire binds to named param"] end F --> L["WASM ABI"] G --> M["Diagnostics"]Reviews (1): Last reviewed commit: "SY-4297: Update RFC and Plan" | Re-trigger Greptile