View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0037760 | FPC | Packages | public | 2020-09-16 21:12 | 2020-09-30 17:01 |
Reporter | Joe care | Assigned To | Michael Van Canneyt | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | x86-64 | OS | Win64 | ||
Product Version | 3.3.1 | ||||
Target Version | 4.0.0 | Fixed in Version | 3.3.1 | ||
Summary | 0037760: fcl-passrc PasWrite writes ';' before else in if then else-blocks. | ||||
Description | When 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 Reproduce | Parse 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 Information | Have not found the right place for a patch, will eventually provide one ... | ||||
Tags | fcl-passrc | ||||
Fixed in Revision | 46884 | ||||
FPCOldBugId | |||||
FPCTarget | 3.2.2 | ||||
Attached Files |
|
|
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. |
|
Fixed in 46884. Also remove spurious ; after else block. |
|
Ah, I just see Colin's remark. Yes, this is just how I fixed it. |
|
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. |
|
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 |
|
Fixed in 46889. I just saw your patch as I was opening the bugreport to mark it fixed... I should learn to wait ;-) |
|
... 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 ... |
|
... this time it's not the PasWrite, it's actually the PParser ... |
|
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. |
|
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 |
|
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.. |
|
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 |
|
Without the patch, i get ""empty statement" expected: <TPasImplCommand> but was: <TPasImplBeginBlock>" [edit] my test, just updating ... |
|
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 |
|
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; |
|
Applied your patches, also for test |
|
Tested in pas2js, the patch is OK there too. Thanks for your efforts, really appreciated! |
|
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; |
|
Thanks. Added in rev. 47021. |
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 |