Skip to content

Improve accuracy of ObjectPassedAsInterface rule #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 7, 2025
Merged

Improve accuracy of ObjectPassedAsInterface rule #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2267770
Add `excludedTypes` property to `ObjectPassedAsInterface`
cirras Aug 7, 2025
ad2c7fa
Include property expressions in `ObjectPassedAsInterface`
cirras Aug 7, 2025
e3130db
Improve handling of complex expressions in `ObjectPassedAsInterface`
cirras Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 35 additions & 5 deletions ...hi-checks/src/main/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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);
Expand All @@ -47,32 +73,36 @@ public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContex

ExpressionNode expression = arguments.get(i).getExpression();

if (isVariableWithClassType(expression)) {
if (isObjectReferenceExpression(expression)) {
reportIssue(context, expression, MESSAGE);
}
}

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<Integer> getInterfaceParameterIndices(ArgumentListNode argumentList) {
Expand Down
121 changes: 116 additions & 5 deletions ...hecks/src/test/java/au/com/integradev/delphi/checks/ObjectPassedAsInterfaceCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;")
Expand All @@ -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;")
Expand All @@ -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<IBar>;")
.appendImpl("begin")
.appendImpl(" Intfs := [TBar.Create];")
.appendImpl(" DoThing(Intfs[0].Foo); // Noncompliant")
.appendImpl("end;"))
.verifyIssues();
}

@Test
void testInterfacePassedAsInterfaceShouldNotAddIssue() {
CheckVerifier.newVerifier()
Expand Down Expand Up @@ -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<TFoo>;")
.appendImpl("begin")
.appendImpl(" Foos := [TFoo.Create];")
.appendImpl(" DoThing(Foos[0].Intf);")
.appendImpl("end;"))
.verifyNoIssues();
}

@Test
void testObjectCastToInterfaceShouldNotAddIssue() {
CheckVerifier.newVerifier()
Expand All @@ -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;")
Expand All @@ -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;")
Expand All @@ -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)")
Expand All @@ -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();
}
}