View Issue Details

IDProjectCategoryView StatusLast Update
0037760FPCPackagespublic2020-09-30 17:01
ReporterJoe care Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx86-64OSWin64 
Product Version3.3.1 
Target Version4.0.0Fixed in Version3.3.1 
Summary0037760: fcl-passrc PasWrite writes ';' before else in if then else-blocks.
DescriptionWhen parsing a program with an if-then-else construct and using paswrite to write it again.
these are semicolons (';') in front of the else command, resulting that the written program does not compile anymore.
Steps To ReproduceParse with fcl-passrc:
begin if true then write('true')else write('false'); end.
Paswrite writes:
program test2;


begin
  if True then
    write('true');

  else
    write('false');
    ;
end.
Additional InformationHave not found the right place for a patch, will eventually provide one ...
Tagsfcl-passrc
Fixed in Revision46884
FPCOldBugId
FPCTarget3.2.2
Attached Files

Activities

Colin Haywood

2020-09-17 05:10

reporter   ~0125586

If this tool always splits up if statements onto multiple lines, then perhaps a good fix would be to get it to always put begin/end around the statements to be executed (the writeln's here), which helps people avoid the classic bug

if something then
    do_this_conditionally();
    do_this_conditionally_too(); // nope, it does this always

when another line is inserted later.

That extra semicolon before the "end." looks rather out-of-place too.

Michael Van Canneyt

2020-09-17 10:21

administrator   ~0125588

Fixed in 46884. Also remove spurious ; after else block.

Michael Van Canneyt

2020-09-17 10:23

administrator   ~0125589

Ah, I just see Colin's remark. Yes, this is just how I fixed it.

Joe care

2020-09-18 16:23

reporter   ~0125615

Now I get nested begin and ends. between If and else so:
[code] // PasWrite line 1258ff
    IncIndent;
    if AIfElse.IfBranch is TPasImplBeginBlock then
      WriteImplBlock(TPasImplBeginBlock(AIfElse.IfBranch))
    else
      WriteImplElement(AIfElse.IfBranch, False);
    DecIndent;
[/code]
So if there is already a begin...end only the inner block (the commands in the block) is/are written.

Joe care

2020-09-18 16:26

reporter   ~0125616

Patch appended ...
paswrite.patch (515 bytes)   
Index: paswrite.pp
===================================================================
--- paswrite.pp	(Revision 46888)
+++ paswrite.pp	(Arbeitskopie)
@@ -1256,7 +1256,10 @@
     if DoBeginEnd then
       AddLn('begin');
     IncIndent;
-    WriteImplElement(AIfElse.IfBranch, False);
+    if AIfElse.IfBranch is TPasImplBeginBlock then
+      WriteImplBlock(TPasImplBeginBlock(AIfElse.IfBranch))
+    else
+      WriteImplElement(AIfElse.IfBranch, False);
     DecIndent;
     if DoBeginEnd then
       begin
paswrite.patch (515 bytes)   

Michael Van Canneyt

2020-09-18 16:31

administrator   ~0125617

Fixed in 46889.

I just saw your patch as I was opening the bugreport to mark it fixed... I should learn to wait ;-)

Joe care

2020-09-30 12:35

reporter   ~0125984

Last edited: 2020-09-30 12:36

View 2 revisions

... still not fully there ...
[code]
begin if true then if false then write('t')else else write('f'); readln end.
[/code] result of the code should be nothing
PasWrite writes:
[code]
begin
  if True then
    if False then
    begin
      write('t');
    end else
      write('f');
  readln;
end.
[/code] which will output an 'f'
... will try to do a patch, but have not found time to debug ...

Joe care

2020-09-30 13:45

reporter   ~0125985

... this time it's not the PasWrite, it's actually the PParser ...

Michael Van Canneyt

2020-09-30 13:56

administrator   ~0125986

I would be really surprised to see that pparser has an error in this domain.

We would surely have noticed when that parsed wrong: pparser is used in the pas2js compiler and many other programs.
If the compiler would not correctly translate if..then, users would have complained a long time ago.

Joe care

2020-09-30 14:53

reporter   ~0125992

It's definitly the parser, the second else branch is set to the else branch of the inner if statement.

also a case [...] if else else construct was parsed in a wrong way here the else of the case is set as the else branch if the inner if statement.
Though else else constucts are realy not very common but ...
pparser.patch (1,141 bytes)   
Index: pparser.pp
===================================================================
--- pparser.pp	(Revision 47014)
+++ pparser.pp	(Arbeitskopie)
@@ -5986,7 +5986,20 @@
               El:=nil;
             end;
             if (CurToken=tkelse) and (TPasImplIfElse(CurBlock).ElseBranch=nil) then
