failed cell dependency check should not set invalid values (tdf#132451)
Calc's dependency check done before parallel formula cell group
calculation tries to ensure valid cell values for all the dependencies
of the group's cell, and if it detects a problem such as a cycle
it bails out. But since ScFormulaCell::Interpret() simply bailed out
without doing anything, other cells could use that cell's possibly
incorrect value for their calculation and get their dirty flag reset.
This fix adds a flag to mark that bailing out is in progress, which
ensures the bail-out is short-circuited and no cell values are set.
Change-Id: Ia93c70d456682e19ce533abd2cf65ce35ffed9ca
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96838
Reviewed-by: Dennis Francis <dennis.francis@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 82803ef4736fbed89dd8ae0723f2c4f30e37ba8e)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96801
(cherry picked from commit afb6dd43af5d1179c2e3cd8f00794cd4c3d75b2d)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/97493
Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org>
diff --git a/sc/inc/recursionhelper.hxx b/sc/inc/recursionhelper.hxx
index 9988a67..477eb2a 100644
--- a/sc/inc/recursionhelper.hxx
+++ b/sc/inc/recursionhelper.hxx
@@ -64,6 +64,7 @@ class ScRecursionHelper
bool bInIterationReturn;
bool bConverging;
bool bGroupsIndependent;
bool bAbortingDependencyComputation;
std::vector< ScFormulaCell* > aTemporaryGroupCells;
std::unordered_set< ScFormulaCellGroup* >* pFGSet;
@@ -77,8 +78,8 @@ public:
void IncRecursionCount() { ++nRecursionCount; }
void DecRecursionCount() { --nRecursionCount; }
sal_uInt16 GetDepComputeLevel() const { return nDependencyComputationLevel; }
void IncDepComputeLevel() { ++nDependencyComputationLevel; }
void DecDepComputeLevel() { --nDependencyComputationLevel; }
void IncDepComputeLevel();
void DecDepComputeLevel();
/// A pure recursion return, no iteration.
bool IsInRecursionReturn() const { return bInRecursionReturn &&
!bInIterationReturn; }
@@ -116,6 +117,11 @@ public:
bool AnyCycleMemberInDependencyEvalMode(ScFormulaCell* pCell);
bool AnyParentFGInCycle();
void SetFormulaGroupDepEvalMode(bool bSet);
// When dependency computation detects a cycle, it may not compute proper cell values.
// This sets a flag that ScFormulaCell will use to avoid setting those new values
// and resetting the dirty flag, until the dependency computation bails out.
void AbortDependencyComputation();
bool IsAbortingDependencyComputation() const { return bAbortingDependencyComputation; }
void AddTemporaryGroupCell(ScFormulaCell* cell);
void CleanTemporaryGroupCells();
diff --git a/sc/qa/unit/parallelism.cxx b/sc/qa/unit/parallelism.cxx
index 9af1daa..7bd370d 100644
--- a/sc/qa/unit/parallelism.cxx
+++ b/sc/qa/unit/parallelism.cxx
@@ -48,6 +48,7 @@ public:
void testFormulaGroupWithForwardSelfReference();
void testFormulaGroupsInCyclesAndWithSelfReference();
void testFormulaGroupsInCyclesAndWithSelfReference2();
void testFormulaGroupsInCyclesAndWithSelfReference3();
CPPUNIT_TEST_SUITE(ScParallelismTest);
CPPUNIT_TEST(testSUMIFS);
@@ -66,6 +67,7 @@ public:
CPPUNIT_TEST(testFormulaGroupWithForwardSelfReference);
CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference);
CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference2);
CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference3);
CPPUNIT_TEST_SUITE_END();
private:
@@ -979,6 +981,58 @@ void ScParallelismTest::testFormulaGroupsInCyclesAndWithSelfReference2()
m_pDoc->DeleteTab(0);
}
void ScParallelismTest::testFormulaGroupsInCyclesAndWithSelfReference3()
{
sc::AutoCalcSwitch aACSwitch(*m_pDoc, false);
m_pDoc->InsertTab(0, "1");
m_pDoc->SetValue(1, 1, 0, 2.0); // B2 <== 2
for (size_t nRow = 1; nRow < 105; ++nRow)
{
// Formula-group in B3:B104 with first cell "=D2+0.001"
if( nRow != 1 )
m_pDoc->SetFormula(ScAddress(1, nRow, 0), "=D" + OUString::number(nRow) + "+0.001",
formula::FormulaGrammar::GRAM_NATIVE_UI);
// Formula-group in C2:C104 with first cell "=B2*1.01011"
m_pDoc->SetFormula(ScAddress(2, nRow, 0), "=B" + OUString::number(nRow + 1) + "*1.01011",
formula::FormulaGrammar::GRAM_NATIVE_UI);
// Formula-group in D2:C104 with first cell "=C2*1.02"
m_pDoc->SetFormula(ScAddress(3, nRow, 0), "=C" + OUString::number(nRow + 1) + "*1.02",
formula::FormulaGrammar::GRAM_NATIVE_UI);
}
m_xDocShell->DoHardRecalc();
// What happens with tdf#132451 is that the copy&paste C6->C5 really just sets the dirty flag
// for C5 and all the cells that depend on it (D5,B6,C6,D6,B7,...), and it also resets
// flags marking the C formula group as disabled for parallel calculation because of the cycle.
m_pDoc->SetFormula(ScAddress(2, 4, 0), "=B5*1.01011", formula::FormulaGrammar::GRAM_NATIVE_UI);
m_pDoc->GetFormulaCell(ScAddress(2,4,0))->GetCellGroup()->mbPartOfCycle = false;
m_pDoc->GetFormulaCell(ScAddress(2,4,0))->GetCellGroup()->meCalcState = sc::GroupCalcEnabled;
m_pDoc->SetAutoCalc(true);
// Without the fix, getting value of C5 would try to parallel-interpret formula group in B
// from its first dirty cell (B6), which depends on D5, which depends on C5, where the cycle
// would be detected and dependency check would bail out. But the result from Interpret()-ing
// D5 would be used and D5's dirty flag reset, with D5 value incorrect.
m_pDoc->GetValue(2,4,0);
double fExpected[2][3] = {
{ 2.19053373572776, 2.21268003179597, 2.25693363243189 },
{ 2.25793363243189, 2.28076134145577, 2.32637656828489 }
};
for (size_t nCol = 1; nCol < 4; ++nCol)
{
for (size_t nRow = 4; nRow < 6; ++nRow)
{
OString aMsg = "Value at Cell (Col = " + OString::number(nCol) + ", Row = " + OString::number(nRow) + ", Tab = 0)";
ASSERT_DOUBLES_EQUAL_MESSAGE(aMsg.getStr(), fExpected[nRow - 4][nCol - 1], m_pDoc->GetValue(nCol, nRow, 0));
}
}
m_pDoc->DeleteTab(0);
}
CPPUNIT_TEST_SUITE_REGISTRATION(ScParallelismTest);
CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sc/source/core/data/column4.cxx b/sc/source/core/data/column4.cxx
index 9cf5603..7acd4b0 100644
--- a/sc/source/core/data/column4.cxx
+++ b/sc/source/core/data/column4.cxx
@@ -1675,8 +1675,6 @@ static bool lcl_InterpretSpan(sc::formula_block::const_iterator& rSpanIter, SCRO
// if intergroup dependency is found, return early.
if ((mxParentGroup && mxParentGroup->mbPartOfCycle) || !rRecursionHelper.AreGroupsIndependent())
{
// Set pCellStart as dirty as pCellStart may be interpreted in InterpretTail()
pCellStart->SetDirtyVar();
bAllowThreading = false;
return bAnyDirty;
}
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index acfe85b..dff86ec 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -1517,6 +1517,11 @@ bool ScFormulaCell::Interpret(SCROW nStartOffset, SCROW nEndOffset)
ScRecursionHelper& rRecursionHelper = pDocument->GetRecursionHelper();
bool bGroupInterpreted = false;
// The result would possibly depend on a cell without a valid value, bail out
// the entire dependency computation.
if (rRecursionHelper.IsAbortingDependencyComputation())
return false;
if ((mxGroup && !rRecursionHelper.CheckFGIndependence(mxGroup.get())) || !rRecursionHelper.AreGroupsIndependent())
return bGroupInterpreted;
@@ -1534,6 +1539,10 @@ bool ScFormulaCell::Interpret(SCROW nStartOffset, SCROW nEndOffset)
// Reaching here does not necessarily mean a circular reference, so don't set Err:522 here yet.
// If there is a genuine circular reference, it will be marked so when all groups
// in the cycle get out of dependency evaluation mode.
// But returning without calculation a new value means other cells depending
// on this one would use a possibly invalid value, so ensure the dependency
// computation is aborted without resetting the dirty flag of any cell.
rRecursionHelper.AbortDependencyComputation();
return bGroupInterpreted;
}
@@ -1941,6 +1950,12 @@ void ScFormulaCell::InterpretTail( ScInterpreterContext& rContext, ScInterpretTa
}
bRunning = bOldRunning;
// The result may be invalid or depend on another invalid result, just abort
// without updating the cell value. Since the dirty flag will not be reset,
// the proper value will be computed later.
if(pDocument->GetRecursionHelper().IsAbortingDependencyComputation())
return;
// #i102616# For single-sheet saving consider only content changes, not format type,
// because format type isn't set on loading (might be changed later)
bool bContentChanged = false;
diff --git a/sc/source/core/tool/recursionhelper.cxx b/sc/source/core/tool/recursionhelper.cxx
index a13c60e..a7a8abb 100644
--- a/sc/source/core/tool/recursionhelper.cxx
+++ b/sc/source/core/tool/recursionhelper.cxx
@@ -15,6 +15,7 @@ void ScRecursionHelper::Init()
nRecursionCount = 0;
nDependencyComputationLevel = 0;
bInRecursionReturn = bDoingRecursion = bInIterationReturn = false;
bAbortingDependencyComputation = false;
aInsertPos = GetIterationEnd();
ResetIteration();
// Must not force clear aFGList ever.
@@ -195,6 +196,23 @@ void ScRecursionHelper::SetFormulaGroupDepEvalMode(bool bSet)
aInDependencyEvalMode.back() = bSet;
}
void ScRecursionHelper::AbortDependencyComputation()
{
assert( nDependencyComputationLevel > 0 );
bAbortingDependencyComputation = true;
}
void ScRecursionHelper::IncDepComputeLevel()
{
++nDependencyComputationLevel;
}
void ScRecursionHelper::DecDepComputeLevel()
{
--nDependencyComputationLevel;
bAbortingDependencyComputation = false;
}
void ScRecursionHelper::AddTemporaryGroupCell(ScFormulaCell* cell)
{
aTemporaryGroupCells.push_back( cell );
