Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

fix(formula): spreadsheet deadlock #458

Merged
merged 4 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/formula/src/__tests__/errorParse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ describe('errorParse', () => {

it.each(testCases.errorTestCases)('$jestTitle', async args => {
const parseResult = ctx.parseDirectly(args)
expect(parseResult.success).toBe(false)
expect([parseResult.errorMessages[0], parseResult.variableParseResult.valid]).toStrictEqual([
expect([parseResult.success, parseResult.errorMessages[0], parseResult.variableParseResult.valid]).toStrictEqual([
false,
{ type: args.errorType, message: args.errorMessage },
args.valid ?? true
])
Expand Down
12 changes: 11 additions & 1 deletion packages/formula/src/context/variable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ export class VariableClass implements VariableInterface {
level: number,
input?: FormulaDefinition
): Promise<void> {
// console.debug(
// console.log(
// `reparse: ${sourceUuid && this.currentUUID === sourceUuid}`,
// { sourceUuid, id: this.id },
// this.t.meta.name,
Expand Down Expand Up @@ -418,6 +418,16 @@ export class VariableClass implements VariableInterface {
}

const parseResult = parse(ctx)
if (parseResult.errorMessages[0]?.type === 'circular_dependency') {
if (
!this.t.task.async &&
this.t.task.variableValue.result.type === 'Error' &&
this.t.task.variableValue.result.result.type === 'circular_dependency'
) {
// Skip circular dependency
return
}
}
const tempT = await interpret({ variable: this, ctx, parseResult })
await this.cleanup()
this.t = tempT
Expand Down
2 changes: 1 addition & 1 deletion packages/formula/src/controls/block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class BlockClass implements BlockType {

const firstArgumentType = fetchResult(variable.t).type

parseTrackVariable(visitor, variable, this.id)
parseTrackVariable(visitor, variable)

let finalCodeFragments = codeFragments

Expand Down
20 changes: 16 additions & 4 deletions packages/formula/src/controls/column.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
import { codeFragments2definition, CodeFragmentVisitor, FormulaInterpreter } from '../grammar'
import { CellClass } from '.'
import { SpreadsheetReloadViaId, SpreadsheetUpdateNamePayload } from '../events'
import { parseTrackCell } from '../grammar/dependency'

export class ColumnClass implements ColumnType {
columnId: ColumnId
Expand Down Expand Up @@ -72,7 +73,11 @@ export class ColumnClass implements ColumnType {
}
}

private findCellByNumber(meta: VariableMetadata, name: string): AnyTypeResult<'Cell' | 'Error'> {
private findCellByNumber(
meta: VariableMetadata,
name: string,
kind: 'parse' | 'interpret'
): AnyTypeResult<'Cell' | 'Error'> {
const number = Number(name)
if (isNaN(number)) {
return { type: 'Error', result: { message: `Need a number: ${name}`, type: 'syntax' } }
Expand All @@ -88,7 +93,13 @@ export class ColumnClass implements ColumnType {
const { spreadsheetId, rowId, columnId } = meta.richType.meta
if (spreadsheetId === this.spreadsheetId && rowId === cell.rowId && columnId === cell.columnId) {
return {
result: { message: 'errors.interpret.circular_dependency.spreadsheet', type: 'circular_dependency' },
result: {
message:
kind === 'parse'
? 'errors.parse.circular_dependency.spreadsheet'
: 'errors.interpret.circular_dependency.spreadsheet',
type: 'circular_dependency'
},
type: 'Error'
}
}
Expand Down Expand Up @@ -133,7 +144,7 @@ export class ColumnClass implements ColumnType {
}

async handleInterpret(interpreter: FormulaInterpreter, name: string): Promise<AnyTypeResult> {
return this.findCellByNumber(interpreter.ctx.meta, name)
return this.findCellByNumber(interpreter.ctx.meta, name, 'interpret')
}

handleCodeFragments(
Expand All @@ -143,7 +154,7 @@ export class ColumnClass implements ColumnType {
): { errors: ErrorMessage[]; firstArgumentType: FormulaType | undefined; codeFragments: CodeFragment[] } {
visitor.eventDependencies.push(this.eventDependency({ rowKey: name }))

const result = this.findCellByNumber(visitor.ctx.meta, name)
const result = this.findCellByNumber(visitor.ctx.meta, name, 'parse')
const errors: ErrorMessage[] = []

if (result.type === 'Error') {
Expand All @@ -156,6 +167,7 @@ export class ColumnClass implements ColumnType {
}

const cell = result.result
parseTrackCell(visitor, cell)
if (visitor.ctx.meta.richType.type === 'spreadsheet') {
const { spreadsheetId, rowId, columnId } = visitor.ctx.meta.richType.meta
if (spreadsheetId === this.spreadsheetId && rowId === cell.rowId && columnId === cell.columnId) {
Expand Down
2 changes: 2 additions & 0 deletions packages/formula/src/controls/row.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { SpreadsheetReloadViaId, SpreadsheetUpdateNamePayload } from '../events'
import { CodeFragmentVisitor, FormulaInterpreter } from '../grammar'
import { parseTrackCell } from '../grammar/dependency'
import {
AnyTypeResult,
CodeFragment,
Expand Down Expand Up @@ -129,6 +130,7 @@ export class RowClass implements RowType {
}

const errors: ErrorMessage[] = []
parseTrackCell(visitor, cell)

if (visitor.ctx.meta.richType.type === 'spreadsheet') {
const { spreadsheetId, rowId, columnId } = visitor.ctx.meta.richType.meta
Expand Down
1 change: 1 addition & 0 deletions packages/formula/src/controls/spreadsheet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export class SpreadsheetClass implements SpreadsheetType {
const changedColumnIds = [
...new Set([...Object.values(pairs1), ...Object.values(pairs2)].flatMap(p => [p.columnId, p.displayIndex]))
]

if (!changedColumnIds.length) return

const result = MashcardEventBus.dispatch(
Expand Down
18 changes: 14 additions & 4 deletions packages/formula/src/grammar/dependency.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { EventType } from '@mashcard/schema'
import { BlockType, ColumnType, SpreadsheetType } from '../controls'
import { BlockType, Cell, ColumnType, SpreadsheetType } from '../controls'
import { SpreadsheetUpdateNameViaId, SpreadsheetReloadViaId, SpreadsheetUpdateNamePayload } from '../events'
import { EventDependency, VariableInterface } from '../type'
import { CodeFragmentVisitor } from './codeFragment'
Expand Down Expand Up @@ -64,10 +64,19 @@ export const parseTrackColumn = (visitor: Visitor, column: ColumnType): void =>
visitor.eventDependencies.push(column.eventDependency({}))
}

export const parseTrackCell = (visitor: Visitor, cell: Cell): void => {
const variable = visitor.ctx.formulaContext.findVariableById(cell.namespaceId, cell.variableId)
if (variable) {
parseTrackVariable(visitor, variable)
} else {
visitor.variableDependencies.push({ namespaceId: cell.namespaceId, variableId: cell.variableId })
}
}

/**
* Track variableDependencies and flattenVariableDependencies when parse variable
*/
export const parseTrackVariable = (visitor: Visitor, variable: VariableInterface, namespaceId: string): void => {
export const parseTrackVariable = (visitor: Visitor, variable: VariableInterface): void => {
if (variable.t.variableParseResult.async) {
visitor.async = true
}
Expand All @@ -80,10 +89,11 @@ export const parseTrackVariable = (visitor: Visitor, variable: VariableInterface
if (!variable.t.variableParseResult.pure) {
visitor.pure = false
}
const { namespaceId, variableId } = variable.t.meta

visitor.variableDependencies.push({ namespaceId, variableId: variable.t.meta.variableId })
visitor.variableDependencies.push({ namespaceId, variableId })
visitor.flattenVariableDependencies.push(...variable.t.variableParseResult.flattenVariableDependencies, {
namespaceId,
variableId: variable.t.meta.variableId
variableId
})
}
4 changes: 2 additions & 2 deletions packages/formula/src/grammar/operations/name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export const nameOperator: OperatorType = {
type: 'Column'
}
case 'Variable':
parseTrackVariable(cstVisitor, result, cstVisitor.ctx.meta.namespaceId)
parseTrackVariable(cstVisitor, result)
return {
codeFragments: [variable2codeFragment(result, cstVisitor.ctx.meta.namespaceId)],
image,
Expand Down Expand Up @@ -198,7 +198,7 @@ export const nameOperator: OperatorType = {
{ definition: '=A', errorMessage: '"A" not found', errorType: 'syntax', namespaceId },
{
definition: '=A.1',
errorMessage: 'errors.interpret.circular_dependency.spreadsheet',
errorMessage: 'errors.parse.circular_dependency.spreadsheet',
errorType: 'circular_dependency',
richType,
namespaceId
Expand Down
3 changes: 1 addition & 2 deletions packages/formula/src/grammar/operations/thisRecord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,7 @@ export const thisRecordOperator: OperatorType = {
{
definition: `=ThisRecord.A.1`,
errorType: 'circular_dependency',
groupOptions: [{ name: 'basicError' }],
errorMessage: 'errors.interpret.circular_dependency.spreadsheet',
errorMessage: 'errors.parse.circular_dependency.spreadsheet',
richType,
namespaceId
},
Expand Down
4 changes: 3 additions & 1 deletion packages/formula/src/tests/feature/event/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { TestCaseInterface, TestCaseName } from '../../testType'
import { BlockEventTestCase } from './blockEvent'
import { ColumnEventTestCase } from './columnEvent'
import { RowEventTestCase } from './rowEvent'
import { SpreadsheetDeadlockEventTestCase } from './spreadsheetDeadlockEvent'
import { SpreadsheetEventTestCase } from './spreadsheetEvent'
import { VariableEventTestCase } from './variableEvent'

Expand All @@ -10,7 +11,8 @@ export const EventTestCases: TestCaseInterface[] = [
SpreadsheetEventTestCase,
VariableEventTestCase,
ColumnEventTestCase,
RowEventTestCase
RowEventTestCase,
SpreadsheetDeadlockEventTestCase
]

export const EventNames: TestCaseName[] = EventTestCases.map(c => c.name)
Loading