-              break; // add next statement as ElseBranch
+              begin
+                // Check if next token is an else too
+                NextToken;
+                if CurToken = tkElse then
+                  begin
+                    // empty ELSE statement without semicolon e.g. if condition then [...] else else
+                    El:=TPasImplCommand(CreateElement(TPasImplCommand,'', CurBlock,CurTokenPos));
+                    CurBlock.AddElement(El); // this sets TPasImplIfElse(CurBlock).IfBranch:=El
+                    El:=nil;
+                    CloseBlock;
+                  end;
+                UngetToken;
+                break; // add next statement as ElseBranch
+              end;
             end
           else if (CurBlock is TPasImplTryExcept) and (CurToken=tkelse) then
             begin
pparser.patch (1,141 bytes)   

Michael Van Canneyt

2020-09-30 15:08

administrator   ~0125993

Last edited: 2020-09-30 15:10

View 2 revisions

I tested with your program:

home:~> fpc a.pp
home:~> ./a
home:~> pas2js a.pp -Tnodejs -Jirtl.js -Jc
Info: 1156 lines in 2 files compiled, 0.1 secs
home:~> nodejs a.js
home:~>

Compiled in fpc/pas2js, both programs output empty, without your patch..

Joe care

2020-09-30 15:32

reporter   ~0125994

Hmm .. i don't know what pas2js does, but I looked at the actual datastucures, and they seemd wrong.
Let's try a test ...
BTW could you upload a.pp ?
tests.patch (2,308 bytes)   
Index: tcbaseparser.pas
===================================================================
--- tcbaseparser.pas	(Revision 47014)
+++ tcbaseparser.pas	(Arbeitskopie)
@@ -661,9 +661,11 @@
     FFileName:=MainFilename;
   FResolver.AddStream(FFileName,TStringStream.Create(FSource.Text));
   FScanner.OpenFile(FFileName);
+  {$ifndef NOCONSOLE} // JC: To get the tests to run with GUI
   Writeln('// Test : ',Self.TestName);
   for i:=0 to FSource.Count-1 do
     Writeln(Format('%:4d: ',[i+1]),FSource[i]);
+  {$EndIf}
 end;
 
 procedure TTestParser.ParseDeclarations;
Index: tcstatements.pas
===================================================================
--- tcstatements.pas	(Revision 47014)
+++ tcstatements.pas	(Arbeitskopie)
@@ -65,6 +65,7 @@
     Procedure TestIfAssignment;
     Procedure TestIfElse;
     Procedure TestIfElseBlock;
+    Procedure TestIfIfElseElseBlock;
     Procedure TestIfSemiColonElseError;
     procedure TestIfforElseBlock;
     procedure TestIfRaiseElseBlock;
@@ -637,6 +638,28 @@
   AssertEquals('begin end block',TPasImplBeginBlock,I.ElseBranch.ClassType);
 end;
 
+procedure TTestStatementParser.TestIfIfElseElseBlock;
+var
+  OuterIf,InnerIf: TPasImplIfElse;
+begin
+  DeclareVar('boolean');
+  DeclareVar('boolean','B');
+  TestStatement(['if a then','if b then','  begin','  end','else','else','  begin','  end']);
+  OuterIf:=AssertStatement('If statement',TPasImplIfElse) as TPasImplIfElse;
+  AssertExpression('IF condition',OuterIf.ConditionExpr,pekIdent,'a');
+  AssertNotNull('if branch',OuterIf.IfBranch);
+  AssertEquals('if else block',TPasImplIfElse,OuterIf.ifBranch.ClassType);
+  InnerIf:=OuterIf.IfBranch as TPasImplIfElse;
+  AssertExpression('IF condition',InnerIf.ConditionExpr,pekIdent,'b');
+  AssertNotNull('if branch',InnerIf.IfBranch);
+  AssertEquals('begin end block',TPasImplBeginBlock,InnerIf.ifBranch.ClassType);
+  AssertNotNull('Else branch',InnerIf.ElseBranch);
+  AssertEquals('empty statement',TPasImplCommand,InnerIf.ElseBranch.ClassType);
+  AssertEquals('empty command','',TPasImplCommand(InnerIf.ElseBranch).Command);
+  AssertNotNull('Else branch',OuterIf.ElseBranch);
+  AssertEquals('begin end block',TPasImplBeginBlock,OuterIf.ElseBranch.ClassType);
+end;
+
 procedure TTestStatementParser.TestIfforElseBlock;
 
 Var
tests.patch (2,308 bytes)   

