diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f9526f79..13cbed101 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ 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`. +- Handle property references in `ObjectPassedAsInterface` (in addition to variables/fields). + ### 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..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 @@ -18,24 +18,50 @@ */ 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; 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; 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 { 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 +73,7 @@ public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContex ExpressionNode expression = arguments.get(i).getExpression(); - if (isVariableWithClassType(expression)) { + if (isObjectReferenceExpression(expression)) { reportIssue(context, expression, MESSAGE); } } @@ -55,24 +81,28 @@ 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)) { return false; } - var maybeName = expression.getChild(0); + var maybeName = Iterables.getLast(expression.getChildren()); if (!(maybeName instanceof NameReferenceNode)) { return false; } var declaration = ((NameReferenceNode) maybeName).getLastName().getNameDeclaration(); - if (!(declaration instanceof VariableNameDeclaration)) { + if (!(declaration instanceof VariableNameDeclaration + || declaration instanceof PropertyNameDeclaration)) { return false; } - return ((VariableNameDeclaration) declaration).getType().isClass(); + Type type = ((Typed) 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..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 @@ -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;") @@ -67,6 +67,56 @@ 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 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() @@ -105,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() @@ -114,7 +185,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 +207,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 +226,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 +241,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(); + } }