Skip to content

Commit d78fe51

Browse files
committed
fix(detectors/routine-result): recognise var Result argument as assignment
The RoutineResultUnassigned detector flagged this real-world pattern (spotted on horse-master Horse.Core.Param.pas:105): function THorseCoreParam.GetItem(const AKey: string): string; begin FParams.TryGetValue(AKey, Result); // var-param write end; `Result` is written through TryGetValue's `var AValue` parameter - the function returns a defined value (either the looked-up entry or the default-initialised empty string). The detector's heuristic only looked for `Result := ...` or `<FuncName> := ...` direct assignments and missed the var-param case. Fix: after the assign-loop, also scan nkCall nodes in the method. For each call, parse out the argument list (from nkCall.Name which holds the full call expression text like `Foo(arg1,arg2)`) and check whether `Result` (or the function name) appears as a word-bounded identifier inside. If yes, treat it as a potential write and skip the finding. Trade-off: lexically we can't distinguish var/out/by-value parameters, so calls like `Foo(Result)` where Foo takes Result by value (read-only) now get a free pass even though Result is actually only being read. Accepted - eliminates the FP cluster for TryGetValue / TryStrToInt / TryParse... at small false-negative cost for the rarer "Result passed by value as an arg, never written" pattern. 4 new tests: - TryGetValuePassesResult_NoFinding (the actual horse-master case) - TryStrToIntPassesResult_NoFinding (RTL Try* pattern) - CallPassesResultFollowedByOther_NoFinding (mid-arg position) - UnrelatedVarSimilarName_StillReported (word-boundary guard: `ResultCache` must not silence the finding)
1 parent e64f6bb commit d78fe51

3 files changed

Lines changed: 142 additions & 2 deletions

File tree

StaticCodeAnalyser.d12.groupproj.local

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,6 @@
22
<BorlandProject>
33
<Transactions/>
44
<Default.Personality>
5-
<Projects ActiveProject="D:\git-demos\delphi\StaticCodeAnalyser\StaticCodeAnalyserForm\StaticCodeAnalyser.d12.dproj"/>
5+
<Projects ActiveProject="D:\git-demos\delphi\StaticCodeAnalyser\StaticCodeAnalyserForm\tests\TestProject.dproj"/>
66
</Default.Personality>
77
</BorlandProject>

StaticCodeAnalyserForm/sources/Detectors/uRoutineResultAssigned.pas

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
interface
5151

5252
uses
53-
System.SysUtils, System.Generics.Collections,
53+
System.SysUtils, System.StrUtils, System.Generics.Collections,
5454
uAstNode, uSCAConsts, uMethodd12;
5555

5656
type
@@ -176,6 +176,59 @@ function IsResultLhs(const LhsLow, FnNameLow: string): Boolean;
176176
Result := IsHeadOrAccess('result') or IsHeadOrAccess(FnNameLow);
177177
end;
178178

