Bug 80731 - Incorrect syntax does compile, MID without end bracket
Summary: Incorrect syntax does compile, MID without end bracket
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
4.2.4.2 release
Hardware: x86 (IA32) Linux (All)
: medium minor
Assignee: Pierre Lepage
QA Contact:
URL:
Whiteboard: target:5.4.0 target:5.2.6 target:5.3....
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-30 20:02 UTC by ge.huber
Modified: 2017-07-13 10:45 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Testing program for Mid function and Mid instruction (3.72 KB, text/plain)
2016-11-01 00:35 UTC, Pierre Lepage
Details
Bug Solution Report (141.83 KB, application/vnd.oasis.opendocument.text)
2016-12-21 11:18 UTC, Pierre Lepage
Details
View of the proposed change. (57.64 KB, image/png)
2017-06-28 01:40 UTC, Pierre Lepage
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ge.huber 2014-06-30 20:02:35 UTC
Run this code 

Sub AcceptableSyntax
Dim alpha as String
alpha = "abcdefghijklmn"
'Compiles, but doesn't work
Mid(alpha,3,3 = ""
MsgBox alpha, 0, "Compiles, but doesn't work"
'Works
Mid(alpha,3,3) = ""
MsgBox alpha, 0, "Works"
End Sub

Line 5 should not compile and should instead throw a syntax error.
Comment 1 raal 2014-07-30 13:38:08 UTC
Reproducible with Version: 4.4.0.0.alpha0+
Build ID: 7d06a0601ddccc50185ea97fddcdf2ea39299096
TinderBox: Linux-rpm_deb-x86_64@46-TDF, Branch:master, Time: 2014-07-28_06:17:50
Comment 2 QA Administrators 2015-09-04 02:47:49 UTC Comment hidden (obsolete)
Comment 3 ge.huber 2015-09-13 22:30:02 UTC
Reproducible with Version: 5.0.0.5
Build-ID: 00m0(Build:5)
Comment 4 QA Administrators 2016-09-20 10:32:13 UTC Comment hidden (obsolete)
Comment 5 Pierre Lepage 2016-10-03 01:00:29 UTC
Reproducible with version 5.2.1.2
Build ID: 31dd62db80d4e60af04904455ec9c9219178d620
Comment 6 Pierre Lepage 2016-10-04 00:51:16 UTC
Line 7: Mid (alpha, 3.3 = ""
If I write Mid(alpha,3) then it will crash "writer".
Comment 7 Pierre Lepage 2016-10-08 13:01:26 UTC
MID has two versions. One is a statement that can be written to the left of the "=". The other is a function that is always to the right of "=" sign. In the case of an instruction, Mid imperatively requires four arguments. In the case of a function, Mid imperatively requires the first two arguments, the third being optional and fourth do not exist.

I take this opportunity to draw attention to the text in the online help. This text is convoluted! I think I propose a new, clearer.

I confirm the issue raised in comment No. 1. Even if you add a closing parenthesis, LibreOffice crash completely and abruptly. Digging through the code, I realized Mid was entitled to special treatment by the BASIC interpreter. See it in the method "void SbiParser :: Symbol (const * KeywordSymbolInfo pKeywordSymbolInfo)", line 489 in parser.cxx.

It digests bad the Mid statement and I do not know why.

In the table of methods (aMethod) we have to Mid:
{ "Mid", SbxSTRING, 3 | LFUNCTION_, RTLNAME (Mid), 0}

In the first argument, we find the reserved common name "Mid". In the second argument, the expected control result is a string. The third argument, the command is declared to have three parameters (a bug? The online help shows four parameters for instruction form (LFUNCTION_). Finally the routine running the Mid recipe is declared bear the name RTLNAME(Mid). For the last item in the table (here 0), it is the nhash variable, and I do not know yet what it is.

The recipe is from Mid 1190 line methods.cxx file (RTLFUNC (Mid)).
Comment 8 Pierre Lepage 2016-10-08 20:49:47 UTC
sub Test1_Mid
'
' Instruction Mid
Dim s As String

	s="abcdefghij"
	
	mid(s,3,3,"X")

	msgbox s, 0, "Résult"
end sub

With two brackets or no, Mid behavior (2, 3 or 4 parameters) is OK. But with:

mid s,3,3,"X" = "X"

Give -> ab-1fghij
Comment 9 Pierre Lepage 2016-10-25 09:58:23 UTC
The problem is not strictly the Mid statement. For example, the following code compiles and runs (result expected is "A, B, C"):

Sub Main
	TestParenthesis("A, ", "B, ", "C"
End Sub

Function TestParenthesis(a, b, c) As String
	MsgBox a & b & c, 0, "Test parenthesis"
End Function


I was watching towards the translation of expressions for the treatment of parentheses in a situation of an LVALUE type of instruction. Normally, a compilation error should be raised.
Comment 10 Pierre Lepage 2016-11-01 00:35:33 UTC
Created attachment 128397 [details]
Testing program for Mid function and Mid instruction

This file is a Basic program to test different version of Mid as instruction, as function, with 3 or 4 arguments, with bug et sans bug.
Comment 11 Pierre Lepage 2016-11-01 01:02:38 UTC
I did a little BASIC program to test all variants of the function and the MID instruction. Try it!

My search for the source of the problem leads me to the method 

SbiExprNode * SbiExpression :: Term (const * KeywordSymbolInfo pKeywordSymbolInfo)

in exprtree.cxx, line 230 (while (etok == LPAREN)). I do not quite understand the logic here. I think the bug is here.

Mid is treated as a token "SYMBOL" (line 405 parser.cxx) In fact, the method

void SbiParser :: Symbol (const KeywordSymbolInfo pKeywordSymbolInfo *)

is used, leading to the line 492 (parser.cxx) it bring us to the Term method previously mentioned.
Comment 12 Pierre Lepage 2016-11-02 01:37:27 UTC
A Work in progress...

In exprtree.cxx, line 1057, the condition ( pExprList->bBracket && eTok == RPAREN ) || SbiTokenizer::IsEoln( eTok ) arrive when we are at the end of parameters list. Two conditions are possible.

First : in an standalone expression (like Mid) opening parenthesis is present and the current token is a left parenthesis as LPAREN = '('.

Second : the current token (etok) is an EOL (end of line). This was the case of Mid without parenthesis... or Mid with an opening parenthesis but without a closing one. 

So, I inserted code to test case where we have bBracket to True and EOL condition to True. If so, we must raise an error for a missing closing parenthesis.

New code look like this:

            if( ( pExprList->bBracket && eTok == RPAREN ) || SbiTokenizer::IsEoln( eTok ) )
            {
                if ( SbiTokenizer::IsEoln( eTok ) && pExprList->bBracket)
                {
                    pParser->Error( pExprList->bBracket ? ERRCODE_BASIC_BAD_BRACKETS : ERRCODE_BASIC_EXPECTED, RPAREN );
                }
                break;
            }

I compiled and this seem to work fine. But, I must test thoroughly before submitting a patch.
Comment 13 Pierre Lepage 2016-11-02 02:02:44 UTC
Refinement:

pParser->Error( ERRCODE_BASIC_EXPECTED, RPAREN );
Comment 14 Commit Notification 2016-11-30 06:44:39 UTC
Pierre Lepage committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=379b7ffb68bed5bc376a91032a781be147a6eff1

tdf#80731 Closing parenthesis is now detected (Mid statement and functions).

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Pierre Lepage 2016-12-21 11:18:00 UTC
Created attachment 129841 [details]
Bug Solution Report

You will find here a document summarizing the search for the cause of the problem and its solution.
Comment 16 Commit Notification 2017-01-19 15:14:16 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ecd82b5121899fe23f0ccbdd8a89c8b18436f027

Related: tdf#80731 bug in our own wizards revealed by new bracket test

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Commit Notification 2017-01-19 16:34:48 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ad00157b7b60fa1feb35d1e5a4f7a6d418d10f19&h=libreoffice-5-2

Related: tdf#80731 bug in our own wizards revealed by new bracket test

It will be available in 5.2.6.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2017-01-19 19:16:40 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e6b5a545baf516b3f835a97bb7c89aafd3f63d95&h=libreoffice-5-3

Related: tdf#80731 bug in our own wizards revealed by new bracket test

It will be available in 5.3.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Commit Notification 2017-01-25 11:43:24 UTC
Pierre Lepage committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=65e294e221e0ad1d37c6667eecb995817ace870d&h=libreoffice-5-3

tdf#80731 Closing parenthesis is now detected (Mid statement and functions).

It will be available in 5.3.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 20 Commit Notification 2017-02-28 21:29:19 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6bb6ca1fb30f786385c2357e5435077066a49f82

Related: tdf#80731 missing ')' breaks Gimmicks.Autotext macro

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Commit Notification 2017-03-21 11:46:41 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=660e394b893a045a077ccded263f94a1be03fcf9&h=libreoffice-5-3

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.3.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Commit Notification 2017-03-27 12:58:51 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-3-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4725c2a282f2c6468125073ba9cb5ba66f488e08&h=libreoffice-5-3-2

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.3.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 23 ge.huber 2017-03-28 00:37:24 UTC
I can sympathize with people who simply
want to run their old code, which they 
always experienced as working.
On the other hand I like to have my 
errors escalated to me as early as possible.
Also simply leaving the bug in creates 
a locked-in-situation, that is as long
as this is not flagged as an error
more syntactically incorrect but in some 
sense working code will likely be produced.

Therefor I propose a compiler option

Option Pragma Strict

the default being that without this 
option the missing parenthesis is ignored.
Comment 24 Pierre Lepage 2017-03-29 10:14:05 UTC
(In reply to ge.huber from comment #23)

Would the option be only for the problem of the missing closing parenthesis? If so, this is a lot of programming in StarBasic for an exception. Otherwise, we embark on a dangerous slope where the rigor in the respect of the grammar of StarBasic will suffer from an exception chooses unclear on what arbitrary bases.

It would be better to add a comment in the list of improvements accompanying the release of the revision where the patch will appear for the first time (5.3.2, I think). Example: Correcting the missing closing parenthesis problem could have a profound impact on the functioning of existing procedures written in StarBasic. A revision of the existing codes may therefore be necessary. It will then be necessary to add the missing closing parenthesis.

Regards

Pierre
Comment 25 ge.huber 2017-03-29 11:02:20 UTC
I'm fine with this.

BTW a nice piece of detective work you've done here.
Thanks for it all.
Comment 26 Commit Notification 2017-03-31 14:29:30 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=351dead74b4c213b13102f81b5ae9bb47ad8ca39

tdf#106529: Revert "tdf#80731 Closing parenthesis is now detected"

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 27 Volga 2017-04-05 11:17:18 UTC
I suggest we should add an exception to macros created by extensions.
Comment 28 Pierre Lepage 2017-04-08 01:00:09 UTC
Currently I have version 5.3.2 on my workstation. The patch is implanted and therefore the missing closing parentheses are detected. I understand the issues for existing macros. Especially those where the code is in read-only mode. Myself, I had a macro that suddenly did not work any more ... because of the absence of closing parentheses;)

My assessment of the situation is that the temporary withdrawal of the corrective is a wise decision. Adding a trick to bypass the BASIC code crash is not a good solution. I recall that the absence of closing parenthesis does not only concern the instruction MID. All functions of the user are also affected. It's rather embarrassing for an interpreter who also wants to be a gateway to learning programming rigorously.

A possible approach would be to make available to the users an application (macro?) Which would analyze the code to detect all cases of missing closing parentheses. Here, I wonder if it is possible to take advantage of the StarBasic analyzer. But the latter is rather inconvenient with his habit of stopping at the first mistake!

For a program in read-only mode, the programmer will have to get involved and update his code. If the programmer is no longer available, it will be necessary to either rewrite the code from scratch (by a good Samaritan, hum ...), or to hard code an "automatic" bypass of the detection of the closing parenthesis with the " Assumed that the read-only code was already functioning correctly. The bypass would be available for a limited time period (3 to 4 years).
Comment 29 Pierre Lepage 2017-06-27 01:33:18 UTC
So, what do we do now? Should we
A) code for an exception (Volga)?
B) code for an option on rigor (ge.huber)?
C) tolerate the current situation and not change anything?

To lose everything, I prefer b). In this case, a check box should be inserted in the IDE options dialog "Tolerate absence of closing parenthesis".
Comment 30 ge.huber 2017-06-27 05:44:17 UTC
I'm opposed to c). Warn and tolerate for up to 3 or 4 years seems to be reasonable middle ground.
Comment 31 Pierre Lepage 2017-06-28 01:38:25 UTC
Here is a test dialog (see attachment 'Transitional measures.png') that introduces a new section for transitions measurements. Here we have "StarBasic Pragma Strict". I still have to make the links with the source code. Comments?
Comment 32 Pierre Lepage 2017-06-28 01:40:19 UTC
Created attachment 134331 [details]
View of the proposed change.
Comment 33 ge.huber 2017-06-28 08:03:42 UTC
Comment on attachment 134331 [details]
View of the proposed change.

I feel honoured by having the exact wording of my proposal chosen. But of course this is open to improvements, especially because it is not selfexplaining. On the other hand a Quicktip which appears on hoovering other the option might suffice.
Comment 34 Xisco Faulí 2017-07-13 10:04:52 UTC
Setting Assignee back to default. Please assign it back to yourself if you're still working on this issue
Comment 35 Pierre Lepage 2017-07-13 10:34:44 UTC
I am always thinking about the solution and its implementation.