From 22677702dccf8a12cdbbc17e54872f03aa7a83d1 Mon Sep 17 00:00:00 2001 From: Jonah Jeleniewski Date: Thu, 7 Aug 2025 10:37:27 +1000 Subject: [PATCH 1/3] Add `excludedTypes` property to `ObjectPassedAsInterface` By default, we'll exclude some common types that are known not to use the standard ref-counting implementation. --- CHANGELOG.md | 8 +++ .../checks/ObjectPassedAsInterfaceCheck.java | 32 ++++++++++-- .../ObjectPassedAsInterfaceCheckTest.java | 50 +++++++++++++++++-- 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f9526f79..a29c27c5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `excludedTypes` rule property to the `ObjectPassedAsInterface` rule. + +### Changed + +- Exclude common non-ref-counted interface implementations by default in `ObjectPassedAsInterface`. + ### Fixed - Inaccurate type comparisons between class and interface types, where class-to-class upcasts were diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java index 9e58ce547..b918eadd8 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java @@ -18,11 +18,14 @@ */ package au.com.integradev.delphi.checks; +import com.google.common.base.Splitter; import java.util.Collections; +import java.util.List; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.IntStream; import org.sonar.check.Rule; +import org.sonar.check.RuleProperty; import org.sonar.plugins.communitydelphi.api.ast.ArgumentListNode; import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode; import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode; @@ -31,11 +34,31 @@ import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext; import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration; import org.sonar.plugins.communitydelphi.api.symbol.declaration.VariableNameDeclaration; +import org.sonar.plugins.communitydelphi.api.type.Type; @Rule(key = "ObjectPassedAsInterface") public class ObjectPassedAsInterfaceCheck extends DelphiCheck { private static final String MESSAGE = "Do not pass this object reference as an interface."; + private static final String DEFAULT_EXCLUDED_TYPES = + "System.TNoRefCountObject," + + "System.Generics.Defaults.TSingletonImplementation," + + "System.Classes.TComponent"; + + @RuleProperty( + key = "excludedTypes", + description = + "Comma-delimited list of object types that this rule ignores. (case-insensitive)", + defaultValue = DEFAULT_EXCLUDED_TYPES) + public String excludedTypes = DEFAULT_EXCLUDED_TYPES; + + private List excludedTypesList; + + @Override + public void start(DelphiCheckContext context) { + excludedTypesList = Splitter.on(',').trimResults().splitToList(excludedTypes); + } + @Override public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContext context) { var interfaceIndices = getInterfaceParameterIndices(argumentList); @@ -47,7 +70,7 @@ public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContex ExpressionNode expression = arguments.get(i).getExpression(); - if (isVariableWithClassType(expression)) { + if (isObjectReferenceExpression(expression)) { reportIssue(context, expression, MESSAGE); } } @@ -55,7 +78,7 @@ public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContex return super.visit(argumentList, context); } - private static boolean isVariableWithClassType(ExpressionNode expression) { + private boolean isObjectReferenceExpression(ExpressionNode expression) { expression = expression.skipParentheses(); if (!(expression instanceof PrimaryExpressionNode)) { @@ -72,7 +95,10 @@ private static boolean isVariableWithClassType(ExpressionNode expression) { return false; } - return ((VariableNameDeclaration) declaration).getType().isClass(); + Type type = ((VariableNameDeclaration) declaration).getType(); + + return type.isClass() + && excludedTypesList.stream().noneMatch(e -> type.is(e) || type.isDescendantOf(e)); } private static Set getInterfaceParameterIndices(ArgumentListNode argumentList) { diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java index 812f25bb3..c94a868ec 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java @@ -32,7 +32,7 @@ void testObjectPassedAsObjectShouldNotAddIssue() { .appendDecl("type") .appendDecl(" IFooIntf = interface") .appendDecl(" end;") - .appendDecl(" TFooImpl = class(TObject, IFooIntf)") + .appendDecl(" TFooImpl = class(TInterfacedObject, IFooIntf)") .appendDecl(" end;") .appendDecl("procedure DoThing(Obj: TFooImpl);") .appendImpl("procedure Test;") @@ -54,7 +54,7 @@ void testObjectPassedAsInterfaceShouldAddIssue() { .appendDecl("type") .appendDecl(" IFooIntf = interface") .appendDecl(" end;") - .appendDecl(" TFooImpl = class(TObject, IFooIntf)") + .appendDecl(" TFooImpl = class(TInterfacedObject, IFooIntf)") .appendDecl(" end;") .appendDecl("procedure DoThing(Obj: IFooIntf);") .appendImpl("procedure Test;") @@ -114,7 +114,7 @@ void testObjectCastToInterfaceShouldNotAddIssue() { .appendDecl("type") .appendDecl(" IFooIntf = interface") .appendDecl(" end;") - .appendDecl(" TFooImpl = class(TObject, IFooIntf)") + .appendDecl(" TFooImpl = class(TInterfacedObject, IFooIntf)") .appendDecl(" end;") .appendDecl("procedure DoThing(Obj: IFooIntf);") .appendImpl("procedure Test;") @@ -136,7 +136,7 @@ void testNewObjectPassedAsInterfaceShouldNotAddIssue() { .appendDecl("type") .appendDecl(" IFooIntf = interface") .appendDecl(" end;") - .appendDecl(" TFooImpl = class(TObject, IFooIntf)") + .appendDecl(" TFooImpl = class(TInterfacedObject, IFooIntf)") .appendDecl(" end;") .appendDecl("procedure DoThing(Obj: IFooIntf);") .appendImpl("procedure Test;") @@ -155,7 +155,7 @@ void testObjectPassedAsInterfaceToInheritedShouldAddIssue() { .appendDecl("type") .appendDecl(" IFooIntf = interface") .appendDecl(" end;") - .appendDecl(" TFooParent = class(TObject)") + .appendDecl(" TFooParent = class(TInterfacedObject)") .appendDecl(" procedure Bar(Foo: IFooIntf); virtual;") .appendDecl(" end;") .appendDecl(" TFooImpl = class(TFooParent, IFooIntf)") @@ -170,4 +170,44 @@ void testObjectPassedAsInterfaceToInheritedShouldAddIssue() { .appendImpl("end;")) .verifyIssues(); } + + @Test + void testExcludedTypePassedAsInterfaceShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ObjectPassedAsInterfaceCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("uses") + .appendDecl(" System.Classes;") + .appendDecl("procedure DoThing(Obj: IInterface);") + .appendImpl("procedure Test(Obj: TComponent);") + .appendImpl("begin") + .appendImpl(" DoThing(Obj);") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testExcludedTypeDescendentPassedAsInterfaceShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ObjectPassedAsInterfaceCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("uses") + .appendDecl(" System.Classes;") + .appendDecl("type") + .appendDecl(" IFooIntf = interface") + .appendDecl(" end;") + .appendDecl(" TFooImpl = class(TComponent, IFooIntf)") + .appendDecl(" end;") + .appendDecl("procedure DoThing(Obj: IFooIntf);") + .appendImpl("procedure Test;") + .appendImpl("var") + .appendImpl(" Obj: TFooImpl;") + .appendImpl("begin") + .appendImpl(" Obj := TFooImpl.Create;") + .appendImpl(" DoThing(Obj);") + .appendImpl("end;")) + .verifyNoIssues(); + } } From ad2c7faada46c74aec22271e2664db0ef78b26a1 Mon Sep 17 00:00:00 2001 From: Jonah Jeleniewski Date: Thu, 7 Aug 2025 10:44:11 +1000 Subject: [PATCH 2/3] Include property expressions in `ObjectPassedAsInterface` --- CHANGELOG.md | 1 + .../checks/ObjectPassedAsInterfaceCheck.java | 7 ++++-- .../ObjectPassedAsInterfaceCheckTest.java | 25 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a29c27c5f..13cbed101 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Exclude common non-ref-counted interface implementations by default in `ObjectPassedAsInterface`. +- Handle property references in `ObjectPassedAsInterface` (in addition to variables/fields). ### Fixed diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java index b918eadd8..be517b28e 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java @@ -32,9 +32,11 @@ import org.sonar.plugins.communitydelphi.api.ast.PrimaryExpressionNode; import org.sonar.plugins.communitydelphi.api.check.DelphiCheck; import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext; +import org.sonar.plugins.communitydelphi.api.symbol.declaration.PropertyNameDeclaration; import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration; import org.sonar.plugins.communitydelphi.api.symbol.declaration.VariableNameDeclaration; import org.sonar.plugins.communitydelphi.api.type.Type; +import org.sonar.plugins.communitydelphi.api.type.Typed; @Rule(key = "ObjectPassedAsInterface") public class ObjectPassedAsInterfaceCheck extends DelphiCheck { @@ -91,11 +93,12 @@ private boolean isObjectReferenceExpression(ExpressionNode expression) { } var declaration = ((NameReferenceNode) maybeName).getLastName().getNameDeclaration(); - if (!(declaration instanceof VariableNameDeclaration)) { + if (!(declaration instanceof VariableNameDeclaration + || declaration instanceof PropertyNameDeclaration)) { return false; } - Type type = ((VariableNameDeclaration) declaration).getType(); + Type type = ((Typed) declaration).getType(); return type.isClass() && excludedTypesList.stream().noneMatch(e -> type.is(e) || type.isDescendantOf(e)); diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java index c94a868ec..b49c57ae3 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java @@ -67,6 +67,31 @@ void testObjectPassedAsInterfaceShouldAddIssue() { .verifyIssues(); } + @Test + void testQualifiedObjectPassedAsInterfaceShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ObjectPassedAsInterfaceCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TFoo = class(TInterfacedObject, IInterface)") + .appendDecl(" end;") + .appendDecl(" IBar = interface") + .appendDecl(" property Foo: TFoo;") + .appendDecl(" end;") + .appendDecl(" TBar = class(TInterfacedObject, IBar)") + .appendDecl(" end;") + .appendDecl("procedure DoThing(Obj: IInterface);") + .appendImpl("procedure Test;") + .appendImpl("var") + .appendImpl(" Intf: IBar;") + .appendImpl("begin") + .appendImpl(" Intf := TBar.Create;") + .appendImpl(" DoThing(Intf.Foo); // Noncompliant") + .appendImpl("end;")) + .verifyIssues(); + } + @Test void testInterfacePassedAsInterfaceShouldNotAddIssue() { CheckVerifier.newVerifier() From e3130db772bfbe322887348443617129bc09c33f Mon Sep 17 00:00:00 2001 From: Jonah Jeleniewski Date: Thu, 7 Aug 2025 11:09:04 +1000 Subject: [PATCH 3/3] Improve handling of complex expressions in `ObjectPassedAsInterface` --- .../checks/ObjectPassedAsInterfaceCheck.java | 3 +- .../ObjectPassedAsInterfaceCheckTest.java | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java index be517b28e..f1ca7b8f2 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java @@ -19,6 +19,7 @@ package au.com.integradev.delphi.checks; import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; import java.util.Collections; import java.util.List; import java.util.Set; @@ -87,7 +88,7 @@ private boolean isObjectReferenceExpression(ExpressionNode expression) { return false; } - var maybeName = expression.getChild(0); + var maybeName = Iterables.getLast(expression.getChildren()); if (!(maybeName instanceof NameReferenceNode)) { return false; } diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java index b49c57ae3..2de3f13db 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java @@ -92,6 +92,31 @@ void testQualifiedObjectPassedAsInterfaceShouldAddIssue() { .verifyIssues(); } + @Test + void testComplexQualifiedObjectPassedAsInterfaceShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ObjectPassedAsInterfaceCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TFoo = class(TInterfacedObject, IInterface)") + .appendDecl(" end;") + .appendDecl(" IBar = interface") + .appendDecl(" property Foo: TFoo;") + .appendDecl(" end;") + .appendDecl(" TBar = class(TInterfacedObject, IBar)") + .appendDecl(" end;") + .appendDecl("procedure DoThing(Obj: IInterface);") + .appendImpl("procedure Test;") + .appendImpl("var") + .appendImpl(" Intfs: TArray;") + .appendImpl("begin") + .appendImpl(" Intfs := [TBar.Create];") + .appendImpl(" DoThing(Intfs[0].Foo); // Noncompliant") + .appendImpl("end;")) + .verifyIssues(); + } + @Test void testInterfacePassedAsInterfaceShouldNotAddIssue() { CheckVerifier.newVerifier() @@ -130,6 +155,27 @@ void testQualifiedInterfacePassedAsInterfaceShouldNotAddIssue() { .verifyNoIssues(); } + @Test + void testComplexQualifiedInterfacePassedAsInterfaceShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ObjectPassedAsInterfaceCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TFoo = class") + .appendDecl(" property Intf: IInterface;") + .appendDecl(" end;") + .appendDecl("procedure DoThing(Intf: IInterface);") + .appendImpl("procedure Test;") + .appendImpl("var") + .appendImpl(" Foos: TArray;") + .appendImpl("begin") + .appendImpl(" Foos := [TFoo.Create];") + .appendImpl(" DoThing(Foos[0].Intf);") + .appendImpl("end;")) + .verifyNoIssues(); + } + @Test void testObjectCastToInterfaceShouldNotAddIssue() { CheckVerifier.newVerifier()