View Issue Details

IDProjectCategoryView StatusLast Update
0038585LazarusIDEpublic2021-03-14 08:44
ReporterMartok Assigned ToJuha Manninen  
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Summary0038585: Crash when reading LFM of Frame that has custom class parent
DescriptionLazarus mis-detects the BaseResourceType of Frames that have a class other than TFrame as their parent, and then fails to load the LFM, as it tries to construct a TForm descendant.

So, this hierarchy results in a crash:
TFrame -> TMyBaseFrame -> TFrame1 {with LFM}

I suspected it is because the intheritance is detected based on class name strings, but loading the next frame down the line does work (after skipping the previous error messages):
TFrame -> TMyBaseFrame -> TFrame1 {with LFM} -> TFrame2 {with LFM}
Steps To ReproduceOpen demo project, observe loading errors when trying to edit TFrame1.

Also, if one ignores the error messages, the LPI is "fixed" to say [ResourceBaseClass Value="Form"], so it's definitely something that gets propagated.

It doesn't matter if TMyBaseFrame is declared in the same or in another unit.
TagsNo tags attached.
Fixed in Revisionr64763, r64769, r64800
LazTarget-
Widgetset
Attached Files

Activities

Martok

2021-03-05 13:55

reporter   ~0129398

Juha Manninen

2021-03-07 14:26

developer   ~0129477

I think I fixed it, amazingly enough.
Please test with different scenarios. At least I didn't see any negative side effects yet.

Martok

2021-03-07 16:55

reporter   ~0129489

Last edited: 2021-03-07 18:45

View 2 revisions

This one is fixed, but inheritance is now further down is broken in the IDE.
For example, drop a control on TFrame1 of the demo project, it doesn't show up on TFrame2.

... and now I'm getting AVs when dropping a Frame on a Form, even in versions that worked a few days ago. Huh.

Edit: the AV is from PkgManager, which fails to locate the unit to add to the uses list and ends up with a null pointer. But that part hasn't fundamentally changed in months, why did that ever work?

Juha Manninen

2021-03-07 20:27

developer   ~0129497

> This one is fixed, but inheritance is now further down is broken in the IDE.
> For example, drop a control on TFrame1 of the demo project, it doesn't show up on TFrame2.

True, I can reproduce this. The fix was not good.

> ... and now I'm getting AVs when dropping a Frame on a Form, even in versions that worked a few days ago. Huh.
> Edit: the AV is from PkgManager, which fails to locate the unit to add to the uses list and ends up with a null pointer.
> But that part hasn't fundamentally changed in months, why did that ever work?

Based on my tests it did not work. I suspected the recent package dependency fix made by Mattias but the AV (Nil reference) came already earlier.
Can you please test again.
It is possible nobody tested with frames, or at least did not report problems with them, until now.

Martok

2021-03-07 22:49

reporter   ~0129500

> Can you please test again.

I absolutely positvely remember placing down KControls' kmemofrm last week without an error but also without the unit being added to the uses list, but I consistently get the AV now. The oldest build archive I have lying around is r63965, and that had the same problem. Guess it must have randomly worked exactly once.

Attached patch just catches the error. As far as I can tell, known frames aren't currently handled for used units updates, that's a separate feature request then.
0038585_crash_frame_drop.patch (423 bytes)   
Index: packager/pkgmanager.pas
===================================================================
--- packager/pkgmanager.pas	(revision 64763)
+++ packager/pkgmanager.pas	(working copy)
@@ -4453,6 +4453,7 @@
   try
     Result:=GetUnitsAndDepsForComps(ComponentClasses, Dependencies, UnitNames);
     if Result<>mrOk then exit;
+    if not Assigned(UnitNames) then exit;
 
     if (Dependencies<>nil) then
     begin

Juha Manninen

2021-03-08 08:25

developer   ~0129502

Last edited: 2021-03-08 08:58

View 2 revisions

> The oldest build archive I have lying around is r63965, and that had the same problem. Guess it must have randomly worked exactly once.

Martok, you really must learn to use revision control tools!
The whole idea of revision control is that you can study and test old revisions. What keeps you from doing that?

The AV actually got fixed by r64765 by Mattias.

Martok

2021-03-08 11:03

reporter   ~0129507

I had written a different reply first, but then the Mantis access token error thing ate it, so here's a rewritten one...

> The whole idea of revision control is that you can study and test old revisions. What keeps you from doing that?
Compiler versions. Running both trunks is usually fine, but sometimes the interactions make bisecting more than a few weeks away from a particular compiler quite difficult. And in this case, just from the git blame it looks like if there ever maybe was a version that correctly added frame units, that was many *years* ago, requiring other compiler (and FCL) major versions. Hardly seems worth it, since we already identified the AV's cause and it is so easily suppressed.

