Possible bug in Lazarus 4.4?

[Alex.Machado]

Hi there,

after building and installing IntraWeb packages in Lazarus 4.4 I started receiving an exception when creating a new IntraWeb project (it used to work fine in Laz 3.4, 3.6 and 3.8 ).

Code: Pascal  [Select][+][-]

1. [Debugger Exception Notification]
2. Project Lazarus raised exception class 'EListError' with message:
3. List index (9) out of bounds
4.  
5.  At address 1000D8488
6.  

The exception happens here (Line 11 below):

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. var
3.   i: Integer;
4. begin
5.   Result:=-1;
6.   for i:=0 to FDesignerBaseClasses.Count-1 do
7.   begin
8.     if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
9.     begin
10.       if (Result<0)
11.           or (TClass(FDesignerBaseClassesCanCreateForm[i]).InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
12.         Result:=i;
13.     end;
14.   end;
15. end;
16.  

This code contains a big code smell, IMO:

It access elements in 2 separate lists:

FDesignerBaseClasses
FDesignerBaseClassesCanCreateForm

But only one is guaranteed to be within bounds via the loop variable, FDesignerBaseClasses list.
The other list, FDesignerBaseClassesCanCreateForm, is not. In the case presented here, it definitely contain a different number of elements.

I checked the code of version 3.8 and it's correct:

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. begin
3.   Result:=FDesignerBaseClasses.Count-1;
4.   while (Result>=0)
5.   and (not AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) do
6.     dec(Result);
7. end;

This is the related call stack:

Code: [Select]

#0 FPC_RAISEEXCEPTION at :0
#1 CLASSES$_$TFPLIST_$__$$_ERROR$ANSISTRING$INT64+97 at :0
#2 CLASSES$_$TFPLIST_$__$$_CHECKINDEX$LONGINT+40 at :0
#3 CLASSES$_$TFPLIST_$__$$_GET$LONGINT$$POINTER+20 at :0
#4 TCustomFormEditor.DescendFromDesignerBaseClass(TCustomFormEditor($00000000091A5A60), TComponentClass($000000000CDDED10)) at customformeditor.pp:1902
#5 GetDsgnComponentBaseClassname(TClass($000000000CDDED10)) at sourcefilemanager.pas:8472
#6 LoadLFM(TUnitInfo($000000000C0CB050), TCodeBuffer($000000000ADC4D40), [], []) at sourcefilemanager.pas:6493
#7 NewFile(TProjectFileDescriptor($000000000916B850), 'z:\IWProject\unit1.pas', '', [nfIsPartOfProject, nfOpenInEditor, nfSave, nfCreateDefaultSrc], TObject($0000000006CFC0B0)) at sourcefilemanager.pas:2532
#8 TMainIDE.DoNewFile(TMainIDE($0000000006497C40), TProjectFileDescriptor($000000000916B850), 'z:\IWProject\unit1.pas', '', [nfIsPartOfProject, nfOpenInEditor, nfSave, nfCreateDefaultSrc], nil) at main.pp:5823
#9 TLazIDEInterface.DoNewEditorFile(TLazIDEInterface($0000000006497C40), TProjectFileDescriptor($000000000916B850), 'z:\IWProject\unit1.pas', '', [nfIsPartOfProject, nfOpenInEditor, nfSave, nfCreateDefaultSrc]) at lazideintf.pas:824

As you can see, my code calls DoNewEditorFile() when creating a new IntraWeb project and a long sequence of events will trigger the exception.

Is this a known thing? What can be done in this case?

Thanks in advance

[Alex.Machado]

May I give you a suggestion? If you think FDesignerBaseClassesCanCreateForm must have the same number of elements as FDesignerBaseClasses, add this to the code:

Code: Pascal  [Select][+][-]

1.   Assert(FDesignerBaseClasses.Count = FDesignerBaseClassesCanCreateForm.Count);
2.  

Better yet using the ToString() method of each class to give a perfect error message:

Code: Pascal  [Select][+][-]

1.   Assert(FDesignerBaseClasses.Count = FDesignerBaseClassesCanCreateForm.Count, 'FDesignerBaseClasses: ' + FDesignerBaseClasses.ToString + ', FDesignerBaseClassesCanCreateForm: ' + FDesignerBaseClassesCanCreateForm.ToString);
2.  

Where ToString will evidently return the list content in a human readable format.

[bytebites]

Trunk-version has this code in ide/customformeditor.pp

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. var
3.   i: Integer;
4. begin
5.   Result:=-1;
6.   for i:=0 to FDesignerBaseClasses.Count-1 do
7.   begin
8.     if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
9.     begin
10.       if (Result<0)
11.           or (AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
12.         Result:=i;
13.     end;
14.   end;
15. end;
16.  

[Zvoni]

Implicitly relying on short-circuiting the "or"?
Ouch!
I don't like that at all!
Especially as there is a "ticking timebomb" for the first iteration (Result:=-1)

[Thaddy]

It is enough to remove the or and evaluate both separately:

Code: Pascal  [Select][+][-]

1.   if (Result<0 then continue;// iterate next in loop
2.   if (AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
3.         Result:=i;// shouldn't that be exit(i) ????

The second condition will be skipped by continue.
The loop continues until there is a match or finished with -1.(no match)
Someone plz check if my second hunch is correct, assuming continue is used as I suggested:

Code: Pascal  [Select][+][-]

1. exit(i)

On my current machine I do not have Lazarus available to test.
As I read it, the logic is that you can exit the loop when result is not -1.

[bytebites]

Classic code snippet player 

[Thaddy]

Well, norrmally I don't do that... No lazarus present atm.
The logic is not good anyway says my brain compiler.You can ignore it for now. (always continues)

Another snipper player

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. var
3.         i: Integer;
4. begin
5.         Result:=-1;
6.         for i:=0 to FDesignerBaseClasses.Count-1 do
7.                 if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
8.                         if (AClass.InheritsFrom(TClass(FDesignerBaseClasses[i]))) then
9.                                         exit(i);
10. end;

???? Line 8 --> changed result to i.

[Zvoni]

This code is making no sense to me

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. var
3.   i: Integer;
4. begin
5.   Result:=-1;
6.   for i:=0 to FDesignerBaseClasses.Count-1 do
7.   begin
8.     if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
9.     begin
10.       if (Result<0)
11.           or (AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
12.         Result:=i;
13.     end;
14.   end;
15. end;

Execution only enters Line 9 to Line 13 if Line 8 returns True.
Meaning: We have already established that

Code: Pascal  [Select][+][-]

1. TClass(FDesignerBaseClasses[i])

is an Ancestor of "AClass".

So, why are we checking again against
AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))
??
*confused*
This would only make sense if "AClass" has multiple Ancestors/Inheritance

[BrunoK]

I interpret this method as
  Return the highest index in FDesignerBaseClasses[ i ] that is a parent class of aClass. That is either a parent class or the aClass index if in the list.

The index out of range seems curious because normally if Result is < 0 (not set yet), shortcut boolean evaluation should not, even if range check on, use result = -1 value to index the FDesignerBaseClasses list.

I suggest the OP

  tries the following code :

Code: Pascal  [Select][+][-]

1. function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2. var
3.   i: Integer;
4. begin
5.   Result:=-1;
6.   for i:=0 to FDesignerBaseClasses.Count-1 do
7.   begin
8.     if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
9.     begin
10.       if (Result < 0) then
11.         Result := i
12.       else if (AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
13.         Result := i;
14.     end;
15.   end;
16. end;

Difficult to understand what the developer intended when he modified the code.

[Zvoni]

I interpret this method as
  Return the highest index in FDesignerBaseClasses[ i ] that is a parent class of aClass. That is either a parent class or the aClass index if in the list.

The index out of range seems curious because normally if Result is < 0 (not set yet), shortcut boolean evaluation should not, even if range check on, use result = -1 value to index the FDesignerBaseClasses list.

Hmmm.... you mean like Running through the whole Inheritance-tree?

As regards to your Code:
Thaddy has a IMO nicer solution in Reply4 / First Code-Block
EDIT: Thaddy missed, that the "original" code also sets Result to i if Result<0

So i don't understand the very first line "Result:=-1;"....
The Function-Result will never be "-1", since in the very first iteration it checks "Result<0" (which shortcircuits the "or"-ed second check), setting Result to 0 (First value of i)

What IS actually the purpose of this Function?

EDIT2: hmm.... i think i have understood the purpose of the Function....
It tries to find the LAST Ancestor in that List "FDesignerBaseClasses"...

Why not turn it around?
Proposal (untested)

Code: Pascal  [Select][+][-]

1.     function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2.     var
3.       i: Integer;
4.     begin
5.       Result:=-1;
6.       for i:=FDesignerBaseClasses.Count-1 Downto 0 do
7.       begin
8.         if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then Exit(i);        
9.       end;
10.     end;

[Thaddy]

@Zvoni
I believe my second example is correct (where I changed line 8  :
The result stays -1 unless a match is found.
I believe is that is how the loop should be interpreted.
It is also faster and it compiles.

[Zvoni]

@Zvoni
I believe my second example is correct (where I changed line :
The result stays -1 unless a match is found.
I believe is that is how the loop should be interpreted.
It is also faster and it compiles.

Not sure.
Your code jumps out of the Function/returns on the first match in the List.
The "original" (ignoring its weakness and/or convoluted way) and my proposal return the "last" Entry which has a match.

EDIT: My comments and proposal are to Bytebites post Reply #2, quoting the code from Trunk

But funnily enough...

I checked the code of version 3.8 and it's correct:

Code: Pascal  [Select][+][-]

1.     function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2.     begin
3.       Result:=FDesignerBaseClasses.Count-1;
4.       while (Result>=0)
5.       and (not AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) do
6.         dec(Result);
7.     end;

I would still be right, that it returns the "Last match" (Highest Index) in the list, otherwise -1

[BrunoK]

EDIT: Thaddy missed, that the "original" code also sets Result to i if Result<0

Saw that at once ...

So i don't understand the very first line "Result:=-1;"....

Obviously one would want to know that AClass was not in the list.

What IS actually the purpose of this Function?

I don't know in this context. I could imagine good reasons, like short circuiting inheritance calls to virtual methods.

EDIT2: hmm.... i think i have understood the purpose of the Function....
It tries to find the LAST Ancestor in that List "FDesignerBaseClasses"...

Why not turn it around?

Before the new version it was like you suggest but the developer saw things differently..

[Zvoni]

Before the new version it was like you suggest but the developer saw things differently..

Still not making any sense to me.

This is apparently the function in current trunk (See ByteBites post)

Code: Pascal  [Select][+][-]

1.     function TCustomFormEditor.DescendFromDesignerBaseClass(AClass: TComponentClass): integer;
2.     var
3.       i: Integer;
4.     begin
5.       Result:=-1;
6.       for i:=0 to FDesignerBaseClasses.Count-1 do
7.       begin
8.         if AClass.InheritsFrom(TClass(FDesignerBaseClasses[i])) then
9.         begin
10.           if (Result<0)
11.               or (AClass.InheritsFrom(TClass(FDesignerBaseClasses[Result]))) then
12.             Result:=i;
13.         end;
14.       end;
15.     end;

For all intents and purposes let's say "FDesignerBaseClasses" has 12 Entries (From wherever those entries come from)
Result is "-1" from the get go
for i=0, 1, 2 Line 8 returns false (No Match), but on i = 3 there is a Match
Since Result is -1 at that point, left side of the "or" short-circuits (right side is ignored), and sets Result to 3
for i=4,5,6 there is no match again, but on i=7 there is a match
Since Result is now >0 (from iteration i=3) right side of the "or" gets executed, but it checks if there is a match for "Result" (which is "3" in our example) as Index of the List.
WHICH HAS ALREADY BEEN ESTABLISHED AS TRUE on the fourth iteration (i=3), and sets the Result to the "current" i (which is "7").

Irrespective of any intermediary Results: The Function returns the highest Index with a Match (Or -1 if no match at all).

Where is the difference to my Proposal or the Code in Lazarus 3.8?

[Thaddy]

@Zvoni
Either that reversed example you gave or my second example was right.
The original code is wrong: it has no collector,
It must be either first or last or -1.
I still think my second example is right. The other tries make no sense.
BTW my second example works and is now tested,
It ends up at the same index as the reverse you gave.
The other answers have a severe logic problem.
@Bytebites: you did not rebuild Lazarus, did you?