Bug 80731 - Incorrect syntax does compile, MID without end bracket
Summary: Incorrect syntax does compile, MID without end bracket
Status: REOPENED
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-03-29 11:02 UTC (History)
2 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

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.