179+
function IsIdentChar(c: Char): Boolean; inline;
180+
begin
181+
Result := CharInSet(c, ['A'..'Z', 'a'..'z', '0'..'9', '_']);
182+
end;
183+
184+
// Word-boundary-Lookup von Needle in Haystack. Beide bereits lowercased.
185+
// Behandelt `Result.Field`, `Result[i]`, `Result^` als Treffer
186+
// (`.`, `[`, `^` sind keine Identifier-Chars und zaehlen als Boundary).
187+
function ContainsIdentifier(const Haystack, Needle: string): Boolean;
188+
var
189+
pIx : Integer;
190+
Before, After : Char;
191+
begin
192+
Result := False;
193+
if Needle = '' then Exit;
194+
pIx := Pos(Needle, Haystack);
195+
while pIx > 0 do
196+
begin
197+
if pIx = 1 then Before := ' ' else Before := Haystack[pIx - 1];
198+
if pIx + Length(Needle) > Length(Haystack) then After := ' '
199+
else After := Haystack[pIx + Length(Needle)];
200+
if (not IsIdentChar(Before)) and (not IsIdentChar(After)) then
201+
Exit(True);
202+
pIx := PosEx(Needle, Haystack, pIx + 1);
203+
end;
204+
end;
205+
206+
// True wenn der Call Result (oder den Function-Namen) als Argument
207+
// uebergibt - z.B. `FParams.TryGetValue(AKey, Result)` oder
208+
// `TryStrToInt(s, Result)`. Wir wissen lexisch nicht ob das Argument
209+
// `var`/`out`/by-value ist, behandeln das aber konservativ als potentielle
210+
// Zuweisung. Eliminiert den FP-Cluster bei TryGetValue / TryParse /
211+
// TryStrToXxx / TryEncode... bei winzigem False-Negative-Risiko fuer
212+
// `Foo(Result)`-Calls die Result nur LESEN.
213+
function CallPassesResultAsArg(CallNode: TAstNode; const FnNameLow: string): Boolean;
214+
var
215+
S, ArgsLow : string;
216+
LParen, RParen: Integer;
217+
begin
218+
Result := False;
219+
if CallNode.Kind <> nkCall then Exit;
220+
S := CallNode.Name;
221+
LParen := Pos('(', S);
222+
if LParen = 0 then Exit;
223+
RParen := Length(S);
224+
while (RParen > LParen) and (S[RParen] <> ')') do Dec(RParen);
225+
if RParen <= LParen + 1 then Exit;
226+
ArgsLow := LowerCase(Copy(S, LParen + 1, RParen - LParen - 1));
227+
Result := ContainsIdentifier(ArgsLow, 'result');
228+
if (not Result) and (FnNameLow <> '') then
229+
Result := ContainsIdentifier(ArgsLow, FnNameLow);
230+
end;
231+
179232
class procedure TRoutineResultAssignedDetector.AnalyzeMethod(MethodNode: TAstNode;
180233
const FileName: string; Results: TObjectList<TLeakFinding>);
181234
var
@@ -230,6 +283,25 @@ class procedure TRoutineResultAssignedDetector.AnalyzeMethod(MethodNode: TAstNod
230283
Assigns.Free;
231284
end;
232285

286+
// Ergaenzung: Result koennte auch via var/out-Parameter an einen Call
287+
// geschrieben werden (`TryGetValue(Key, Result)`, `TryStrToInt(s, Result)`,
288+
// ...). Lexisch nicht von by-value-Pass unterscheidbar; konservativ als
289+
// potentielle Zuweisung behandeln um FP-Cluster zu eliminieren.
290+
if not HasResult then
291+
begin
292+
Assigns := MethodNode.FindAll(nkCall);
293+
try
294+
for N in Assigns do
295+
if CallPassesResultAsArg(N, FnNameLow) then
296+
begin
297+
HasResult := True;
298+
Break;
299+
end;
300+
finally
301+
Assigns.Free;
302+
end;
303+
end;
304+
233305
if HasResult then Exit;
234306

235307
F := TLeakFinding.Create;

StaticCodeAnalyserForm/tests/uTestRoutineResultAssigned.pas

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ TTestRoutineResultAssigned = class
2828
[Test] procedure RecordResult_FieldAssign_NoFinding;
2929
[Test] procedure ArrayResult_IndexAssign_NoFinding;
3030
[Test] procedure FnNameDotField_NoFinding;
31+
[Test] procedure TryGetValuePassesResult_NoFinding;
32+
[Test] procedure TryStrToIntPassesResult_NoFinding;
33+
[Test] procedure CallPassesResultFollowedByOther_NoFinding;
34+
[Test] procedure UnrelatedVarSimilarName_StillReported;
3135

3236
// ---- Finding-Inhalt ----------------------------------------------------
3337
[Test] procedure Finding_KindAndSeverity;
@@ -252,6 +256,70 @@ procedure TTestRoutineResultAssigned.FnNameDotField_NoFinding;
252256
finally F.Free; end;
253257
end;
254258

259+
procedure TTestRoutineResultAssigned.TryGetValuePassesResult_NoFinding;
260+
// Original-FP aus horse-master/Horse.Core.Param.pas:
261+
// Result wird via `var`-Parameter an TryGetValue uebergeben - das ist
262+
// eine Zuweisung von der Callee-Seite, der Detector muss sie erkennen.
263+
const SRC =
264+
'unit t; implementation'#13#10 +
265+
'function GetItem(const AKey: string): string;'#13#10 +
266+
'begin'#13#10 +
267+
' FParams.TryGetValue(AKey, Result);'#13#10 +
268+
'end;';
269+
var F: TObjectList<TLeakFinding>;
270+
begin
271+
F := TFindingHelper.FindingsOf(SRC);
272+
try Assert.AreEqual(0, TFindingHelper.Count(F, fkRoutineResultUnassigned));
273+
finally F.Free; end;
274+
end;
275+
276+
procedure TTestRoutineResultAssigned.TryStrToIntPassesResult_NoFinding;
277+
// RTL-Standard: TryStrToInt schreibt das Ergebnis ueber var-Param.
278+
const SRC =
279+
'unit t; implementation'#13#10 +
280+
'function Parse(const S: string): Integer;'#13#10 +
281+
'begin'#13#10 +
282+
' TryStrToInt(S, Result);'#13#10 +
283+
'end;';
284+
var F: TObjectList<TLeakFinding>;
285+
begin
286+
F := TFindingHelper.FindingsOf(SRC);
287+
try Assert.AreEqual(0, TFindingHelper.Count(F, fkRoutineResultUnassigned));
288+
finally F.Free; end;
289+
end;
290+
291+
procedure TTestRoutineResultAssigned.CallPassesResultFollowedByOther_NoFinding;
292+
// Result als nicht-letztes Argument.
293+
const SRC =
294+
'unit t; implementation'#13#10 +
295+
'function Compute: Integer;'#13#10 +
296+
'begin'#13#10 +
297+
' DoWork(Result, ''logKey'', 42);'#13#10 +
298+
'end;';
299+
var F: TObjectList<TLeakFinding>;
300+
begin
301+
F := TFindingHelper.FindingsOf(SRC);
302+
try Assert.AreEqual(0, TFindingHelper.Count(F, fkRoutineResultUnassigned));
303+
finally F.Free; end;
304+
end;
305+
306+
procedure TTestRoutineResultAssigned.UnrelatedVarSimilarName_StillReported;
307+
// `ResultCache` ist NICHT `Result` - Word-Boundary muss verhindern, dass
308+
// das Finding faelschlich verschwindet.
309+
const SRC =
310+
'unit t; implementation'#13#10 +
311+
'function Foo: Integer;'#13#10 +
312+
'var ResultCache: Integer;'#13#10 +
313+
'begin'#13#10 +
314+
' DoWork(ResultCache);'#13#10 +
315+
'end;';
316+
var F: TObjectList<TLeakFinding>;
317+
begin
318+
F := TFindingHelper.FindingsOf(SRC);
319+
try Assert.AreEqual(1, TFindingHelper.Count(F, fkRoutineResultUnassigned));
320+
finally F.Free; end;
321+
end;
322+
255323
procedure TTestRoutineResultAssigned.Finding_KindAndSeverity;
256324
const SRC =
257325
'unit t; implementation'#13#10 +

0 commit comments

Comments
 (0)