Joe care

2020-09-30 15:34

reporter   ~0125997

Last edited: 2020-09-30 15:36

View 2 revisions

Without the patch, i get ""empty statement" expected: <TPasImplCommand> but was: <TPasImplBeginBlock>"
[edit]
my test, just updating ...

Michael Van Canneyt

2020-09-30 15:37

administrator   ~0125998

Seems our remarks crossed.. :-)

I added your testcase, it works.
But please test the case I posted as well. It seems something is not closed correctly

Joe care

2020-09-30 15:53

reporter   ~0126000

Last edited: 2020-09-30 15:59

View 3 revisions

I did, the test is slightly wrong, the else branch cannnot be nil otherwise the else would be omitted ...
[edit]
it's the same command that is put as the then-branch for an empty 'if then else' statement.
... wait a moment, i also do an
  TestStatement(['case a of','1 : if b then',' begin end','else','else','DoElse',' end;']);
test.
tcstatements.patch (705 bytes)   
Index: tcstatements.pas
===================================================================
--- tcstatements.pas	(Revision 47017)
+++ tcstatements.pas	(Arbeitskopie)
@@ -739,7 +739,9 @@
   I2:=I.Ifbranch as TPasImplIfElse;
   AssertExpression('IF condition',I2.ConditionExpr,pekIdent,'b');
   AssertNotNull('Have then for inner if',I2.ifBranch);
-  AssertNull('Empty else for inner if',I2.ElseBranch);
+  AssertnotNull('Empty else for inner if',I2.ElseBranch);
+  AssertEquals('Have a commend for inner if else',TPasImplCommand,I2.ElseBranch.ClassType);
+  AssertEquals('... an empty command','',TPasImplCommand(I2.ElseBranch).Command);
 end;
 
 procedure TTestStatementParser.TestIfIfElseElseBlock;
tcstatements.patch (705 bytes)   

Michael Van Canneyt

2020-09-30 16:04

administrator   ~0126001

Applied your patches, also for test

Michael Van Canneyt

2020-09-30 16:06

administrator   ~0126002

Tested in pas2js, the patch is OK there too.

Thanks for your efforts, really appreciated!

Joe care

2020-09-30 16:14

reporter   ~0126003

Appended a test for the case if then else else - problem.
tcstatements-2.patch (2,434 bytes)   
Index: tcstatements.pas
===================================================================
--- tcstatements.pas	(Revision 47020)
+++ tcstatements.pas	(Arbeitskopie)
@@ -100,6 +100,7 @@
     Procedure TestCaseElseBlockAssignment;
     Procedure TestCaseElseBlock2Assignments;
     Procedure TestCaseIfCaseElse;
+    Procedure TestCaseIfCaseElseElse;
     Procedure TestCaseIfElse;
     Procedure TestCaseElseNoSemicolon;
     Procedure TestCaseIfElseNoSemicolon;
@@ -1301,7 +1302,7 @@
   C:=AssertStatement('Case statement',TpasImplCaseOf) as TpasImplCaseOf;
   AssertNotNull('Have case expression',C.CaseExpr);
   AssertExpression('Case expression',C.CaseExpr,pekIdent,'a');
-  AssertEquals('Two case labels',1,C.Elements.Count);
+  AssertEquals('One case label',1,C.Elements.Count);
   AssertNull('Have no else branch',C.ElseBranch);
   S:=TPasImplCaseStatement(C.Elements[0]);
   AssertEquals('2 expressions for case 1',1,S.Expressions.Count);
@@ -1311,6 +1312,30 @@
   AssertNotNull('If statement has else block',TPasImplIfElse(S.Elements[0]).ElseBranch);
 end;
 
+procedure TTestStatementParser.TestCaseIfCaseElseElse;
+Var
+  C : TPasImplCaseOf;
+  S : TPasImplCaseStatement;
+
+begin
+  DeclareVar('integer');
+  DeclareVar('boolean','b');
+  TestStatement(['case a of','1 : if b then',' begin end','else','else','DoElse',' end;']);
+  C:=AssertStatement('Case statement',TpasImplCaseOf) as TpasImplCaseOf;
+  AssertNotNull('Have case expression',C.CaseExpr);
+  AssertExpression('Case expression',C.CaseExpr,pekIdent,'a');
+  AssertEquals('Two case labels',2,C.Elements.Count);
+  AssertNotNull('Have an else branch',C.ElseBranch);
+  S:=TPasImplCaseStatement(C.Elements[0]);
+  AssertEquals('2 expressions for case 1',1,S.Expressions.Count);
+  AssertExpression('Case With identifier 1',TPasExpr(S.Expressions[0]),pekNumber,'1');
+  AssertEquals('1 case label statement',1,S.Elements.Count);
+  AssertEquals('If statement in case label 1',TPasImplIfElse,TPasElement(S.Elements[0]).ClassType);
+  AssertNotNull('If statement has else block',TPasImplIfElse(S.Elements[0]).ElseBranch);
+  AssertEquals('If statement has a commend as else block',TPasImplCommand,TPasImplIfElse(S.Elements[0]).ElseBranch.ClassType);
+  AssertEquals('But ... an empty command','',TPasImplCommand(TPasImplIfElse(S.Elements[0]).ElseBranch).Command);
+end;
+
 procedure TTestStatementParser.TestCaseElseNoSemicolon;
 Var
   C : TPasImplCaseOf;
