- commit
- 6c03099a895951ce3844d7178f97887bf1693d6b
- parent
- fb360a2bcfa394efb8b5942f90b0dbfdf97cbc3f
- parent
- 072db59473af5d363a9b0a24dd7cab1d6380077e
- author
- Dami <dami.etoile@gmail.com>
- kind
- fix risk score 2.1 of 3, Jev's estimate the message may not describe the diff
| M | docs/ | 4 | +- |
| M | docs/ | 5 | ++- |
| M | internal/ | 9 | ++--- |
| M | internal/ | 18 | ++++++++++ |
| M | internal/ | 4 | +- |
| M | internal/ | 56 | ++++++++++++++++++++++++++++++ |
| M | internal/ | 44 | ++++++++++++++++++++++++ |
| M | internal/ | 12 | ++++++ |
| M | internal/ | 9 | +++-- |
| M | internal/ | 55 | +++++++++++++++++++++++++++--- |
| M | internal/ | 36 | +++++++++++++++++++ |
| A | internal/ | 2 | + |
| A | internal/ | 2 | + |
| A | internal/ | 2 | + |
@@ -29,7 +29,7 @@ saved and saves them again, byte for byte
2929 ([File fixtures](releasing.md#file-fixtures)).3030 3131 `TestRandomEdits` (`internal/sheet/randedit_test.go`) checks what holds32-between features: from each of a thousand seeds it builds a workbook32+between features: from each of 3,000 seeds it builds a workbook3333 of two sheets and a notebook tab, makes up to 40 edits drawn from the3434 seed (entries, pastes, fills, series, sorts, inserted and deleted rows3535 and columns, formats, borders, merges, moves, notes, rules, names,@@ -43,7 +43,7 @@ scratch), and recalculating everything in place changes no value.
4343 Outputs' rows come from outside the undo history, so the test sends4444 them again after each edit, undo and reopening, as the UI does. A4545 failure names the fewest of the seed's edits that still fail. It takes46-about a second; `-randedit.seeds=20000` runs more, and46+a few seconds; `-randedit.seeds=20000` runs more, and4747 `FuzzRandomEdits` (in `make fuzz`) lets the fuzzer choose the edits.4848 4949 `TestRandomSharedEdits` (`randedit_shared_test.go`) makes the same@@ -70,7 +70,10 @@ was typed first, so a sheet looks the same when opened again. An array that woul
7070 spill into cells its formula reads, `=SORT(B8:D9)` in A9, is a circular7171 dependency and shows `#REF!` too, as are arrays that would spill into7272 each other's inputs, even when one of them, blocked and read as73-`#REF!`, would come out smaller and free the other.73+`#REF!`, would come out smaller and free the other, or is blocked by a74+value only at the size the other's spill gives it. So is an array that75+would spill over a notebook output or linked file whose cells it reads:76+the output would give way, and its `#REF!` free the array again.7477 7578 ## Names in a formula: LET and LAMBDA7679 @@ -298,10 +298,9 @@ func (s *Sheet) pasteMerges(carried []Rect, p pasteLayout) {
298298 if len(carried) == 0 {299299 return300300 }301- next := slices.DeleteFunc(slices.Clone(s.view.merges), func(m Rect) bool {302- _, overlaps := intersectRect(m, p.dst)303- return overlaps304- })301+ // The merges it replaces may reach past it: arrays they blocked302+ // there spill again too.303+ next, span := s.mergesOutside(p.dst)305304 for tr := range p.down {306305 for tc := range p.across {307306 at := Addr{Col: p.dst.From.Col + tc*p.tw, Row: p.dst.From.Row + tr*p.th}@@ -312,7 +311,7 @@ func (s *Sheet) pasteMerges(carried []Rect, p pasteLayout) {
312311 }313312 }314313 }315- s.setMerges(next, p.dst)314+ s.setMerges(next, span)316315 }317316 318317 // moveMerges moves the merges wholly inside src on from to dst on s, as@@ -69,3 +69,21 @@ func TestUnmergeFreesArrayPastTheRange(t *testing.T) {
6969 t.Errorf("H12 once the merge went = %v", v)7070 }7171 }72+73+// A paste carrying merges replaces those it lands over, whole: an array74+// that one blocked past the paste spills again.75+func TestPastedMergeFreesAnArrayPastIt(t *testing.T) {76+ s := New()77+ if err := s.Merge(NewRect(at("A1"), at("A2")), MergeAll); err != nil {78+ t.Fatal(err)79+ }80+ if err := s.Merge(NewRect(at("B14"), at("C14")), MergeAll); err != nil {81+ t.Fatal(err)82+ }83+ s.Set(at("B14"), "=SEQUENCE(2)")84+ wantShown(t, s, map[string]string{"B14": "#REF!"})85+ if _, err := s.Paste(s.Copy(NewRect(at("A1"), at("A2"))), NewRect(at("C13"), at("C14")), false); err != nil {86+ t.Fatal(err)87+ }88+ wantShown(t, s, map[string]string{"B14": "1", "B15": "2"})89+}@@ -26,10 +26,10 @@ import (
2626 // recalculation agreeing with the incremental one);2727 // - recalculates in place, everything at once, to the same values.2828 //29-// go test runs a fixed set of seeds in a second or two; -randedit.seeds29+// go test runs a fixed set of seeds in a few seconds; -randedit.seeds3030 // runs more, and FuzzRandomEdits lets the fuzzer choose the edits.3131 var (32- randSeeds = flag.Int("randedit.seeds", 1000, "seeds TestRandomEdits runs")32+ randSeeds = flag.Int("randedit.seeds", 3000, "seeds TestRandomEdits runs")3333 randSteps = flag.Int("randedit.steps", 40, "edits per seed")3434 )3535 @@ -132,3 +132,59 @@ func (s *Sheet) wakeAllRegions() {
132132 }133133 }134134 }135+136+// table is the cells the region with meta me takes: its table shown or,137+// blocked, the one it needs; none before its rows first arrive.138+func (me *regionMeta) table() (Rect, bool) {139+ switch {140+ case me == nil || !me.has:141+ return Rect{}, false142+ case me.why != "":143+ return me.need, true144+ }145+ return me.written, true146+}147+148+// regionMoved returns the anchors of the arrays over the region's149+// table, before and after it changed, spilling or blocked by a cycle,150+// to compute again: an array reading the table's cells is blocked by151+// it (spillsIntoItself) wherever the table goes, which the cells it152+// reads may not tell it.153+func (s *Sheet) regionMoved(before, after Rect, had bool) []loc {154+ var out []loc155+ for b, sp := range s.spills {156+ if (sp.why == "" || sp.circular) && (had && overlaps(sp.area, before) || overlaps(sp.area, after)) {157+ sp.stale = true158+ out = append(out, loc{s, b})159+ }160+ }161+ return out162+}163+164+// heldByArrays is the region r's table, when an array spilling over it165+// decides whether it shows: shown, or blocked by arrays alone, which it166+// would show without.167+func (s *Sheet) heldByArrays(r Region) (Rect, bool) {168+ me := s.regionMeta[nameKey(r.Name)]169+ t, ok := me.table()170+ if !ok || me.why == "" {171+ return t, ok172+ }173+ if t.To.Row >= MaxRows || t.To.Col >= MaxCols {174+ return Rect{}, false175+ }176+ for a := range s.cells.anyKeysIn(t) {177+ if a == r.At || !s.cells.filledAt(a) {178+ continue179+ }180+ if _, spilled := s.SpillAnchor(a); !spilled || s.cells.derivedAt(a) != slotSpill {181+ return Rect{}, false182+ }183+ }184+ for _, q := range s.regions.list {185+ if q.Name != r.Name && t.Contains(q.At) {186+ return Rect{}, false187+ }188+ }189+ return t, true190+}@@ -101,3 +101,47 @@ func TestPasteIntoARegionsBlankCell(t *testing.T) {
101101 apply(t, s, op)102102 wantShown(t, s, map[string]string{"A1": "#REF!", "A2": "", "B2": "9"})103103 }104+105+// An array that would spill over a region's table whose cells it reads106+// is a circular dependency: spilling, it would block the region and107+// read its #REF!, come out smaller and free it. It shows #REF! and the108+// region its table, whichever came first, and opened again.109+func TestArrayOverRegionItReadsTheCellsOf(t *testing.T) {110+ op := LiveOp{Region: "a", Reset: true, Header: liveRow("x", "y"), Rows: []LiveRow{liveRow("1", "2")}}111+ feeds := map[string]LiveOp{"a": op}112+ want := map[string]string{"I9": "#REF!", "H10": "x", "I11": "2"}113+ for _, arrayFirst := range []bool{false, true} {114+ s := New()115+ sent(t, s, "a", "H10")116+ if !arrayFirst {117+ apply(t, s, op)118+ }119+ s.Set(at("I9"), "=SORT(H9:H11)")120+ if arrayFirst {121+ apply(t, s, op)122+ }123+ feedStale(s.Book(), feeds)124+ if stale := s.Book().StaleOutputs(); len(stale) > 0 {125+ t.Errorf("array first %v: still stale %v", arrayFirst, stale)126+ }127+ wantShown(t, s, want)128+ opened := roundTrip(t, s)129+ feedAll(opened.Book(), feeds)130+ feedStale(opened.Book(), feeds)131+ wantShown(t, opened, want)132+ }133+}134+135+// A region blocked by a formula and an array reading its cells, the formula136+// cleared, is blocked by the array alone, which then finds the cycle.137+func TestRegionFreedButForAnArrayReadingIt(t *testing.T) {138+ op := LiveOp{Region: "a", Reset: true, Header: liveRow("x", "y", "z"), Rows: []LiveRow{liveRow("1", "2", "3")}}139+ s := New()140+ sent(t, s, "a", "F10")141+ s.Set(at("F11"), "=SEQUENCE(1)")142+ s.Set(at("H9"), "=SORT(F6:F10)")143+ apply(t, s, op)144+ s.Set(at("F11"), "")145+ feedStale(s.Book(), map[string]LiveOp{"a": op})146+ wantShown(t, s, map[string]string{"H9": "#REF!", "F10": "x", "H11": "3"})147+}@@ -76,6 +76,9 @@ func (s *Sheet) writeRegions() []loc {
7676 for k, me := range s.regionMeta {7777 if s.regionIndex(k) < 0 {7878 changed = append(changed, s.clearOwned(me, noRect)...)79+ if t, ok := me.table(); ok {80+ changed = append(changed, s.regionMoved(t, noRect, true)...)81+ }7982 delete(s.regionMeta, k)8083 s.wakeAllRegions()8184 for u := range s.wb.nameUsers[nameKey(regionPrefix+k)] {@@ -97,6 +100,9 @@ func (s *Sheet) writeRegions() []loc {
97100 // naming it read #REF! until then, so they're among the cells returned.98101 func (s *Sheet) emptyRegion(r Region, me *regionMeta) []loc {99102 changed := s.clearOwned(me, noRect)103+ if t, ok := me.table(); ok {104+ changed = append(changed, s.regionMoved(t, noRect, true)...)105+ }100106 if me.has {101107 changed = s.regionReaders(r, changed)102108 }@@ -115,6 +121,7 @@ func (s *Sheet) emptyRegion(r Region, me *regionMeta) []loc {
115121 func (s *Sheet) writeTable(r Region, me *regionMeta, header LiveRow, rows []LiveRow, reset bool) []loc {116122 cols, nrows := tableSize(me, header, rows, reset)117123 was, had, shape := me.written, me.has, [2]int{me.rows, me.cols}124+ before, _ := me.table()118125 need := s.tableArea(r, cols, nrows)119126 changed := s.makeRoomAt(r)120127 whole := me.why != "" || me.placed // it may not hold every cell it wrote@@ -160,6 +167,11 @@ func (s *Sheet) writeTable(r Region, me *regionMeta, header LiveRow, rows []Live
160167 for _, c := range changed {161168 s.freedFor(c.a)162169 }170+ // Blocked, what's in its way may have gone but for arrays, which then171+ // find it (heldByArrays).172+ if after, _ := me.table(); !had || after != before || me.why != "" {173+ changed = append(changed, s.regionMoved(before, after, had)...)174+ }163175 if !had || was != me.written || shape != [2]int{me.rows, me.cols} { // moved, or its table changed shape164176 changed = s.regionReaders(r, changed)165177 }@@ -154,6 +154,9 @@ func (s *Sheet) applySpill(a Addr, p pendingSpill) ([]loc, bool) {
154154 changed = s.wb.recheckArrays(loc{s, a})155155 }156156 if why != "" {157+ if !circ && s.wb.blockedMoved(old, area) {158+ changed = append(changed, s.wb.recheckArrays(loc{s, a})...)159+ }157160 changed = append(changed, s.dropSpill(a)...)158161 s.setSpill(a, &spill{area: area, why: why, circular: circ})159162 c.Value = ErrRef@@ -167,9 +170,9 @@ func (s *Sheet) applySpill(a Addr, p pendingSpill) ([]loc, bool) {
167170 changed = append(changed, s.regionsGiveWay(area)...)168171 if old != nil && old.why == "" {169172 changed = append(changed, s.clearSpilled(old.area, area)...)170- if old.area != area {171- changed = append(changed, s.wb.recheckCircular()...)172- }173+ }174+ if old != nil && old.area != area {175+ changed = append(changed, s.wb.recheckCircular()...)173176 }174177 s.setSpill(a, &spill{area: area, arr: p.arr, auto: c.auto})175178 c.Value = p.top@@ -19,7 +19,13 @@ import "slices"
1919 // (regionsGiveWay), so what reads it by name reads #REF! once the array2020 // spills, whatever the array holds: the walk doesn't follow it, or the2121 // array would be blocked or not by whether the region's rows arrived22-// before it spilled.22+// before it spilled. A formula reading the table's cells, though, leads23+// the walk from each of them, those outside area too: spilling empties24+// them, the anchor showing #REF!, which may make the array smaller and25+// free the region, spilling again once it's sent its rows, without26+// end. The table is the one shown or, blocked by arrays alone, the one27+// it needs, so the array is blocked either way (regionMoved has it28+// checked again).2329 func (s *Sheet) spillsIntoItself(a Addr, area Rect) (string, []loc) {2430 if area.From == area.To {2531 return "", nil@@ -30,13 +36,18 @@ func (s *Sheet) spillsIntoItself(a Addr, area Rect) (string, []loc) {
3036 if w.cells(s, area, a, loc{}, true) {3137 return circular(w.origin), nil3238 }39+ for _, r := range s.regions.list {40+ if t, ok := s.heldByArrays(r); ok && overlaps(t, area) && w.cells(s, t, a, loc{}, true) {41+ return "Circular dependency: the array would spill over " + r.Name + "'s table, whose cells its formula reads", nil42+ }43+ }3344 for i := 0; i < len(w.queue); i++ {3445 u := w.queue[i]3546 w.origin, w.parent = w.nodes[i].from, i3647 // An array's cells first, so a cycle through them is found as3748 // one, and the array blocked with it, rather than through the3849 // anchor alone.39- if sp := u.s.spills[u.a]; sp != nil && (sp.why == "" || sp.circular) {50+ if sp := u.s.spills[u.a]; sp != nil && sp.walked() {4051 w.cells(u.s, sp.area, u.a, u, false)4152 }4253 if !w.found {@@ -101,6 +112,25 @@ func (w *Workbook) cycleMoved(circ bool, old *spill) bool {
101112 return false102113 }103114 115+// blockedMoved reports whether the arrays are to be checked again, an116+// array whose spill was old found blocked over area by something other117+// than a cycle: counted as spilling over the cells it needs (walked), it118+// may close or open a cycle through arrays that read them, when it's119+// blocked over other cells, or blocked still, the first time in the120+// recalculation: a formula on the way around a cycle through it may121+// have changed, which the others aren't computed again for, as in122+// cycleMoved.123+func (w *Workbook) blockedMoved(old *spill, area Rect) bool {124+ switch {125+ case old == nil || old.why == "" || old.area != area:126+ return true127+ case !w.spillRechecked:128+ w.spillRechecked = true129+ return true130+ }131+ return false132+}133+104134 // noteCycle counts the array at l found blocked by a cycle, from spilling105135 // or not, in the recalculation.106136 func (w *Workbook) noteCycle(l loc) {@@ -230,13 +260,13 @@ func (w *Workbook) recheckCircular() []loc {
230260 }231261 232262 // forgetSpill drops the anchor at a's spill for good, its formula233-// computing one value or none, returning the cells that changed. When a234-// cycle blocked it, it counted as spilling over the cells it needed, so263+// computing one value or none, returning the cells that changed. When264+// blocked, it counted as spilling over the cells it needed (walked), so235265 // the arrays blocked by a cycle are checked again.236266 func (s *Sheet) forgetSpill(a Addr) []loc {237267 old := s.spills[a]238268 changed := s.dropSpill(a)239- if old != nil && old.circular {269+ if old != nil && old.why != "" {240270 changed = append(changed, s.wb.recheckCircular()...)241271 }242272 return changed@@ -260,3 +290,18 @@ func blockCircular(arrays []loc) []loc {
260290 }261291 return changed262292 }293+294+// walked reports whether spillsIntoItself follows the cells of the295+// array: spilled, or blocked, over the cells it needs, what it would296+// spill over given room. A blocked array whose size follows what it297+// reads may be blocked or free by whether another spills into its298+// inputs, and that other by whether it spills: counted as spilling,299+// the two are blocked together, as when the cycle is found in one go,300+// rather than by the order they were computed in. The cells of an array301+// too big for max-cells aren't walked.302+func (sp *spill) walked() bool {303+ if sp.why == "" || sp.circular {304+ return true305+ }306+ return (sp.area.To.Row-sp.area.From.Row+1)*(sp.area.To.Col-sp.area.From.Col+1) <= MaxCells()307+}@@ -64,3 +64,39 @@ func TestCycleGoneWithAnArray(t *testing.T) {
6464 t.Errorf("C9 opened: %+v", sp)6565 }6666 }67+68+// An array spilling into the inputs of another, blocked by a value but69+// only as wide as it is for the first's spill, and reading what the70+// other needs: counted as spilling, the blocked one closes the cycle,71+// so both are blocked whichever was typed first, as when opened.72+func TestCycleThroughABlockedArray(t *testing.T) {73+ cells := map[string]string{"C9": "1.5", "C13": "=SORT(D19:E20)", "D18": "=SORT(C9:E13)", "E19": "=SEQUENCE(3)"}74+ want := map[string]string{"C13": "#REF!", "D18": "#REF!", "D13": "", "E20": "2"}75+ for _, order := range [][]string{{"C9", "E19", "C13", "D18"}, {"C9", "D18", "C13", "E19"}, {"D18", "C13", "C9", "E19"}} {76+ wantSettled(t, "blocked", order, cells, want)77+ }78+}79+80+// A formula typed last closing a cycle through an array blocked by a81+// value has the arrays checked again, as when the file opens.82+func TestCycleClosedThroughABlockedArray(t *testing.T) {83+ cells := map[string]string{"D9": "=SORT(G15:H19)", "E9": "=COUNTIF(A9:A12,\">2\")", "B16": "=ABS($A$9)+1", "H19": "=SORT(B13:D16)", "H16": "=COUNTIF(H20,\">2\")"}84+ wantSettled(t, "closed", []string{"D9", "E9", "B16", "H19", "H16"}, cells, map[string]string{"D9": "#REF!", "H19": "#REF!"})85+}86+87+// A blocked array that goes, its formula computing one value, frees the88+// arrays whose cycle went through the cells it needed (spill.walked):89+// once Total is defined, B7 reads its own #REF! through G9.90+func TestCycleGoneWithABlockedArray(t *testing.T) {91+ cells := map[string]string{92+ "H9": "=VLOOKUP(G8,G14:G15,1,FALSE)", "E7": "=SORT(F10:G14)", "G9": "=SUM(Total)*2", "B7": "=SORT(G7:H11)",93+ "H5": "=SORT(C4:E8)", "B9": "=VLOOKUP(A13,C14:D18,1,FALSE)", "G18": "=ABS(C14)+1",94+ }95+ s := New()96+ for _, a := range []string{"H9", "E7", "G9", "B7", "H5", "B9"} {97+ s.Set(at(a), cells[a])98+ }99+ s.Book().DefineName("Total", s, NewRect(Addr{}, Addr{Col: 1, Row: 9}))100+ s.Set(at("G18"), cells["G18"])101+ wantShown(t, s, map[string]string{"B7": "#REF!", "H5": ""})102+}