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

5 files changed, 95 insertions, 4 deletions

diff --git a/sc/inc/recursionhelper.hxx b/sc/inc/recursionhelper.hxx
index 10c4ddc6f02e..48508ec61e64 100644
--- a/sc/inc/recursionhelper.hxx
+++ b/sc/inc/recursionhelper.hxx
@@ -63,6 +63,7 @@ class ScRecursionHelper
     bool                                bInIterationReturn;
     bool                                bConverging;
     bool                                bGroupsIndependent;
+    bool                                bAbortingDependencyComputation;
     std::vector< ScFormulaCell* >       aTemporaryGroupCells;
     o3tl::sorted_vector< ScFormulaCellGroup* >* pFGSet;
 
@@ -76,8 +77,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; }
@@ -115,6 +116,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 ae5bf3c0b132..2b5b0ba73cc5 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 1b23ff713ce5..3ca9a2892357 100644
--- a/sc/source/core/data/column4.cxx
+++ b/sc/source/core/data/column4.cxx
@@ -1677,8 +1677,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 448c778c8995..e20a147ef528 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -1528,6 +1528,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;
 
@@ -1545,6 +1550,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;
     }
 
@@ -1955,6 +1964,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 efbf924ab853..d50fca1d87f2 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 );