tcstatements-2.patch (2,434 bytes)   

Michael Van Canneyt

2020-09-30 17:01

administrator   ~0126004

Thanks. Added in rev. 47021.

Issue History

Date Modified Username Field Change
2020-09-16 21:12 Joe care New Issue
2020-09-16 21:12 Joe care Tag Attached: fcl-passrc
2020-09-17 05:10 Colin Haywood Note Added: 0125586
2020-09-17 10:21 Michael Van Canneyt Assigned To => Michael Van Canneyt
2020-09-17 10:21 Michael Van Canneyt Status new => resolved
2020-09-17 10:21 Michael Van Canneyt Resolution open => fixed
2020-09-17 10:21 Michael Van Canneyt Fixed in Revision => 46884
2020-09-17 10:21 Michael Van Canneyt FPCTarget => -
2020-09-17 10:21 Michael Van Canneyt Note Added: 0125588
2020-09-17 10:22 Michael Van Canneyt Fixed in Version => 3.3.1
2020-09-17 10:22 Michael Van Canneyt Target Version => 4.0.0
2020-09-17 10:22 Michael Van Canneyt FPCTarget - => 3.2.2
2020-09-17 10:23 Michael Van Canneyt Note Added: 0125589
2020-09-18 16:23 Joe care Status resolved => feedback
2020-09-18 16:23 Joe care Resolution fixed => open
2020-09-18 16:23 Joe care Note Added: 0125615
2020-09-18 16:26 Joe care Note Added: 0125616
2020-09-18 16:26 Joe care File Added: paswrite.patch
2020-09-18 16:26 Joe care Status feedback => assigned
2020-09-18 16:31 Michael Van Canneyt Status assigned => resolved
2020-09-18 16:31 Michael Van Canneyt Resolution open => fixed
2020-09-18 16:31 Michael Van Canneyt Note Added: 0125617
2020-09-30 12:35 Joe care Status resolved => feedback
2020-09-30 12:35 Joe care Resolution fixed => open
2020-09-30 12:35 Joe care Note Added: 0125984
2020-09-30 12:36 Joe care Note Edited: 0125984 View Revisions
2020-09-30 13:45 Joe care Note Added: 0125985
2020-09-30 13:45 Joe care Status feedback => assigned
2020-09-30 13:56 Michael Van Canneyt Note Added: 0125986
2020-09-30 14:53 Joe care Note Added: 0125992
2020-09-30 14:53 Joe care File Added: pparser.patch
2020-09-30 15:08 Michael Van Canneyt Note Added: 0125993
2020-09-30 15:10 Michael Van Canneyt Note Edited: 0125993 View Revisions
2020-09-30 15:32 Joe care Note Added: 0125994
2020-09-30 15:32 Joe care File Added: tests.patch
2020-09-30 15:34 Joe care Note Added: 0125997
2020-09-30 15:36 Joe care Note Edited: 0125997 View Revisions
2020-09-30 15:37 Michael Van Canneyt Note Added: 0125998
2020-09-30 15:53 Joe care Note Added: 0126000
2020-09-30 15:53 Joe care File Added: tcstatements.patch
2020-09-30 15:54 Joe care Note Edited: 0126000 View Revisions
2020-09-30 15:59 Joe care Note Edited: 0126000 View Revisions
2020-09-30 16:04 Michael Van Canneyt Status assigned => resolved
2020-09-30 16:04 Michael Van Canneyt Resolution open => fixed
2020-09-30 16:04 Michael Van Canneyt Note Added: 0126001
2020-09-30 16:06 Michael Van Canneyt Note Added: 0126002
2020-09-30 16:14 Joe care Note Added: 0126003
2020-09-30 16:14 Joe care File Added: tcstatements-2.patch
2020-09-30 17:01 Michael Van Canneyt Note Added: 0126004