Apache OpenOffice (AOO) Bugzilla – Issue 128585
Make the "IF" function compatible with ODF 1.3
Last modified: 2024-02-02 06:35:20 UTC
I am creating this issue to track a harder kind of change to our OpenFormula code, needed for ODF 1.3 compatibility. On AOO: | Condition IF formula |TRUE | FALSE --------------------------------+---------------+------ IF(condition) |TRUE |FALSE IF(condition;) |Err:511 |Err:511 IF(condition;IfTrue) |IfTrue |FALSE IF(condition;;) |Err:511 |Err:511 IF(condition;;IfFalse) |Err:518 |IfFalse IF(condition;IfTrue;) |Err:511 |Err:511 IF(condition;IfTrue;IfFalse) |IfTrue |IfFalse On LO, and as per the ODF 1.3 requirements: | Condition IF formula |TRUE | FALSE --------------------------------+---------------+------ IF(condition) |TRUE |FALSE IF(condition;) |0 |FALSE IF(condition;IfTrue) |IfTrue |FALSE IF(condition;;) |0 |0 IF(condition;;IfFalse) |0 |IfFalse IF(condition;IfTrue;) |IfTrue |0 IF(condition;IfTrue;IfFalse) |IfTrue |IfFalse
The calc method implementing the "IF" function is in ScInterpreter::ScIfJump() in file main/sc/source/core/tool/interpr1.cxx. However setting a breakpoint on this method, and then trying to type an "=IF(...)" formula, results in the breakpoint getting triggered when the formula is something AOO currently sees as valid, such as =IF(condition) or =IF(condition;IfTrue). When typing something that AOO currently doesn't see as valid, such as =IF(condition;) or =IF(condition;;), the breakpoint is NOT triggered. This means we need to change the "IF" function somewhere else, perhaps earlier on in the code, when the formula is getting parsed.
Formula parsing seems to work in stages: In main/sc/source/ui/view/viewfunc.cxx, method ScViewFunc::EnterData() calls ScCompiler::CompileString(), which lexes "=IF(TRUE;)" (the 2nd line in my original post) into: [ocIf, ocOpen, ocPush, ocSep, ocClose] This is then passed to ScCompiler::CompileTokenArray() which is implemented in its parent class, FormulaCompiler::CompileTokenArray(), in file main/formula/source/core/api/FormulaCompiler.cxx. This then parses the lexed tokens into the correct structure for the formulas. Eventually that somehow arrives in ScInterpreter::ScIfJump() from file main/sc/source/core/tool/interpr1.cxx for actual execution. The problem happens in FormulaCompiler::Factor(), in the "else if (eOp == ocIf || eOp == ocChose)" case, where it expects to find an expression for the 2nd parameter, but gets ocSep (";") instead. This generates an error during the call to Expression(). So the lexing is definitely correct, but we need to patch the parsing and possibly the execution.
Some further notes on how the lexing happens: IF(c) [ocIf, ocOpen, ocPush, ocClose] IF(c;) [ocIf, ocOpen, ocPush, ocSep, ocClose] IF(c;IfTrue) [ocIf, ocOpen, ocPush, ocSep, ocPush, ocClose] IF(c;;) [ocIf, ocOpen, ocPush, ocSep, ocMissing, ocSep, ocClose] IF(c;;IfFalse) [ocIf, ocOpen, ocPush, ocSep, ocMissing, ocSep, ocPush, ocClose] IF(c;IfTrue;) [ocIf, ocOpen, ocPush, ocSep, ocPush, ocSep, ocClose] IF(c;IfTrue;IfFalse) [ocIf, ocOpen, ocPush, ocSep, ocPush, ocSep, ocPush, ocClose] So it appears that: IF => ocIf ( => ocOpen values => ocPush ; => ocSep ) => ocClose 2 x adjacent ocSep also get ocMissing between them
In light of the fact that it's Expression() that fails, we can avoid calling it for ocClose with this kind of patch: ---snip--- diff --git a/main/formula/source/core/api/FormulaCompiler.cxx b/main/formula/source/core/api/FormulaCompiler.cxx index d5554e30c0..b1d7384d2c 100644 --- a/main/formula/source/core/api/FormulaCompiler.cxx +++ b/main/formula/source/core/api/FormulaCompiler.cxx @@ -1096,8 +1096,9 @@ void FormulaCompiler::Factor() { if ( ++nJumpCount <= nJumpMax ) pFacToken->GetJump()[nJumpCount] = pc-1; - NextToken(); - eOp = Expression(); + eOp = NextToken(); + if (eOp != ocClose) + eOp = Expression(); // ocSep or ocClose terminate the subexpression PutCode( pToken ); } ---snip--- and that gets these results: | Condition IF formula |TRUE | FALSE --------------------------------+---------------+------ IF(condition) |TRUE |FALSE IF(condition;) |#NULL! |FALSE IF(condition;IfTrue) |IfTrue |FALSE IF(condition;;) |Err:518 |#NULL! IF(condition;;IfFalse) |Err:518 |IfFalse IF(condition;IfTrue;) |IfTrue |#NULL! IF(condition;IfTrue;IfFalse) |IfTrue |IfFalse which is slightly better, but still needs further fixes. It also gets: =CHOICE(1;2;) to show "2" instead of "Err:511", as the same code is used to parse ocIf and ocChoice. Google Sheets and LO also display "2". However I also note that for Google Sheets and LO: =OR(TRUE;) returns TRUE =AND(TRUE;) returns TRUE but for us, those are Err:511. Thus Expression() needs to be fixed for other tokens too, not just ocIf and ocChoice.