> The AV actually got fixed by r64765 by Mattias.
Given that I did my last tests with 64766: no, that was a different one.
After reading some more of the surroundings: GetUnitsAndDepsForComps should probably return mrIgnore when it doesn't find anything. That can only ever happen for Frames, every other component that passes through there is a registered component.

Might open a new issue for the unit not being added, but that's much less of a problem than the inheritance issue.

Reiner Sombrowsky

2021-03-08 19:06

reporter   ~0129513

Hallo Juha,
I have the same problem.
I use Lazarus Rev. 64768 FPC Rev. 48914.
I have a lot of projects where units derived from TFrame.
If I use a new Unit derived from a unit with TFrame, no
Constrol is derived within TFrame.
I have attached a little project without Bins. The unit2 has
a TFrame container with some controls. If you try to
derive a new unit from unit2 no control is shown.
My last correct Lazarus Rev. was 64760 and Fpc Rev. 48893
test.zip (109,760 bytes)

Juha Manninen

2021-03-08 19:54

developer   ~0129515

Last edited: 2021-03-09 10:26

View 4 revisions

Please test with r64769. I used heuristics based on the class name. If there is text "frame" the type is set accordingly. The exact type was difficult to get right in every case (for me at least).
I also added an Assertion where the AV used to happen. I have not seen since a change from Mattias but I keep looking.

BTW, Having Lazarus trunk + FPC trunk is a challenge. Many moving parts.
When studying a bug in Lazarus source, it makes sense to use a released FPC. For example I am testing with FPC 3.2 now.

Martok

2021-03-10 00:52

reporter   ~0129536

Last edited: 2021-03-11 00:11

View 2 revisions

Hm. My frames are usually called TfrmSomething, so that heuristic immediately doesn't work. But it seems to not work in a benign way and doesn't explode or mess up the LFMs too much.

Do you mind leaving this report open for a while? I think I might have an idea what needs to happen... something more like the first attempt, but with loading dependencies. Will probably take a few days though.

Edit/Update: seems like my idea works, but I'll have to verify a few side conditions first and test with unconventional hierarchies.

Martok

2021-03-12 23:57

reporter   ~0129617

Okay, at the very least I can't find any problems, so there's that...

First patch replaces the assertion in updating the uses list with a safe exit. The only way anything can be dropped on the form that is not found as a registered component is as a Frame. This remains unimplemented, the user just has to add the unit manually. If someone wants to implement that in the future, there is now a TODO comment.

Second patch is the actual Frame loading stuff. This partially reverts r64763 and r64769, and reintroduces a loop based on r64763. There is a comment describing what it does: basically, traverse the class parents until either TFrame/TForm/TDatamodule are recognized by FindBaseComponentClass (then that's the editor) or class backed by a LFM resource is loaded (then that is where inherited components come from and will have located its editor already). Source files are traversed by using pre-existing methods.

There is one new Assertion in FindComponentClass.TryUsedUnitInterface. I'm not 100% sure the UnitInfo for a file always present this point, but I did not find any counterexample, even for files that were not related to the project and only found via search path.
0038585_002_frames_instances.patch (11,586 bytes)   
commit df7766ae247d98e5086d7db778e3ce09305f0411
Author: Martok <martok@martoks-place.de>
Date:   Thu Mar 11 22:28:28 2021 +0100

    IDE: fixed crashes and load errors for Frames and Forms with complex inheritance. Issue #38585.pi

diff --git components/ideintf/unitresources.pas components/ideintf/unitresources.pas
index 2b7c6c6721..5440fb3e5c 100644
--- components/ideintf/unitresources.pas
+++ components/ideintf/unitresources.pas
@@ -23,7 +23,7 @@ uses
   // LCL
   LCLMemManager, Forms, LResources,
   // LazUtils
-  UITypes, LazStringUtils;
+  UITypes;
 
 type
 
@@ -45,7 +45,7 @@ type
       out MissingClasses: TStrings// e.g. MyFrame2:TMyFrame
       ): TModalResult; virtual; abstract;
     class function Priority: integer; virtual; // higher priority is tested first
-    class function DefaultComponentClass(aClassName: string): TComponentClass; virtual;
+    class function DefaultComponentClass: TComponentClass; virtual;
     class function FindComponentClass({%H-}aClassName: string): TComponentClass; virtual;
   end;
   TUnitResourcefileFormatClass = class of TUnitResourcefileFormat;
@@ -62,7 +62,7 @@ type
     class function GetClassNameFromStream(s: TStream; out IsInherited: Boolean): shortstring; override;
     class function CreateReader(s: TStream; var DestroyDriver: boolean): TReader; override;
     class function CreateWriter(s: TStream; var DestroyDriver: boolean): TWriter; override;
-    class function DefaultComponentClass(aClassName: string): TComponentClass; override;
+    class function DefaultComponentClass: TComponentClass; override;
     class function FindComponentClass(aClassName: string): TComponentClass; override;
   end;
 
