Repository navigation
feat(ir): origins replace declaration handles for execution text - #2156
Open
devin-ai-integration[bot] wants to merge 13 commits into
Open
devin-ai-integration[bot] wants to merge 13 commits into
devin-ai-integration[bot] wants to merge 13 commits into
Conversation
The state executor's own records name states, regions, pseudostates and transitions by the program.StateID/RegionID/PseudoID/TransID values lowering assigns, and vertices by the new program.VertexRef, in place of AST and lower.Transition pointers: the active configuration, state stack, state attributes, history, ahead sets, routes, region and lazy entries, unit heads, firing scopes, dispatch candidates, held entries, join arrivals, breakpoints, timer and change-trigger maps, the held image and the snapshot. FiredTransition carries the transition id, source and target VertexRefs, an EntryRef for an entry transition and the Origin; StateExecutor.FiredNodes maps a firing back to its declarations for the LSP. Lowering now numbers the hidden states a parallel body's typed usages stand for, which the executor reaches through parent chains and region tables. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # internal/ir/lower/identity_test.go
firedNames spells a fired transition through FiredNodes, letting StateVertexName name a missing source as the empty string rather than branching on it. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…cutor Keying the state executor's records by graph-local ids adds the id tables' accessors at every record read; the RE REPL wasm grew to 16108375 gzipped bytes and the engine to 9526983, past the 16100000 and 9500000 budgets the base holds under. The budgets move to 16200000 and 9600000. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
statesInOrder reaches hidden region owners through a worklist over parent chains and composite regions, and regionsInOrder walks it rather than States, so a hidden owner's own regions take ids too. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…rging state ids Co-Authored-By: jason.han <hanhuijun@gmail.com>
Action-node Origin.Kind is the construct's name (program.NodeKind.Word) and Origin.Element the declaration's element id; entry transitions and bindings carry an Origin, entry transitions the text of their written source. The runtime describes nodes by Origin instead of nodeDescription, keys binding owners and registered declarations without AST handles, and drops the unread NoValueError.Ref; gRPC branches read ActionEdge.Else and node kinds, and document state rows read state origins and provenance. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ode out of line Deriving element ids at lowering time needs a semantics model; creating one per graph attaches it to the shared resolver (semantics.NewModel) and clears its caches, which tests/stressmodel caught as state outliving edits, and importing identity from lower costs sysml-engine 32 KB gzipped and sysml-wasm its size budget. Element stays empty for the owner of the model to fill. describeNode is not inlined: its error-path callers each carried the lookup, 17 KB of wasm code. A binding's Origin has no Path, there being no set it is unique within. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
Contributor
Author
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
A connector end's implicit binding was lowered without one, so a run keying binding owners by origin took every such binding for the same one and missed a cycle between them. Co-Authored-By: jason.han <hanhuijun@gmail.com>
Contributor
Author
There was a problem hiding this comment.
Devin Review found 1 finding. Devin automatically fixed it.
Fixed by Devin (1)
- Implicit binding cycles go unreported — EndValueBindings lowered connector-end value bindings without an Origin, so binding owners keyed by origin could not tell them apart; they now carry the end attribute's origin, and TestEndValueBindingsLowerEndBodyValues pins that the two differ.
This branch has not been deployed
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
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.
What and why
Step 4 of the execution-owned IR migration (
docs/internals/execution-ir-design.md, "Migration as a stack of pull requests", row 4): origins replace declaration handles for the text a run prints, so the executors, the gRPC session and the document state query name things fromprogram.Originand the lowered records rather than fromastnodes. Expressions stay AST-based; theexec/runtime → syntax/astedge is not removed here.Stacked on #2144 (
feat/ir-action-exec-ids), with #2145 (feat/ir-state-exec-ids) merged in--no-ff. Until #2145 merges, this diff shows #2145's changes as well as its own; the step-4 diff alone isgit diff fa62971a6b..HEAD. Required merge order: #2137, then #2144 and #2145, then this PR (retargeted todevelopafter they land).Kind words.
program.NodeKind.Word()is the construct's SysML v2 name andOrigin.Kindcarries it for action nodes:initial node,final node,fork node,join node,merge node,decision node,action,perform action,accept action,send action,assignment action,terminate action,if action,while loop,block,statement. Vertices keep thelower.VertexKindwords (state,pseudostate,terminate action, …); regions, transitions, entry transitions and bindings areregion,transition,entry transition,binding.Origin.String()isnode <name>when named,the <kind>otherwise,the nodefor the zero Origin. An unnamed node that a message once printed asthe *ast.IfActionNodeis nowthe if action.Public surface:
program:NodeKind.Word();Origin.String()defines the zero value;Origin.Kindis a construct name, never a Go type name.lower:(*ActionGraph).OriginOf(node);EntryTransition.OriginandEntryTransition.SourceText(thelower.EndpointTextof aTransitionMember's source, lowered once);Binding.Origin(no Path: a binding belongs to no set it is unique within).runtime:nodeDescriptionis gone;describeNode(graph, node)andNodeRef.Origin()read the graph's origins (guardHoldstakes the graph).firedEntry,entryFromName,entryTransitionEffectHeadandstateSourceOriginreadEntryTransition.Origin/SourceTextand the state origin table instead oflower.DocOf/lower.EndpointText.Context.bindingOwnersholdsprogram.Origin,Model.declaredis keyed bysymbols.ElementKey(symbols.KeyOf), andNoValueError.Refis removed: nothing read it.session.go:branchesOf/gateOfreadKinds,OriginsandActionEdge.Elseinstead of type-switching onast.DecisionNode/ast.ControlFlowEdge.queryexec/states.go: state and region names come fromOrigins/RegionOrigins, the state symbol throughProvenance.State(id).Conformance expectations moved: none. No
.expected.json, trace or gRPC golden carried athe *ast.Xrendering, so every baseline is byte-identical; the step-1 pin test is rewritten asTestOriginDescriptions(node <name>/the <NodeKind.Word>, kind word, span and document over every conformance action) rather than deleted.Merge of #2144 and #2145: no textual conflict. One semantic conflict — both sides removed the last
astuse insnapshot.go, leaving an unused import that failed the build — fixed in its own commit.Where the design met the code:
Origin.ElementandOrigin.Docsstay empty. Element ids come fromidentity, which needs asemantics.Model: creating one per lowered graph is not an option (semantics.NewModelattaches itself to the shared resolver and clears its caches —tests/stressmodelTestEditsHoldNoStaleStatecaught the first attempt as 40 MB of state outliving edits), reading the model the resolver's owner attached works but importingidentityfromlowercostssysml-engine32 KB gzipped (it never linkedidentity) and putssysml-wasm526 bytes over its budget. Element wants an id provider the model's owner registers (or a fill throughProvenancein the frontends that already hold an identity table); that is a follow-up, not a budget raise. Docs need the source text (semantics.Model.DocumentationOf), and lowering has no channel for it: the graphs are lowered from a*resolve.Resolverthreaded through some twenty functions, several in the calc-lowering files step 5 owns, and the index holds no text.describeNodeis//go:noinline: inlined, its error-path callers each carried the origin lookup, 17 KB of wasm code; the combined budget holds at 10,498,449 gzipped (10,494,609 on the bare merge, budget 10,500,000; no budget moved).NoValueError.Refwas dropped rather than lowered: no caller read it.stateOriginanswers the zero Origin for it andstateSourceOriginfalls back to the machine's document as before.How it was verified
go build ./...,go vet ./...,gofmt -l .(empty),make lint,go test ./...,go test -count=1 ./tests/corpus -run 'TestTrainingExamples|TestPilotCorpora'with both corpora downloaded,go test ./tests/hygiene,go test -C tools ./...,go test ./tests/wasm(budgets unchanged),python3 scripts/check-doc-ids.py,python3 scripts/changelog.py check; the comment-length check over the merge-base diff prints nothing. The same gate ran on the bare merge of #2144 and #2145 before any step-4 code (it found thesnapshot.goimport).Tests that pin the step:
TestExecutionConformance(error text),TestOriginDescriptions,TestNodeKindWord/TestOriginString,TestAssignIDs_ActionOriginsNameWhatMessagesName,TestGRPCConformance,conformance_docquery_test,tests/grpcand the runtime robustness suites.Checklist
make testandmake lintpass locallychanges/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved (compliance rows need nothing: the census is counted at docs build)F4,K5) in the body, docs, or changelogLink to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/42ffb49bd028432a81e2981ea0a239bf
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/42ffb49bd028432a81e2981ea0a239bf?variant=devin
Requested by: @HuiJun
🟢 Devin Review found nothing to fix · View findings · Follow Devin's work