diff --git a/formula/source/core/api/FormulaCompiler.cxx b/formula/source/core/api/FormulaCompiler.cxx index aa2d51a8369f..3a3601961414 100644 --- a/formula/source/core/api/FormulaCompiler.cxx +++ b/formula/source/core/api/FormulaCompiler.cxx @@ -2091,41 +2091,6 @@ void FormulaCompiler::Factor() assert(!"FormulaCompiler::Factor: someone forgot to add a jump count case"); } OpCode eFacOpCode = pFacToken->GetOpCode(); - eOp = NextToken(); - if (eOp == ocOpen) - { - NextToken(); - // Optional LAMBDA parameters are surrounded by [] - if (eFacOpCode == ocLambda && mpToken->GetOpCode() == ocTableRefOpen) - { - meLastOp = ocSep; // So that NextToken() doesn't produce an error - eOp = NextToken(); - if (eOp == ocPush && mpToken->GetType() == svStringName) - { - CheckSetForceArrayParameter(mpToken, 0); - static_cast(mpToken.get())->SetIsOptional(true); - PutCode(mpToken); - eOp = NextToken(); - if (eOp == ocTableRefClose) - { - meLastOp = ocPush; // So that NextToken() doesn't produce an error - eOp = NextToken(); - } - else - SetError(FormulaError::PairExpected); - } - else - SetError(FormulaError::ParameterExpected); - } - else - { - CheckSetForceArrayParameter(mpToken, 0); - eOp = Expression(); - } - } - else - SetError( FormulaError::PairExpected); - PutCode( pFacToken ); // During AutoCorrect (since pArr->GetCodeError() is // ignored) an unlimited ocIf would crash because // ScRawToken::Clone() allocates the JumpBuffer according to @@ -2159,42 +2124,114 @@ void FormulaCompiler::Factor() assert(!"FormulaCompiler::Factor: someone forgot to add a jump max case"); } short nJumpCount = 0; - while ( (nJumpCount < (FORMULA_MAXPARAMS - 1)) && (eOp == ocSep) - && (mpArr->GetCodeError() == FormulaError::NONE || !mbStopOnError)) + // ocLambda requires the operator to appear before all subexpressions in RPN + // ocIf, ocIfError, ocIfNA, and ocChoose require the first subexpression + // to appear before the operator. ocLet could be arranged either way, but the + // interpreter expects the first subexpression to appear before the operator. + if (eFacOpCode == ocLambda) { - if ( ++nJumpCount <= nJumpMax ) - static_cast(&*pFacToken)->GetJump()[nJumpCount] = mnPC-1; - NextToken(); - // Optional LAMBDA parameters are surrounded by [] - if (eFacOpCode == ocLambda && mpToken->GetOpCode() == ocTableRefOpen) + PutCode(pFacToken); + if (++nJumpCount <= nJumpMax) + static_cast(&*pFacToken)->GetJump()[ nJumpCount ] = mnPC-1; + + eOp = NextToken(); + if (eOp == ocOpen) { - meLastOp = ocSep; // So that NextToken() doesn't produce an error - eOp = NextToken(); - if (eOp == ocPush && mpToken->GetType() == svStringName) + NextToken(); + // Optional LAMBDA parameters are surrounded by [] + if (mpToken->GetOpCode() == ocTableRefOpen) { - CheckSetForceArrayParameter(mpToken, nJumpCount - 1); - static_cast(mpToken.get())->SetIsOptional(true); - PutCode(mpToken); + meLastOp = ocSep; // So that NextToken() doesn't produce an error eOp = NextToken(); - if (eOp == ocTableRefClose) + if (eOp == ocPush && mpToken->GetType() == svStringName) { - meLastOp = ocPush; // So that NextToken() doesn't produce an error + CheckSetForceArrayParameter(mpToken, 0); + static_cast(mpToken.get())->SetIsOptional(true); + PutCode(mpToken); eOp = NextToken(); + if (eOp == ocTableRefClose) + { + meLastOp = ocPush; // So that NextToken() doesn't produce an error + eOp = NextToken(); + } + else + SetError(FormulaError::PairExpected); } else - SetError(FormulaError::PairExpected); + SetError(FormulaError::ParameterExpected); } else - SetError(FormulaError::ParameterExpected); + { + CheckSetForceArrayParameter(mpToken, 0); + eOp = Expression(); + } } else - { - CheckSetForceArrayParameter(mpToken, nJumpCount - 1); - eOp = Expression(); - } + SetError(FormulaError::PairExpected); // ocSep or ocClose terminate the subexpression PutCode(mpToken); - } + while ( (nJumpCount < (FORMULA_MAXPARAMS - 1)) && (eOp == ocSep) + && (mpArr->GetCodeError() == FormulaError::NONE || !mbStopOnError)) + { + if ( ++nJumpCount <= nJumpMax ) + static_cast(&*pFacToken)->GetJump()[nJumpCount] = mnPC-1; + NextToken(); + // Optional LAMBDA parameters are surrounded by [] + if (mpToken->GetOpCode() == ocTableRefOpen) + { + meLastOp = ocSep; // So that NextToken() doesn't produce an error + eOp = NextToken(); + if (eOp == ocPush && mpToken->GetType() == svStringName) + { + CheckSetForceArrayParameter(mpToken, nJumpCount - 1); + static_cast(mpToken.get())->SetIsOptional(true); + PutCode(mpToken); + eOp = NextToken(); + if (eOp == ocTableRefClose) + { + meLastOp = ocPush; // So that NextToken() doesn't produce an error + eOp = NextToken(); + } + else + SetError(FormulaError::PairExpected); + } + else + SetError(FormulaError::ParameterExpected); + } + else + { + CheckSetForceArrayParameter(mpToken, nJumpCount - 1); + eOp = Expression(); + } + // ocSep or ocClose terminate the subexpression + PutCode(mpToken); + } + } // eFacOpCode == ocLambda + else + { + eOp = NextToken(); + if (eOp == ocOpen) + { + NextToken(); + CheckSetForceArrayParameter(mpToken, 0); + eOp = Expression(); + } + else + SetError(FormulaError::PairExpected); + PutCode(pFacToken); + + while ( (nJumpCount < (FORMULA_MAXPARAMS - 1)) && (eOp == ocSep) + && (mpArr->GetCodeError() == FormulaError::NONE || !mbStopOnError)) + { + if ( ++nJumpCount <= nJumpMax ) + static_cast(&*pFacToken)->GetJump()[nJumpCount] = mnPC-1; + NextToken(); + CheckSetForceArrayParameter(mpToken, nJumpCount - 1); + eOp = Expression(); + // ocSep or ocClose terminate the subexpression + PutCode(mpToken); + } + } // eFacOpCode != ocLambda if (eOp != ocClose) SetError( FormulaError::PairExpected); else diff --git a/sc/README.Lambda.md b/sc/README.Lambda.md index 1a2167b1cbcf..4ca1efcf1215 100644 --- a/sc/README.Lambda.md +++ b/sc/README.Lambda.md @@ -14,7 +14,6 @@ letters, numbers, and underscores (`_`). * You may specify no parameters, if you want the function to operate the same way every time it is called (this may be useful if its expression includes volatile operators such as `RAND` or `TIME`). -FIXME: This is not yet true. * The result of a `LAMBDA` is a function. Don't expect that to show up in the spreadsheet in any useful way! You must call the function to use it. @@ -27,15 +26,16 @@ arguments you want to pass to the function. * If you are nesting one `LAMBDA` inside another, you may use the same parameter name in both (if you really want - this can easily make things hard to read!). The innermost `LAMBDA` specifying a parameter of a given name applies at each -level of scope. +level of scope. This also works with `LET`. * A function can call itself (this is called recursion), but you must be able to refer to it from within the function. For example, if cell `A1` is a `LAMBDA` formula, the body of the `LAMBDA` can call itself by referring to `A1`. You can name your `LAMBDA` cell to make this easier to read. -* If you want your `LAMBDA` function to permit optional parameters, enclose those -parameter names (in the parameter list, not in the body) in brackets, like this: +* If you want your `LAMBDA` function to permit optional parameters, enclose +those parameter names (in the parameter list, not in the body) in brackets, like +this: > =LAMBDA(name1, name2, [name3], [name4], expression) * `ISOMITTED` can be used within a `LAMBDA` body to detect whether an optional @@ -97,9 +97,17 @@ and body. It also looks for any parameters between `ocTableRefOpen`/`ocTableRefClose` (which correspond to brackets), and sets `mIsOptional` for those parameters. +* The `ocLambda` is built in the compiled code as a sequence consisting of the +`ocLambda` token itself, then the parameter names (`svStringName`) separated by +`ocSep`, followed by another `ocSep`, then the lambda body, with `ocClose` at +the end. The jump table points to the `ocLambda`, each `ocSep`, and the +`ocClose`, which allows the interpreter to find each parameter name (the +`ocLambda` and `ocSep`s except the last), the body (the last `ocSep`), and the +position where execution continues (`ocClose`). + * When interpreted, `ocLambda` causes a `ScFormulaFunction` to be built using -the contents of the jump table. At this point the optional parameters are -counted, and that information is passed on to the next step. +the contents of the jump table. At this point all parameters are counted, and +that information is passed on to the next step. * `ScFormulaFunction`'s constructor clones the lambda body and saves the interpreter's contextual information, then locates each use of each parameter in @@ -117,3 +125,6 @@ appear in the body. * The presence of an enclosing `LAMBDA` also prevents circular references from being a problem, as the body of a `LAMBDA` is not executed until the cell containing it already has a value. This allows recursion. + +* `LAMBDA` does not currently check for infinite recursion; avoid recursive +calls that can cause such a loop. diff --git a/sc/qa/unit/ucalc_formula2.cxx b/sc/qa/unit/ucalc_formula2.cxx index 373a007e92ca..3534cf2397e0 100644 --- a/sc/qa/unit/ucalc_formula2.cxx +++ b/sc/qa/unit/ucalc_formula2.cxx @@ -210,9 +210,8 @@ CPPUNIT_TEST_FIXTURE(TestFormula2, testFuncLAMBDA) m_pDoc->InsertTab(0, u"Formula"_ustr); // No parameters, constant body. - // TODO : The formula system does not yet support this. - //m_pDoc->SetString(ScAddress(0, 0, 0), u"=LAMBDA(1)()"_ustr); - //CPPUNIT_ASSERT_EQUAL(1.0, m_pDoc->GetValue(ScAddress(0, 0, 0))); + m_pDoc->SetString(ScAddress(0, 0, 0), u"=LAMBDA(1)()"_ustr); + CPPUNIT_ASSERT_EQUAL(1.0, m_pDoc->GetValue(ScAddress(0, 0, 0))); // One unused parameter, constant body. m_pDoc->SetString(ScAddress(0, 1, 0), u"=LAMBDA(a; 2)(1)"_ustr); @@ -231,7 +230,7 @@ CPPUNIT_TEST_FIXTURE(TestFormula2, testFuncLAMBDA) CPPUNIT_ASSERT_EQUAL(7.0, m_pDoc->GetValue(ScAddress(0, 4, 0))); // Parameter used more than once. - m_pDoc->SetString(ScAddress(0, 5, 0), u"=LAMBDA(a;a+a)(5)"_ustr); + m_pDoc->SetString(ScAddress(0, 5, 0), u"=LAMBDA(a; a + a)(5)"_ustr); CPPUNIT_ASSERT_EQUAL(10.0, m_pDoc->GetValue(ScAddress(0, 5, 0))); // Operator precedence inside the body. @@ -253,13 +252,25 @@ CPPUNIT_TEST_FIXTURE(TestFormula2, testFuncLAMBDA) CPPUNIT_ASSERT_EQUAL(15.0, m_pDoc->GetValue(ScAddress(0, 9, 0))); // String argument flowing through identity body. - m_pDoc->SetString(ScAddress(0, 10, 0), u"=LAMBDA(a;a & \"!\")(\"hi\")"_ustr); + m_pDoc->SetString(ScAddress(0, 10, 0), u"=LAMBDA(a; a & \"!\")(\"hi\")"_ustr); CPPUNIT_ASSERT_EQUAL(u"hi!"_ustr, m_pDoc->GetString(ScAddress(0, 10, 0))); // Name precedence matches MSO. m_pDoc->SetString(ScAddress(0, 11, 0), u"=LAMBDA(SQRT; SQRT(SQRT))(4)"_ustr); CPPUNIT_ASSERT_EQUAL(2.0, m_pDoc->GetValue(ScAddress(0, 11, 0))); + // Parameter shadowing: LAMBDA within LAMBDA. + m_pDoc->SetString(ScAddress(0, 12, 0), u"=LAMBDA(x; LAMBDA(x; x * 2)(3))(10)"_ustr); + CPPUNIT_ASSERT_EQUAL(6.0, m_pDoc->GetValue(ScAddress(0, 12, 0))); + m_pDoc->SetString(ScAddress(0, 13, 0), u"=LAMBDA(x; LAMBDA(y; x; x * 2)(5; 3))(10)"_ustr); + CPPUNIT_ASSERT_EQUAL(6.0, m_pDoc->GetValue(ScAddress(0, 13, 0))); + + // Parameter shadowing: LET within LAMBDA. + m_pDoc->SetString(ScAddress(0, 14, 0), u"=LAMBDA(x; LET(x; 3; x * 2))(10)"_ustr); + CPPUNIT_ASSERT_EQUAL(6.0, m_pDoc->GetValue(ScAddress(0, 14, 0))); + m_pDoc->SetString(ScAddress(0, 15, 0), u"=LAMBDA(x; LET(y; 5; x; 3; x * 2))(10)"_ustr); + CPPUNIT_ASSERT_EQUAL(6.0, m_pDoc->GetValue(ScAddress(0, 15, 0))); + m_pDoc->DeleteTab(0); } diff --git a/sc/source/core/inc/interpre.hxx b/sc/source/core/inc/interpre.hxx index ba78c3abc5f2..9e32b49a6507 100644 --- a/sc/source/core/inc/interpre.hxx +++ b/sc/source/core/inc/interpre.hxx @@ -401,7 +401,6 @@ private: void PopError(); double PopDouble(); const svl::SharedString & PopString(); - const svl::SharedString & PopStringName( bool& rIsOptional ); void ValidateRef( const ScSingleRefData & rRef ); void ValidateRef( const ScComplexRefData & rRef ); void ValidateRef( const ScRefList & rRefList ); diff --git a/sc/source/core/tool/callable.cxx b/sc/source/core/tool/callable.cxx index 76208d5b701b..e746f08ce0a4 100644 --- a/sc/source/core/tool/callable.cxx +++ b/sc/source/core/tool/callable.cxx @@ -176,8 +176,7 @@ static std::forward_list lcl_FindReplacementPositions(std::u16string_view { short nNextOp = aIter.GetPC() + 1; if (!(nNextOp < rTokens.GetCodeLen() - && (rTokens.GetCode()[nNextOp]->GetOpCode() == ocLet - || rTokens.GetCode()[nNextOp]->GetOpCode() == ocLambda))) + && rTokens.GetCode()[nNextOp]->GetOpCode() == ocLet)) { if (pStringNameToken->GetString().getString().equalsIgnoreAsciiCase(aStrName)) aPositions.push_front(aIter.GetPC()); @@ -206,7 +205,7 @@ static std::forward_list lcl_FindReplacementPositions(std::u16string_view { aStartPoints.push(pJump[nJump + 1]); pSubToken = formula::GetStringNameToken( - rTokens.GetCode()[pJump[nJump]]); + rTokens.GetCode()[pJump[nJump] + 1]); if (pSubToken && pSubToken->GetString().getString().equalsIgnoreAsciiCase( aStrName)) @@ -223,25 +222,22 @@ static std::forward_list lcl_FindReplacementPositions(std::u16string_view break; case ocLambda: { - auto pSubToken - = formula::GetStringNameToken(rTokens.GetCode()[aIter.GetPC() - 1]); - if (!(pSubToken && pSubToken->GetString().getString() == aStrName)) + bool bFound = false; + for (short nJump = 1; nJump < nJumpCount - 1; ++nJump) { - bool bFound = false; - for (short nJump = 1; nJump < nJumpCount - 1; ++nJump) + auto pSubToken = formula::GetStringNameToken( + rTokens.GetCode()[pJump[nJump] + 1]); + if (pSubToken + && pSubToken->GetString().getString().equalsIgnoreAsciiCase( + aStrName)) { - pSubToken = formula::GetStringNameToken( - rTokens.GetCode()[pJump[nJump]]); - if (pSubToken && pSubToken->GetString().getString() == aStrName) - { - // this name shadows the one we're looking for, in the body - bFound = true; - break; - } + // this name shadows the one we're looking for, in the body + bFound = true; + break; } - if (!bFound) - aStartPoints.push(pJump[nJumpCount - 1]); } + if (!bFound) + aStartPoints.push(pJump[nJumpCount - 1]); } break; default: // nothing else affects local variables diff --git a/sc/source/core/tool/interpr1.cxx b/sc/source/core/tool/interpr1.cxx index 81b77ec019c6..f39257ebc962 100644 --- a/sc/source/core/tool/interpr1.cxx +++ b/sc/source/core/tool/interpr1.cxx @@ -10416,9 +10416,9 @@ void ScInterpreter::ScLambda() { const short* pJump = static_cast(pCur)->GetJump(); short nJumpCount = pJump[0]; - short nJump; - if (nJumpCount < 1) + // This corresponds to LAMBDA(), with no parameters + if (nJumpCount < 2) { PushError(FormulaError::ParameterExpected); aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); @@ -10426,52 +10426,40 @@ void ScInterpreter::ScLambda() } FormulaToken** pCode = pArr->GetCode(); - short nParams = nJumpCount - 1; + // One jump for each param, plus a jump for the body, and a jump after the LAMBDA + short nParams = nJumpCount - 2; std::vector aParams(nParams); short nFirstOptionalParam = nParams; - if (nParams < 1) + // Param names are found at pJump[1] + 1 to pJump[nJumpCount - 2] + 1 + for (short nParam = 0; nParam < nParams; ++nParam) { - // FIXME: jump opcodes need to work differently; this shouldn't be forbidden - PushError(FormulaError::ParameterExpected); - aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); - return; - } - - // First param name is on the top of the stack - bool isOptional = false; - aParams[0] = PopStringName(isOptional).getString(); - - if (nGlobalError != FormulaError::NONE) - { - PushError(nGlobalError); - aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); - return; - } - - if (isOptional) - nFirstOptionalParam = 0; - - // Param names after the first are found at pJump[1] + 1 to pJump[nJumpCount - 2] + 1 - for (nJump = 1; nJump <= nJumpCount - 2; ++nJump) - { - if (const FormulaStringNameToken* pToken = GetStringNameToken(pCode[pJump[nJump] + 1])) + if (const FormulaStringNameToken* pToken = GetStringNameToken(pCode[pJump[nParam + 1] + 1])) { - aParams[nJump] = pToken->GetString().getString(); - // All optional parameters must appear at the end of the parameter list - if (nFirstOptionalParam < nParams) + if (pCode[pJump[nParam + 1] + 2]->GetType() == svSep) // must be followed by a separator { - // We've seen at least one optional param; all params after that point must - // be optional, as well. - if (!pToken->GetIsOptional()) + aParams[nParam] = pToken->GetString().getString(); + // All optional parameters must appear at the end of the parameter list + if (nFirstOptionalParam < nParams) { - PushError(FormulaError::IllegalParameter); - aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); - return; + // We've seen at least one optional param; all params after that point must + // be optional, as well. + if (!pToken->GetIsOptional()) + { + PushError(FormulaError::IllegalParameter); + aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); + return; + } } + else if (pToken->GetIsOptional()) + nFirstOptionalParam = nParam; + } + else + { + PushError(FormulaError::ParameterExpected); + aCode.Jump(pJump[nJumpCount], pJump[nJumpCount]); + return; } - else if (pToken->GetIsOptional()) - nFirstOptionalParam = nJump; } else { @@ -10481,7 +10469,7 @@ void ScInterpreter::ScLambda() } } - // Lambda-body is between pJump[nJumpCount - 1] and pJump[nJumpCount] + // Lambda-body is found between pJump[nJumpCount - 1] and pJump[nJumpCount] FormulaCallableRef pFunc = new ScFormulaFunction(*this, aParams, nFirstOptionalParam, *pArr, pJump[nJumpCount - 1], pJump[nJumpCount]); PushCallable(pFunc); diff --git a/sc/source/core/tool/interpr4.cxx b/sc/source/core/tool/interpr4.cxx index e6b9c2a4362e..aa3ee005b86f 100644 --- a/sc/source/core/tool/interpr4.cxx +++ b/sc/source/core/tool/interpr4.cxx @@ -910,26 +910,6 @@ const svl::SharedString & ScInterpreter::PopString() return svl::SharedString::getEmptyString(); } -const svl::SharedString& ScInterpreter::PopStringName(bool& rIsOptional) -{ - if (sp) - { - --sp; - const FormulaToken* p = pStack[sp]; - if (p->GetType() == svStringName) - { - auto pToken = static_cast(p); - rIsOptional = pToken->GetIsOptional(); - return pToken->GetString(); - } - else - SetError(FormulaError::ParameterExpected); - } - else - SetError(FormulaError::UnknownStackVariable); - return svl::SharedString::getEmptyString(); -} - void ScInterpreter::ValidateRef( const ScSingleRefData & rRef ) { SCCOL nCol;