Bug 58648

Created attachment 33295 [details]
skip whitespace before matching close parentheses

Thanks for finding this problem!
I added a unit tests that shows this problem in r1716338, and can verify that this problem still exists in the latest dev build (2015-11-25, r1716338).

The attached patch, which I think is what your suggested fix was from comment 0, still fails the unit test. Were you able to get a passing build? If so, could you add your patch/diff as an attachment?

Sorry no.  I haven't tried building the code, just been using jars and src-jars up til now.
My suggested fix was based on a quick perusal of the source code.  It may not actually fix the problem.

The second Match() call without another SkipWhite() looked suspiscious, so good guess. I guess whoever works on this bug will become intimately familiar with the deep call-stack that is used for PTG parsing.

*** Bug 59271 has been marked as a duplicate of this bug. ***

Bug 59271 provides a testcase with a testfile

Did some initial analysis: the problem is not skipping whitespaces, but the Excel "intersection operator", which is ... TADA .. a blank! 

Bad idea to use a char for something when you also allow the same char to act as arbitrary whitespace in formulas at various places. Seems we need to put logic in place that at first tries to parse the pieces as intersection and if that fails skip it as whitespace. Quite bad for reproducible behaviour and good error messages. :(

My approach would be to handle this in FormulaParser.intersectionExpression() as follows, however I fear that this might lead to hard to track errors with formula parsing and/or misleading error messages, any thoughts or ideas how to do this better? I.e. maybe intersection is only possible in some specific places in the formula and we can only allow it there in the first place?

   private ParseNode intersectionExpression() {
		ParseNode result = comparisonExpression();
		boolean hasIntersections = false;
		while (true) {
			SkipWhite();
			if (_inIntersection) {
				int savePointer = _pointer;

				// Don't getChar() as the space has already been eaten and recorded by SkipWhite().
				try {
					ParseNode other = comparisonExpression();
					result = new ParseNode(IntersectionPtg.instance, result, other);
					hasIntersections = true;
					continue;
				} catch (FormulaParseException e) {
					// if parsing for intersection fails we assume that we actually had an arbitrary
					// whitespace and thus should simply skip this whitespace
					resetPointer(_pointer);
				}
			}
			if (hasIntersections) {
				return augmentWithMemPtg(result);
			}
			return result;
		}
	}

See http://www.techrepublic.com/blog/microsoft-office/use-excels-intersect-operator-to-evaluate-common-cells/ and http://trumpexcel.com/2013/08/intersect-operator-in-excel/ for some description of the intersection operator.

Wow using a blank as an operator is really weird - I was not aware of that. Did we add support for the intersection in 3.13 or what changes between 3.12 & 3.13 caused this issue?

Seems to have been introduced by bug 52111 when support for the intersection operator was added.

That makes sense. Unfortunately I don't know the whole formula parsing in POI too well, so I don't know if your suggested fix would work ;-(

Intersection is whitespace between two cell range/area PTGs. If the two operands don't evaluate to ranges, treat the whitespace as just whitespace. Is multiple whitespace characters as the operator a valid intersection expression?

Well in excel both

=A1:B1 B1:B2
as well as 
=A1:B1    B1:B2

are valid and return the same value (b) in the following table

 | A | B |  
1| a | b |
2| a | a |

I have applied my proposed simple approach via r1738033 and added a number of tests which verify that all the cases that I could think of seem to work fine now. If there is any that I missed, please report it as new bug so we can fix it and add additional unit-tests.

*** Bug 59652 has been marked as a duplicate of this bug. ***

Re-enabled unit tests in r1765548.

Well in excel both

=A1:B1 B1:B2
as well as 
=A1:B1    B1:B2

As shown here,

https://yodalearning.com/tutorials/conditional-formatting-with-formulas/

I totally agreed with Kia G,

The rest of formula is as stated

Well in excel both

=A1:B1 B1:B2
as well as 
=A1:B1    B1:B2

are valid and return the same value (b) in the following table

 | A | B |  
1| a | b |
2| a | a |


Thanks
http://www.excelstemplates.com/

The reported original problem was fixed quite a while ago, I also tried with the formula which contains white-spaces, which also seems to be parsed now. So as far as I see this is fixed, please create new bug-reports for additional things that you might find not working correctly.