@@ -150,15 +150,9 @@ begin
   Result := CreateLRSWriter(s, DestroyDriver);
 end;
 
-class function TCustomLFMUnitResourceFileFormat.DefaultComponentClass(
-  aClassName: string): TComponentClass;
-begin   // Use heuristics to get a default class.
-  if PosI('DataModule',aClassName) > 0 then
-    Result:=FormEditingHook.StandardDesignerBaseClasses[DesignerBaseClassId_TDataModule]
-  else if PosI('Frame',aClassName) > 0 then
-    Result:=FormEditingHook.StandardDesignerBaseClasses[DesignerBaseClassId_TFrame]
-  else
-    Result := FormEditingHook.StandardDesignerBaseClasses[DesignerBaseClassId_TForm];
+class function TCustomLFMUnitResourceFileFormat.DefaultComponentClass: TComponentClass;
+begin
+  Result := FormEditingHook.StandardDesignerBaseClasses[DesignerBaseClassId_TForm];
 end;
 
 class function TCustomLFMUnitResourceFileFormat.FindComponentClass(
@@ -181,7 +175,7 @@ begin
   Result:=0;
 end;
 
-class function TUnitResourcefileFormat.DefaultComponentClass(aClassName: string): TComponentClass;
+class function TUnitResourcefileFormat.DefaultComponentClass: TComponentClass;
 begin
   Result:=TForm;
 end;
diff --git ide/sourcefilemanager.pas ide/sourcefilemanager.pas
index fe1dbc123c..6ec5ba1b3e 100644
--- ide/sourcefilemanager.pas
+++ ide/sourcefilemanager.pas
@@ -49,14 +49,14 @@ uses
   NewItemIntf, ProjectIntf, PackageIntf, PackageDependencyIntf, IDEExternToolIntf,
   // IdeIntf
   IDEDialogs, PropEdits, IDEMsgIntf, LazIDEIntf, MenuIntf, IDEWindowIntf, FormEditingIntf,
-  ObjectInspector, ComponentReg, SrcEditorIntf, EditorSyntaxHighlighterDef,
+  ObjectInspector, ComponentReg, SrcEditorIntf, EditorSyntaxHighlighterDef, UnitResources,
   // IDE
   IDEProcs, DialogProcs, IDEProtocol, LazarusIDEStrConsts, NewDialog, NewProjectDlg,
   MainBase, MainBar, MainIntf, Project, ProjectDefs, ProjectInspector, CompilerOptions,
   SourceSynEditor, SourceEditor, EditorOptions, EnvironmentOpts, CustomFormEditor,
   ControlSelection, FormEditor, EmptyMethodsDlg, BaseDebugManager, TransferMacros,
   BuildManager, EditorMacroListViewer, FindRenameIdentifier, BuildModesManager,
-  ViewUnit_Dlg, InputHistory, CheckLFMDlg, etMessagesWnd, UnitResources,
+  ViewUnit_Dlg, InputHistory, CheckLFMDlg, etMessagesWnd,
   ConvCodeTool, BasePkgManager, PackageDefs, PackageSystem, Designer, DesignerProcs;
 
 type
@@ -6438,15 +6438,12 @@ function FindBaseComponentClass(AnUnitInfo: TUnitInfo; const AComponentClassName
   DescendantClassName: string; out AComponentClass: TComponentClass): boolean;
 // returns false if an error occured
 // Important: returns true even if AComponentClass=nil
-var
-  ResFormat: TUnitResourcefileFormatClass;
 begin
   AComponentClass:=nil;
   // find the ancestor class
-  ResFormat:=AnUnitInfo.UnitResourceFileformat;
-  if ResFormat<>nil then
+  if AnUnitInfo.UnitResourceFileformat<>nil then
   begin
-    AComponentClass:=ResFormat.FindComponentClass(AComponentClassName);
+    AComponentClass:=AnUnitInfo.UnitResourceFileformat.FindComponentClass(AComponentClassName);
     if AComponentClass<>nil then
       exit(true);
   end;
@@ -6484,65 +6481,93 @@ function LoadAncestorDependencyHidden(AnUnitInfo: TUnitInfo;
   out AncestorClass: TComponentClass;
   out AncestorUnitInfo: TUnitInfo): TModalResult;
 var
-  AncestorClsName, S: String;
+  AncsClsName, IgnoreBtnText, ClsName: String;
   CodeBuf: TCodeBuffer;
   GrandAncestorClass, DefAncestorClass: TComponentClass;
   ResFormat: TUnitResourcefileFormatClass;
+  ClsUnitInfo: TUnitInfo;
 begin
   AncestorClass:=nil;
   AncestorUnitInfo:=nil;
 
-  // find the ancestor type in the source
-  if AnUnitInfo.Source=nil then begin
-    Result:=LoadCodeBuffer(CodeBuf,AnUnitInfo.Filename,
-                           [lbfUpdateFromDisk,lbfCheckIfText],true);
-    if Result<>mrOk then exit;
-    AnUnitInfo.Source:=CodeBuf;
-  end;
-
-  if not CodeToolBoss.FindFormAncestor(AnUnitInfo.Source,aComponentClassName,
-                                       AncestorClsName,true) then
-    DebugLn('LoadAncestorDependencyHidden Filename="',AnUnitInfo.Filename,'" ClassName=',aComponentClassName,
-            '. Unable to find ancestor class: ',CodeToolBoss.ErrorMessage);
-  // try the base designer classes
-  if not FindBaseComponentClass(AnUnitInfo,AncestorClsName,aComponentClassName,
-                                AncestorClass) then
-  begin
-    DebugLn(['LoadAncestorDependencyHidden FindUnitComponentClass failed for AncestorClsName=',AncestorClsName]);
-    exit(mrCancel);
-  end;
-
-  // try loading the ancestor first (unit, lfm and component instance)
-  ResFormat:=AnUnitInfo.UnitResourceFileformat;
+  // fallback ancestor is defined by the resource file format of the form/frame/component being loaded
+  // this is offered to the user in case a lookup fails
+  ResFormat:= AnUnitInfo.UnitResourceFileformat;
   if ResFormat<>nil then
-    DefAncestorClass:=ResFormat.DefaultComponentClass(aComponentClassName)
+    DefAncestorClass:=ResFormat.DefaultComponentClass
   else
+    DefAncestorClass:=nil;
+  // use TForm as default ancestor
+  if DefAncestorClass=nil then
     DefAncestorClass:=BaseFormEditor1.StandardDesignerBaseClasses[DesignerBaseClassId_TForm];
-
-  if (AncestorClass=nil) then begin
-    S:='';
-    if DefAncestorClass<>nil then
-      S:=Format(lisIgnoreUseAsAncestor, [DefAncestorClass.ClassName]);
-    Result:=LoadComponentDependencyHidden(AnUnitInfo,AncestorClsName,OpenFlags,
-                      false,AncestorClass,AncestorUnitInfo,GrandAncestorClass,S);
-    if Result<>mrOk then begin
-      DebugLn(['LoadAncestorDependencyHidden DoLoadComponentDependencyHidden failed AnUnitInfo=',AnUnitInfo.Filename]);
+  IgnoreBtnText:='';
+  if DefAncestorClass<>nil then
+    IgnoreBtnText:=Format(lisIgnoreUseAsAncestor, [DefAncestorClass.ClassName]);
+
+  // traverse the chain of ancestors until either:
+  //   - an error occurs
+  //   - FindBaseComponentClass locates an editor class
+  //   - LoadComponentDependencyHidden loads a full LFM
+  //   - no further class parents exist
+  ClsName:=aComponentClassName;
+  ClsUnitInfo:=AnUnitInfo;
+  repeat
+    // if Source is not already loaded, load from Filename
+    if not Assigned(ClsUnitInfo.Source) then begin
+      Result:=LoadCodeBuffer(CodeBuf,ClsUnitInfo.Filename,
+                             [lbfUpdateFromDisk,lbfCheckIfText],true);
+      if Result<>mrOk then exit;
+      ClsUnitInfo.Source:=CodeBuf;
     end;
-    case Result of
-    mrAbort: exit;
-    mrOk: ;
-    mrIgnore:
-      AncestorUnitInfo:=nil;
-    else
-      // cancel
-      Result:=mrCancel;
-      exit;
+
+    // get ancestor classname from current Source
+    if CodeToolBoss.FindFormAncestor(ClsUnitInfo.Source,ClsName,AncsClsName,true) then begin
+      // is this ancestor a designer class?
+      if not FindBaseComponentClass(ClsUnitInfo,AncsClsName,ClsName,AncestorClass) then
+      begin
+        DebugLn(['LoadAncestorDependencyHidden FindUnitComponentClass failed for AncsClsName=',AncsClsName]);
+        exit(mrCancel);
+      end;
+      // ancestor is not a designer class, go to next ancestor
+      ClsName:=AncsClsName;
+    end else begin
+      // the declaration of ClsName is not in this Unit
+      DebugLn('LoadAncestorDependencyHidden Filename="',ClsUnitInfo.Filename,'" ClassName=',aComponentClassName, '. Unable to find ancestor class: ',CodeToolBoss.ErrorMessage);
+
+      // try loading the ancestor (unit, lfm and component instance)
+      Result:=LoadComponentDependencyHidden(ClsUnitInfo,ClsName,
+               OpenFlags,false,AncestorClass,AncestorUnitInfo,GrandAncestorClass,
+               IgnoreBtnText);
+      if Result<>mrOk then begin
+        DebugLn(['LoadAncestorDependencyHidden DoLoadComponentDependencyHidden failed ClsUnitInfo=',ClsUnitInfo.Filename]);
+      end;
+      case Result of
+      mrAbort: exit;
+      mrOk: ;
+      mrIgnore: break;
+      else
+        // cancel
+        Result:=mrCancel;
+        exit;
+      end;
+      // LoadComponentDependencyHidden loaded something and got a component class for it, everything is set and we are done here
+      if Assigned(AncestorClass) then
+        break
+      else begin
+        // otherwise, we have loaded the unit containing ClsName, but do not yet know what it is
+        // let FindFormAncestor/FindBaseComponentClass try again
+        ClsUnitInfo:= AncestorUnitInfo;
+      end;
     end;
-  end;
+  until Assigned(AncestorClass) or (ClsName = '');
 
-  //DebugLn('LoadAncestorDependencyHidden Filename="',AnUnitInfo.Filename,'" AncestorClsName=',AncestorClsName,' AncestorClass=',dbgsName(AncestorClass));
-  if AncestorClass=nil then
+  if AncestorClass=nil then begin
+    // nothing was found, clear any attempted references
+    AncestorUnitInfo:= nil;
+
+    //DebugLn('LoadAncestorDependencyHidden Filename="',ClsUnitInfo.Filename,'" AncsClsName=',AncsClsName,' AncestorClass=',dbgsName(AncestorClass));
     AncestorClass:=DefAncestorClass;
+  end;
   Result:=mrOk;
 end;
 
@@ -6878,6 +6903,9 @@ var
       {$ENDIF}
       exit;
     end;
+    // Component was located, save UnitInfo for return regardless of AncestorClass resolution
+    ComponentUnitInfo:= Project1.UnitInfoWithFilename(UnitFilename);
+    Assert(Assigned(ComponentUnitInfo), 'FindComponentClass.TryUsedUnitInterface: Declaration is in a unit that was not previously known');
     if TryRegisteredClasses(AncestorClassName,AncestorClass,TheModalResult) then
       exit(true);
   end;
@@ -7062,6 +7090,7 @@ begin
   AComponentClass:=nil;
   Quiet:=([ofProjectLoading,ofQuiet]*Flags<>[]);
   HideAbort:=not (ofProjectLoading in Flags);
+
 {  Will be checked in FindComponentClass()
   if not IsValidIdent(AComponentClassName) then
   begin
@@ -7069,6 +7098,7 @@ begin
     exit(mrCancel);
   end;
 }
+
   // check for cycles
   if AnUnitInfo.LoadingComponent then begin
     Result:=IDEQuestionDialogAb(lisCodeTemplError,
0038585_001_add_unit.patch (849 bytes)   
commit 7940b12dbf9b29b82cc546ac2678750f8378156e
Author: Martok <martok@martoks-place.de>
Date:   Thu Mar 11 21:42:31 2021 +0100

    IDE: fixed crash on placing TFrame instance. Issue #38585.

diff --git packager/pkgmanager.pas packager/pkgmanager.pas
index 8e5cd18b2d..db7631f184 100644
--- packager/pkgmanager.pas
+++ packager/pkgmanager.pas
@@ -4453,7 +4453,9 @@ begin
   try
     Result:=GetUnitsAndDepsForComps(ComponentClasses, Dependencies, UnitNames);
     if Result<>mrOk then exit;
-    Assert(Assigned(UnitNames), 'TPkgManager.AddUnitDepsForCompClasses: UnitNames=Nil.');
+    // TODO: Frame instances are not registered components, UnitNames is not assigned
+    if (UnitNames=nil) then exit(mrCancel);
+
     if (Dependencies<>nil) then
     begin
       Result:=FilterMissingDepsForUnit(UnitFilename,Dependencies,MissingDependencies);
0038585_001_add_unit.patch (849 bytes)   

Juha Manninen

2021-03-13 08:34

developer   ~0129622

Last edited: 2021-03-13 11:48

View 2 revisions

Did the Assert(Assigned(UnitNames)... trigger for you in some situation?

The actual patch causes an access violation with TSourceNotebook = class(TSourceEditorWindowInterface).
TSourceEditorWindowInterface inherits from TForm.
The code gets confused, does 3 loops and crashes.
To reproduce:
Open project lazarus.lpi in Lazarus IDE.
Open unit SourceEditor from directory ide/.

Martok

2021-03-13 16:07

reporter   ~0129630

Last edited: 2021-03-13 16:14

View 2 revisions

> Did the Assert(Assigned(UnitNames)... trigger for you in some situation?
Yes, 100% reproducible. This was the AV I mentioned in 0038585:0129489. Place frame component on editor -> AV.

> The actual patch causes an access violation with TSourceNotebook = class(TSourceEditorWindowInterface).
Interesting, I had the same constellation in test projects, but something is different here.
This might take a while: something else broke debugging the IDE again, breakpoints don't work today. (Edit: for me. I'm pretty sure this is just a local problem.)

Juha Manninen

2021-03-13 17:41

reporter   ~0129633

> Place frame component on editor -> AV.

Ok, now I can reproduce it. I don't know what happened last time.

> something else broke debugging the IDE again, breakpoints don't work today.

Oh, on my Linux it works well. What is your platform? You may want to try FpDebug if GDB does not work.

Martok

2021-03-13 18:32

reporter   ~0129634

Last edited: 2021-03-13 18:42

View 3 revisions

> Oh, on my Linux it works well. What is your platform? You may want to try FpDebug if GDB does not work.
I have never managed to get fpdebug to work, at all. GDB mostly works (for "gdb" values of "works"), so I've never investigated. In this case, clean checkout and rebuilding helped.

I've located the problem with the patch, FindComponentClass has way more paths that don't return the UnitInfo than I thought.
~There are multiple comments on what "AComponentClass<>nil and ComponentUnitInfo=nil" means, do you know if anything relies on that? Nothing in the main source tree does, is this something a user IDE package could access? Changing that would mean I don't have to reinvent the class searching wheel.~ Scratch that, the fact that there are other code paths returning ComponentUnitInfo=nil is already a deviation from these comments. Also, nevermind that, I'm being daft. Testing now...

Martok

2021-03-14 01:21

reporter   ~0129643

Last edited: 2021-03-14 01:22

View 2 revisions

Right, "open every single Form and Frame of the IDE" now works correctly. There's 161 of them, by the way :D

Turns out "I'm not 100% sure the UnitInfo for a file always present" was excactly the problem. If at any point the required file was opened in the editor (even if closed later), it would have worked.

I've also unwrapped the loader loop a bit for readability, no functional changes.

Patch applies on top of the previous ones just for easier review, feel free to squash for SVN.
0038585_003_frames_units.patch (7,774 bytes)   
commit 08840bb9b280d2908caed9bcf3bad3d86f482b65
Author: Martok <martok@martoks-place.de>
Date:   Sun Mar 14 00:05:36 2021 +0100

    IDE: fix locating class dependencies

diff --git ide/sourcefilemanager.pas ide/sourcefilemanager.pas
index 1266304a72..0a6c364e23 100644
--- ide/sourcefilemanager.pas
+++ ide/sourcefilemanager.pas
@@ -6514,9 +6514,11 @@ begin
                              [lbfUpdateFromDisk,lbfCheckIfText],true);
       if Result<>mrOk then exit;
       ClsUnitInfo.Source:=CodeBuf;
+      if FilenameIsPascalSource(ClsUnitInfo.Filename) then
+        ClsUnitInfo.ReadUnitNameFromSource(true);
     end;
 
-    // get ancestor classname from current Source
+    // get ancestor of ClsName from current ClsUnitInfo
     if CodeToolBoss.FindFormAncestor(ClsUnitInfo.Source,ClsName,AncsClsName,true) then begin
       // is this ancestor a designer class?
       if not FindBaseComponentClass(ClsUnitInfo,AncsClsName,ClsName,AncestorClass) then
@@ -6524,38 +6526,46 @@ begin
         DebugLn(['LoadAncestorDependencyHidden FindUnitComponentClass failed for AncsClsName=',AncsClsName]);
         exit(mrCancel);
       end;
-      // ancestor is not a designer class, go to next ancestor
-      ClsName:=AncsClsName;
-    end else begin
-      // the declaration of ClsName is not in this Unit
-      DebugLn('LoadAncestorDependencyHidden Filename="',ClsUnitInfo.Filename,'" ClassName=',aComponentClassName, '. Unable to find ancestor class: ',CodeToolBoss.ErrorMessage);
 
-      // try loading the ancestor (unit, lfm and component instance)
-      Result:=LoadComponentDependencyHidden(ClsUnitInfo,ClsName,
-               OpenFlags,false,AncestorClass,AncestorUnitInfo,GrandAncestorClass,
-               IgnoreBtnText);
-      if Result<>mrOk then begin
-        DebugLn(['LoadAncestorDependencyHidden DoLoadComponentDependencyHidden failed ClsUnitInfo=',ClsUnitInfo.Filename]);
-      end;
-      case Result of
-      mrAbort: exit;
-      mrOk: ;
-      mrIgnore: break;
-      else
-        // cancel
-        Result:=mrCancel;
-        exit;
-      end;
-      // LoadComponentDependencyHidden loaded something and got a component class for it, everything is set and we are done here
       if Assigned(AncestorClass) then
         break
       else begin
-        // otherwise, we have loaded the unit containing ClsName, but do not yet know what it is
-        // let FindFormAncestor/FindBaseComponentClass try again
-        ClsUnitInfo:= AncestorUnitInfo;
+        // immediately go to next ancestor
+        ClsName:=AncsClsName;
+        continue;
       end;
     end;
-  until Assigned(AncestorClass) or (ClsName = '');
+    // -> the declaration of ClsName is not in ClsUnitInfo, let LoadComponentDependencyHidden locate it
+
+    // try loading the ancestor (unit, lfm and component instance)
+    Result:=LoadComponentDependencyHidden(ClsUnitInfo,ClsName,
+             OpenFlags,false,AncestorClass,AncestorUnitInfo,GrandAncestorClass,
+             IgnoreBtnText);
+    if Result<>mrOk then begin
+      DebugLn(['LoadAncestorDependencyHidden DoLoadComponentDependencyHidden failed ClsUnitInfo=',ClsUnitInfo.Filename]);
+    end;
+    case Result of
+    mrAbort: exit;
+    mrOk: ;
+    mrIgnore: break;
+    else
+      // cancel
+      exit(mrCancel);
+    end;
+
+    // possible outcomes of LoadComponentDependencyHidden:
+    if Assigned(AncestorClass) then
+      // loaded something and got a component class for it -> done, everything is set
+      break
+    else if Assigned(AncestorUnitInfo) then
+      // loaded the unit containing ClsName, but it does not have the ComponentClass -> let FindFormAncestor/FindBaseComponentClass try again
+      ClsUnitInfo:= AncestorUnitInfo
+    else begin
+      // likely a bug: declaration is nowhere and was not caught by user interaction in LoadComponentDependencyHidden
+      DebugLn(['LoadAncestorDependencyHidden DoLoadComponentDependencyHidden empty returns for ClsName=',ClsName, ' ClsUnitInfo=',ClsUnitInfo.Filename]);
+      exit(mrCancel);
+    end;
+  until Assigned(AncestorClass) or (ClsName = '') or not Assigned(ClsUnitInfo);
 
   if AncestorClass=nil then begin
     // nothing was found, clear any attempted references
@@ -6576,6 +6586,8 @@ function FindComponentClass(AnUnitInfo: TUnitInfo; const AComponentClassName: st
       designer component
    - AComponentClass<>nil and ComponentUnitInfo=nil
       registered componentclass
+   - AComponentClass=nil and ComponentUnitInfo<>nil
+      componentclass does not exist, but the unit declaring AComponentClassName was found
    - LFMFilename<>''
       lfm of an used unit
    - AncestorClass<>nil
@@ -6720,6 +6732,19 @@ var
     Result:=true;
   end;
 
+  procedure StoreComponentClassDeclaration(UnitFilename: string);
+  begin
+    // The Unit declaring AComponentClassName was located, save UnitInfo for return regardless of AComponentClass instance
+    ComponentUnitInfo:= Project1.UnitInfoWithFilename(UnitFilename);
+    if not Assigned(ComponentUnitInfo) then begin
+      // File was not previously loaded, add reference to project (without loading source for now)
+      ComponentUnitInfo:=TUnitInfo.Create(nil);
+      ComponentUnitInfo.Filename:=UnitFilename;
+      ComponentUnitInfo.IsPartOfProject:=false;
+      Project1.AddFile(ComponentUnitInfo,false);
+    end;
+  end;
+
   function TryFindDeclaration(out TheModalResult: TModalResult): boolean;
   var
     Tool: TCodeTool;
@@ -6839,6 +6864,7 @@ var
         debugln(['FindComponentClass ',AComponentClassName,' is not a TComponent at ',NewTool.CleanPosToStr(NewNode.StartPos,true)]);
         exit;
       end;
+      StoreComponentClassDeclaration(NewTool.MainFilename);
       AncestorNode:=InheritedNode.FirstChild;
       AncestorClassName:=GetIdentifier(@NewTool.Src[AncestorNode.StartPos]);
       //debugln(['TryFindDeclaration declaration of ',AComponentClassName,' found at ',NewTool.CleanPosToStr(NewNode.StartPos),' ancestor="',AncestorClassName,'"']);
@@ -6899,9 +6925,7 @@ var
       {$ENDIF}
       exit;
     end;
-    // Component was located, save UnitInfo for return regardless of AncestorClass resolution
-    ComponentUnitInfo:= Project1.UnitInfoWithFilename(UnitFilename);
-    Assert(Assigned(ComponentUnitInfo), 'FindComponentClass.TryUsedUnitInterface: Declaration is in a unit that was not previously known');
+    StoreComponentClassDeclaration(UnitFilename);
     if TryRegisteredClasses(AncestorClassName,AncestorClass,TheModalResult) then
       exit(true);
   end;
@@ -6930,7 +6954,7 @@ begin
   {$ifdef VerboseFormEditor}
   debugln('FindComponentClass START ',AnUnitInfo.Filename,' AComponentClassName=',AComponentClassName);
   {$endif}
-  // first search the resource of ComponentUnitInfo
+  // first search the resource of AnUnitInfo
   if AnUnitInfo<>nil then begin
     if TryUnitComponent(AnUnitInfo.Filename,Result) then exit;
   end;
@@ -6994,6 +7018,8 @@ function LoadComponentDependencyHidden(AnUnitInfo: TUnitInfo;
       designer component
    - AComponentClass<>nil and ComponentUnitInfo=nil
       registered componentclass
+   - AComponentClass=nil and ComponentUnitInfo<>nil
+      componentclass does not exist, but the unit declaring AComponentClassName was found
    - Only for MustHaveLFM=false: AncestorClass<>nil
       componentclass does not exist, but the ancestor is a registered class
   mrCancel:
@@ -7121,6 +7147,8 @@ begin
     //   designer component
     //- AComponentClass<>nil and ComponentUnitInfo=nil
     //   registered componentclass
+    //- AComponentClass=nil and ComponentUnitInfo<>nil
+    //   componentclass does not exist, but the unit declaring AComponentClassName was found
     //- LFMFilename<>''
     //   lfm of an used unit
     //- AncestorClass<>nil
0038585_003_frames_units.patch (7,774 bytes)   

Juha Manninen

2021-03-14 08:44

reporter   ~0129644

Applied, thanks.
It works well. I had a similar idea but it was complicated to implement. You made it however. Cool!

Issue History

Date Modified Username Field Change
2021-03-05 13:54 Martok New Issue
2021-03-05 13:55 Martok Note Added: 0129398
2021-03-05 13:55 Martok File Added: b0038585_frameinherit.zip
2021-03-07 14:24 Juha Manninen Assigned To => Juha Manninen
2021-03-07 14:24 Juha Manninen Status new => assigned
2021-03-07 14:26 Juha Manninen Status assigned => resolved
2021-03-07 14:26 Juha Manninen Resolution open => fixed
2021-03-07 14:26 Juha Manninen Fixed in Revision => r64763
2021-03-07 14:26 Juha Manninen LazTarget => -
2021-03-07 14:26 Juha Manninen Note Added: 0129477
2021-03-07 16:55 Martok Status resolved => assigned
2021-03-07 16:55 Martok Resolution fixed => open
2021-03-07 16:55 Martok Note Added: 0129489
2021-03-07 18:45 Martok Note Edited: 0129489 View Revisions
2021-03-07 20:27 Juha Manninen Note Added: 0129497
2021-03-07 22:49 Martok Note Added: 0129500
2021-03-07 22:49 Martok File Added: 0038585_crash_frame_drop.patch
2021-03-08 08:25 Juha Manninen Note Added: 0129502
2021-03-08 08:58 Juha Manninen Note Edited: 0129502 View Revisions
2021-03-08 11:03 Martok Note Added: 0129507
2021-03-08 19:06 Reiner Sombrowsky Note Added: 0129513
2021-03-08 19:06 Reiner Sombrowsky File Added: test.zip
2021-03-08 19:54 Juha Manninen Note Added: 0129515
2021-03-08 19:57 Juha Manninen Note Edited: 0129515 View Revisions
2021-03-09 10:15 Juha Manninen Note Edited: 0129515 View Revisions
2021-03-09 10:16 Juha Manninen Status assigned => feedback
2021-03-09 10:26 Juha Manninen Note Edited: 0129515 View Revisions
2021-03-10 00:52 Martok Note Added: 0129536
2021-03-10 00:52 Martok Status feedback => assigned
2021-03-11 00:11 Martok Note Edited: 0129536 View Revisions
2021-03-12 23:57 Martok Note Added: 0129617
2021-03-12 23:57 Martok File Added: 0038585_002_frames_instances.patch
2021-03-12 23:57 Martok File Added: 0038585_001_add_unit.patch
2021-03-13 08:34 Juha Manninen Note Added: 0129622
2021-03-13 11:48 Juha Manninen Note Edited: 0129622 View Revisions
2021-03-13 11:49 Juha Manninen Status assigned => feedback
2021-03-13 16:07 Martok Note Added: 0129630
2021-03-13 16:07 Martok Status feedback => assigned
2021-03-13 16:14 Martok Note Edited: 0129630 View Revisions
2021-03-13 17:41 Juha Manninen Note Added: 0129633
2021-03-13 18:32 Martok Note Added: 0129634
2021-03-13 18:37 Martok Note Edited: 0129634 View Revisions
2021-03-13 18:42 Martok Note Edited: 0129634 View Revisions
2021-03-14 01:21 Martok Note Added: 0129643
2021-03-14 01:21 Martok File Added: 0038585_003_frames_units.patch
2021-03-14 01:22 Martok Note Edited: 0129643 View Revisions
2021-03-14 08:44 Juha Manninen Status assigned => resolved
2021-03-14 08:44 Juha Manninen Resolution open => fixed
2021-03-14 08:44 Juha Manninen Fixed in Revision r64763 => r64763, r64769, r64800
2021-03-14 08:44 Juha Manninen Note Added: 0129644