diff --git a/docs/changes/unreleased/977-frame-law-parameter-shape.md b/docs/changes/unreleased/977-frame-law-parameter-shape.md new file mode 100644 index 0000000000..80515b759c --- /dev/null +++ b/docs/changes/unreleased/977-frame-law-parameter-shape.md @@ -0,0 +1,10 @@ +--- +kind: fixed +title: the frame law sees a surface passed as a parameter, and a press is not a paint (#898) +pr: 977 +surface: [chat, build] +invalidates: + - "The frame law in internal/tui3 stopped at a package function taking the surface as a parameter. `placeFrameWithBar(a, …)` was walked as a function but its `a.…` calls were dropped, so `app.View → app.placeDraw → placeFrameWithBar` ⊘ `a.composerRows → … → a.errandPlace → errandHomeDir → os.Getwd` was invisible to a law whose forbidden set already contained os.Getwd. Every parameter typed `*app` is now a second spelling of the surface (surfaceNames), and the planted shape in TestTheLawSeesEveryShapeOfIndirectionItClaims turns red if that branch is deleted." + - "The frame walk counted EVERY function literal a paint builds as something the paint runs, including the closure a row files for a press — `verb{do: func() tea.Cmd{…}}`. So `a.homeCrossChord`, which asks a running row for its verbs only to NAME them on the hint line, dragged the stop verb's `a.closeHome` and the whole navigation graph behind it onto the frame: 3156 reachable nodes where the paint has 2915, and `app.stowDrafts` reading a draft record off the disk looked like a read a paint took. A literal that answers a `tea.Cmd` is now the loop's and not the paint's (runsInThisBody, answersCommand), because a paint has no way to run a command; the update-loop walk still walks it, and the planted package asserts both halves. No allowlist entry was added for the draft record: the frame does not reach it, and frameDiskDoors still names one door." + - "`errandHomeDir` was believed to be off the frame. It was on it, on every paint, through `a.composerOpensAt → a.errandPlace`, and #898's wider reach is what showed it. The answer is now read ONCE at `open` and held on `app.errandHome`, which both readers take; the function keeps its `os.Getwd` fallback and its absolute answer, because that answer is persisted into a standing item's own `Workspace` and matched by `filepath.Clean` in standing's `Store.ForWorkspace` — `.` there would lose every machine-wide item written by a run that had a home directory, and would draw as a literal `.` where the composer's line draws `~`." +--- diff --git a/internal/tui3/app.go b/internal/tui3/app.go index dd184fff5e..5807c330bd 100644 --- a/internal/tui3/app.go +++ b/internal/tui3/app.go @@ -838,6 +838,19 @@ type app struct { file string build string resumed bool + // errandHome is the person's own home directory, resolved ONCE at `open` and + // held: the `~` project an item that belongs to no repository runs in + // ([app.errandPlace], [app.readBareBands], and homeexchange.go's + // [errandHomeDir] is where it comes from). + // + // IT IS A FIELD BECAUSE THE FRAME ASKS FOR IT. The composer's own line names + // where a send lands on every paint — `app.View → … → a.composerOpensAt → + // a.errandPlace` — and the two syscalls behind the answer are two syscalls + // per frame, which is the law in framedisk_law_test.go. A process does not + // change its home directory or the directory it was started in while it runs, + // so one reading at `open` is the whole of what there is to know: this is + // learned.go's shape for a fact with one name rather than a file per name. + errandHome string // previews holds the pictures this surface has already drawn as half blocks // (imagepreview.go), keyed by the file, its mtime and the shape it was drawn // for. An open picture call is re-rendered on every frame, and decoding a @@ -2689,6 +2702,10 @@ func newApp(ctx context.Context, opts Options) *app { // named once, here, and driven by name nowhere afterwards. a.pictures = newLearned(statPictureFile) a.modelLists = newLearned(readModelCacheName) + // AND THE HOME DIRECTORY IS ONE OF THEM, with one name rather than a file per + // name: the frame names where an errand with no project of its own lands, and + // the answer is a fact about the process and not about the frame asking. + a.errandHome = errandHomeDir() a.learning = []memo{&a.pictures, &a.modelLists} // AND THE MODEL CACHES ARE READ HERE, at `open`, before the shelf below asks // the memo for any of them (models.go's [app.learnModelLists]). diff --git a/internal/tui3/framedisk_law_test.go b/internal/tui3/framedisk_law_test.go index 96aaac9491..478c9d09db 100644 --- a/internal/tui3/framedisk_law_test.go +++ b/internal/tui3/framedisk_law_test.go @@ -16,10 +16,25 @@ package tui3 // directory or touches the network. // // WHAT IT WALKS. The roots are the frame's own doors — [app.View] and everything -// it composes. The edges are three: +// it composes. The edges are four: // // - a call on the SURFACE ITSELF — `a.legend(width)` inside another [app] // method — resolved to that method on that receiver type; +// - a call on the SURFACE HANDED IN AS A PARAMETER — the `a` of +// `placeFrameWithBar(a *app, …)` — which is the same edge, resolved the same +// way. It was the biggest hole this law had until #898: the walk made a +// surface edge out of `x.method()` only where `x` was spelled the same as the +// enclosing method's own receiver, so the body of `placeFrameWithBar(a, …)` +// was walked — it is a package function — while every `a.…` call INSIDE it +// was read as a call on some other value and dropped. The reviewer of #875 +// traced the live one: `app.View → app.placeDraw → placeFrameWithBar(a, …)` +// ⊘ `a.composerRows → a.composerWhereLine → a.composerWhere → +// a.composerOpensAt → a.errandPlace → errandHomeDir → os.Getwd`, a call in +// this file's own forbidden set that this file could not see. [surfaceNames] +// gives a body's receiver and its `*app` parameters as ONE list, THE TYPE +// DECIDES which names are on it, and +// [TestTheLawSeesEveryShapeOfIndirectionItClaims] plants the shape so that +// deleting the branch turns it red; // - a call on no receiver at all — `renderPicture(…)`, `fit(…)` — resolved to // the package function of that name; // - a func-typed FIELD, through whatever function this package assigns to it. @@ -30,20 +45,32 @@ package tui3 // WHAT IT DOES NOT WALK, SAID OUT LOUD, because a law whose reach a reader // cannot predict is a law that lies by omission: // -// - A PACKAGE FUNCTION THAT TAKES THE SURFACE AS A PARAMETER, which is the -// biggest hole in this law and the one to close next. The walk makes a -// surface edge out of `x.method()` only where `x` is spelled the same as the -// enclosing method's own receiver, so the body of `placeFrameWithBar(a, …)` -// is walked — it is a package function — while every `a.…` call INSIDE it is -// read as a call on some other value and dropped. The reviewer of this PR -// traced a live one: `app.View → app.placeDraw → placeFrameWithBar(a, …)` ⊘ -// `a.composerRows → a.composerWhereLine → a.composerWhere → -// a.composerOpensAt → a.errandPlace → errandHomeDir → os.Getwd`, a call in -// this file's own forbidden set that this file cannot see. Closing it is -// about eight lines — treat a parameter of type `*app` as a second receiver -// name — and it is issue #898 rather than done here because it will turn -// things up, and a law that grows an allowlist in the same commit that grows -// its reach teaches nobody anything. +// - A CLOSURE THE PAINT FILES FOR A PRESS. The frame is walked INTO the +// literals it builds, because a paint runs what it builds — but a literal +// that ANSWERS A `tea.Cmd` is not one of them. Nothing in a paint can run a +// command: [app.View] returns a string, and a row that files `verb{do: +// func() tea.Cmd{…}}` ([verb], and homephone's `do func(a *app) tea.Cmd`) is +// handing the LOOP something to run when a key is pressed. Walking those +// made the frame's graph the whole surface's, and #898's wider reach is what +// showed it: `a.homeCrossChord` asks a running row for its verbs in order to +// NAME them on the hint line, the stop verb's closure calls `a.closeHome`, +// and from there the walk reached navigation, `app.stowDrafts` and a draft +// record read off the disk — a call no paint has ever made. A fictional call +// chain is worse than a missing one (the local rule below says the same of +// names), and the press is not missed by this file: it runs on the loop, +// where [TestTheUpdateLoopStartsNoProcessOfItsOwn] walks it, and the planted +// package asserts both halves. +// - A VARIADIC SURFACE — `func f(as ...*app)`. [appPointer] reads `*app` and +// not `...*app`, and reading it would buy nothing: a call inside such a body +// is spelled `as[0].draw()` or on a range variable, and neither is a name +// the walk could resolve back to the parameter. Nothing in this package is +// written that way. +// - A `*app` PARAMETER ON A FUNC LITERAL THE SEAM READER FILED. Such a literal +// is filed with its body and no signature at all, because there is nothing +// to know about it but what it does; [surfaceNames] returns early rather +// than pretend otherwise. A literal walked INLINE is walked with the names +// the body around it spells the surface by, which is the truth about a +// closure. // - A METHOD ON SOME OTHER VALUE the frame happens to hold — `p.rows()`, // `e.word()`, `r.ref()`. Following those by name alone drags in every // same-named method in an eleven-thousand-line package and turns this law @@ -72,13 +99,28 @@ package tui3 // is what would see it, and this law is deliberately not one. // - `go a.method()` is walked as though it were on the loop, while // `go func(){ a.method() }()` is not. That is strict rather than loose, so it -// costs a false positive and never a miss. +// costs a false positive and never a miss. A PARAMETER SHADOWED INSIDE ITS +// OWN BODY — `for _, a := range …` written in a `func f(a *app)` — is the +// same trade: the walk reads `a.x()` as the surface's, which is a false +// positive and not a hole, and it is the trade the receiver name has always +// made. // // THE ALLOWLIST IS THE INTERESTING PART OF THIS FILE. Every name on it is a // MEMOISED DOOR: it reads the disk once per file per epoch and the frame reads // its memo on every frame after, with `open`, the pulse beat or the write that // changed the bytes refreshing it. Adding a name here is a claim that the read // happens once and not per frame, and the claim is written down beside the name. +// +// AND A LAW THAT GROWS ITS ALLOWLIST IN THE SAME CHANGE THAT GROWS ITS REACH +// TEACHES NOBODY ANYTHING. #898 widened the walk; the wider walk found two +// things, and both were answered in the code rather than here — the home +// directory an errand opens at is read once at `open` and held +// ([app.errandHome]), and the press closure that made a draft record look like a +// paint's read is not the paint's at all. A name that goes on this map has to be +// keyed at the door whose own contract is the once-per-epoch one, never at the +// leaf it happens to call: [readDraftKeep] is a bare os.ReadFile with six +// callers and only one memo among them, so exempting it would have exempted the +// five and every caller written after. import ( "go/ast" @@ -221,6 +263,52 @@ func receiverName(fn *ast.FuncDecl) string { return fn.Recv.List[0].Names[0].Name } +// surfaceNames is every name one body spells the surface by, each mapped to the +// receiver type its edges resolve to — ONE LIST, not two spellings of the same +// edge. A method contributes its own receiver name, and a PACKAGE FUNCTION THAT +// TAKES THE SURFACE AS A PARAMETER contributes every parameter typed `*app`, +// which is the shape #898 is about: `placeFrameWithBar(a, …)` is not the method +// `app.placeFrameWithBar` and spells the surface `a` nowhere its receiver would, +// yet its whole body draws the frame, so a call on that `a` is the node `app.…` +// and not an edge dropped on the floor. +// +// THE TYPE DECIDES AND THE SPELLING ONLY KEYS THE LOOKUP. A parameter named `a` +// of any other type contributes nothing, so a helper that happens to call its +// own value `a` cannot manufacture a surface edge out of a shared string. A +// func literal the seam reader filed is skipped: it has a body and no signature +// at all, because there is nothing to know about it but what it does. +func surfaceNames(fn *ast.FuncDecl) map[string]string { + names := map[string]string{} + if recv := receiverName(fn); recv != "" { + names[recv] = receiverType(fn) + } + if fn.Type == nil || fn.Type.Params == nil { + return names + } + for _, field := range fn.Type.Params.List { + if !appPointer(field.Type) { + continue + } + for _, name := range field.Names { + if name.Name != "_" { + names[name.Name] = "app" + } + } + } + return names +} + +// appPointer is whether one expression is the surface by the only fact that +// settles it — its TYPE, never the spelling of the name that holds it. +func appPointer(expr ast.Expr) bool { + star, ok := expr.(*ast.StarExpr) + if !ok { + return false + } + ident, ok := star.X.(*ast.Ident) + return ok && ident.Name == "app" +} + func newSurfaceGraph() *surfaceGraph { return &surfaceGraph{fset: token.NewFileSet(), decls: map[string]*ast.FuncDecl{}, seams: map[string][]string{}} } @@ -406,21 +494,66 @@ func (g *surfaceGraph) readSeams(file *ast.File) { }) } +// answersCommand is whether one literal hands back a `tea.Cmd` — the one fact +// that settles whether A PAINT COULD HAVE RUN IT, and it is settled by the +// literal's own declared result and never by the field it is filed in or the +// name it is given. +// +// A COMMAND IS THE LOOP'S GRAMMAR AND THE PAINT HAS NO VERB FOR IT. [app.View] +// answers a string; a command is a thing bubbletea runs and delivers a message +// for, so a literal typed to answer one is by construction something this body +// FILED rather than something it ran — `verb{do: func() tea.Cmd{…}}` is the +// shape, and the head of this file says what walking it cost. +func answersCommand(lit *ast.FuncLit) bool { + if lit.Type == nil || lit.Type.Results == nil { + return false + } + for _, result := range lit.Type.Results.List { + sel, ok := result.Type.(*ast.SelectorExpr) + if !ok { + continue + } + if pkg, ok := sel.X.(*ast.Ident); ok && pkg.Name == "tea" && sel.Sel.Name == "Cmd" { + return true + } + } + return false +} + +// runsInThisBody is whether one literal written inside a body is RUN by that +// body, which is the whole of what decides whether its calls are that body's. +// +// THE TWO WALKS ASK IT FROM OPPOSITE SIDES. The paint runs everything it builds +// — a `draw := func(…)` local is the paint — and the one thing it cannot run is +// a command ([answersCommand]). The loop is the other way round: it runs what it +// builds EXCEPT what it hands somebody else to run ([handedOff]) — a tea.Cmd it +// returns, a closure given to guard.Go — because that is how this surface gets +// work off the loop at all. +func runsInThisBody(lit *ast.FuncLit, paint bool, off map[*ast.FuncLit]bool) bool { + if paint { + return !answersCommand(lit) + } + return !off[lit] +} + // callsIn is every name one body reaches, and every forbidden call it makes -// itself. intoClosures says whether EVERY function literal counts as part of -// this body — it does for the frame, which runs everything it builds — and when -// it is false the only literals skipped are the ones handed off to be run -// elsewhere ([handedOff]). +// itself. intoClosures says whether this is THE PAINT — which runs everything it +// builds but a command — and when it is false the only literals skipped are the +// ones handed off to be run elsewhere ([runsInThisBody] holds both rules). func (g *surfaceGraph) callsIn(fn *ast.FuncDecl, forbidden map[string]map[string]bool, intoClosures bool) (names []string, bad []string) { - self, recv := receiverName(fn), receiverType(fn) + // A PACKAGE FUNCTION MAY TAKE THE SURFACE RATHER THAN BE ONE. [surfaceNames] + // gives the receiver and every `*app` parameter as ONE list, so an edge + // through either resolves to the same declaration — the head of this file + // says why leaving the parameter out was the law's biggest hole. + surface := surfaceNames(fn) off := handedOff(fn.Body) commands := commandNames(fn.Body) var walk func(ast.Node) walk = func(node ast.Node) { ast.Inspect(node, func(n ast.Node) bool { if lit, ok := n.(*ast.FuncLit); ok { - if intoClosures || !off[lit] { + if runsInThisBody(lit, intoClosures, off) { walk(lit.Body) } return false @@ -455,8 +588,12 @@ func (g *surfaceGraph) callsIn(fn *ast.FuncDecl, forbidden map[string]map[string return true } // A CALL ON THE SURFACE ITSELF IS AN EDGE; a call on some other - // value it is holding is not (this file's head says why). - if self != "" && ident.Name == self { + // value it is holding is not (this file's head says why). The + // surface arrives two ways — the method's own receiver name, and a + // parameter this function declared of type `*app` — and both are + // one list, so both key their edges on the type the name was + // declared with rather than on how the name is spelled. + if recv, onSurface := surface[ident.Name]; onSurface { names = append(names, nodeName(recv, fun.Sel.Name)) // AND IT MAY BE A FUNC-TYPED FIELD RATHER THAN A METHOD, which // is the same call to read and a different thing to resolve. @@ -565,14 +702,28 @@ import ( "os" "os/exec" + tea "github.com/charmbracelet/bubbletea" + "github.com/Agent-Field/aforge-v2/internal/guard" ) type app struct{ probe func(); door func() } +// A row's verb: a word the paint DRAWS and a closure the loop RUNS on a press. +type verb struct{ do func() tea.Cmd } + +func pressTarget() { os.Stat("pressed") } +func loopPressTarget() { exec.Command("pressloop").Run() } + // A field assigned a function: a.gitProbe = gitHead. func fieldTarget() { os.Stat("field") } +// A package function that takes the surface as a PARAMETER — placeFrameWithBar(a, +// …). The sin lives behind the parameter, so nothing but a walk that treats an +// *app parameter as a second spelling of the receiver ever reaches it. +func planted(a *app) { a.draw() } +func (a *app) draw() { os.Stat("param") } + // A package-level var holding a function: var processOpener = startOpener. var viaVar = varTarget func varTarget() { exec.Command("var").Run() } @@ -590,6 +741,9 @@ func (a *app) View() { a.door() inlineLiteral() a.other() + planted(a) + // The paint FILES this and presses nothing. + _ = verb{do: func() tea.Cmd { pressTarget(); return nil }} } // The constructor that installs the door, as newApp installs every Options seam. @@ -605,6 +759,9 @@ func (a *app) Update() { guard.Go("x", func() { exec.Command("elsewhere").Run() }) run := func() { exec.Command("inline").Run() } run() + // AND THE PRESS THE PAINT FILED IS THE LOOP'S: the same shape the frame may + // skip is walked here, because this is where it is pressed. + _ = verb{do: func() tea.Cmd { loopPressTarget(); return nil }} } ` g := newSurfaceGraph() @@ -619,6 +776,7 @@ func (a *app) Update() { {"a field assigned a function", ".fieldTarget"}, {"a package-level var holding a function", ".varTarget"}, {"a composite-literal key", ".literalKeyTarget"}, + {"a package function taking the surface as a parameter", "app.draw"}, } { if len(frame[want.node]) == 0 { t.Errorf("the walk is blind to %s: %s was not reached", want.shape, want.node) @@ -636,6 +794,13 @@ func (a *app) Update() { if _, fictional := frame[".unrelated"]; fictional { t.Error("a local closure lent its body to another function's local of the same name") } + // AND THE PRESS IS NOT THE PAINT'S. A closure typed to answer a tea.Cmd is + // filed by the draw and run by the loop, so counting it as the frame's makes + // every verb's whole action a per-frame call — which is how a draft record on + // disk came to look like a read a paint took (this file's head). + if _, pressed := frame[".pressTarget"]; pressed { + t.Error("the frame walked a closure the paint files for a press, as though a draw ran it") + } for node, sins := range frame { for _, sin := range sins { if strings.Contains(sin, "fictional") || strings.Contains(node, "unrelated") { @@ -656,11 +821,12 @@ func (a *app) Update() { return "" } loop := g.reach([]string{"app.Update"}, processForbidden, nil, false) - inline, elsewhere := false, false + inline, elsewhere, press := false, false, false for _, sins := range loop { for _, sin := range sins { inline = inline || strings.Contains(sin, lineOf(`"inline"`)) elsewhere = elsewhere || strings.Contains(sin, lineOf(`"elsewhere"`)) + press = press || strings.Contains(sin, lineOf(`"pressloop"`)) } } if !inline { @@ -669,4 +835,10 @@ func (a *app) Update() { if elsewhere { t.Error("the update law walked a closure handed to guard.Go, which runs elsewhere") } + // THE PRESS THE FRAME MAY SKIP IS THE LOOP'S, and this is the half that makes + // that skip honest rather than a hole: the same `func() tea.Cmd` the paint + // only filed is walked here, where it is pressed. + if !press { + t.Errorf("the update law skipped a press closure, which the loop is where it runs: %v", loop) + } } diff --git a/internal/tui3/homeexchange.go b/internal/tui3/homeexchange.go index 22cd7c8795..8ca737a2a7 100644 --- a/internal/tui3/homeexchange.go +++ b/internal/tui3/homeexchange.go @@ -1116,7 +1116,7 @@ func (a *app) errandPlace() (workspace, bucket string) { return path, here } } - return errandHomeDir(), a.home.bucket + return a.errandHome, a.home.bucket } // errandBucketOf is the bucket one workspace's conversations are filed in, and @@ -1150,7 +1150,19 @@ func (a *app) errandWorkspaceOf(dir string) string { // errandHomeDir is the `~` project: a reminder belongs to no repository, and // the person's own home directory is where a machine-wide item's work runs // ([standing.Item.Workspace] says the same). A process with no home directory -// falls back to where it is standing, for the reason internal/home's Dir does. +// falls back to where it is standing, which is the last ABSOLUTE answer there +// is — and the answer has to be absolute, because it is written into an item's +// own `Workspace` and read back by another process with another working +// directory. `.` would match only the items some other run had also written `.` +// for, so it would lose the machine-wide ones ([standing.Store.ForWorkspace] +// compares the two cleaned paths), and it would draw as a literal `.` where the +// composer's line draws `~`. +// +// IT IS READ ONCE, AT `open`, AND HELD ON [app.errandHome]. The frame reaches +// the answer — `a.composerOpensAt` names where a send lands on every paint — +// and a syscall there is one syscall per frame, which is the law in +// framedisk_law_test.go. This function is what `open` calls to learn the fact; +// nothing a paint can reach calls it, and the law is what says so. func errandHomeDir() string { if dir, err := os.UserHomeDir(); err == nil && strings.TrimSpace(dir) != "" { return dir diff --git a/internal/tui3/homestanding.go b/internal/tui3/homestanding.go index 991d237dcc..b2a14dd63b 100644 --- a/internal/tui3/homestanding.go +++ b/internal/tui3/homestanding.go @@ -1132,7 +1132,7 @@ func (a *app) standBandsOf(world session.World) (bands map[string][]StandingItem func (a *app) readBareBands(bands map[string][]StandingItemView, known map[string]bool) ([]homeBare, []StandingItemView) { var out []homeBare var fired []StandingItemView - paths := []string{errandHomeDir(), strings.TrimSpace(a.workspace)} + paths := []string{a.errandHome, strings.TrimSpace(a.workspace)} if a.hosted() { // The home directory belongs to the surface machine and is not a path the // far standing store can answer. Until Standing.All can enumerate bare