Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Library path (`DelphiLibraryPath`/`DelphiTranslatedLibraryPath`)
- Browsing path (`DelphiBrowsingPath`)
- Standard library
- Empty anonymous methods are now ignored in `EmptyBlock`.
- Empty anonymous methods are now flagged in `EmptyRoutine`.
- **API:** `AnonymousMethodNode::getStatementBlock` method.
- **API:** `AnonymousMethodNode::isEmpty` method.

## [1.12.2] - 2025-01-06

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package au.com.integradev.delphi.checks;

import org.sonar.check.Rule;
import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode;
import org.sonar.plugins.communitydelphi.api.ast.CaseItemStatementNode;
import org.sonar.plugins.communitydelphi.api.ast.CaseStatementNode;
import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode;
Expand Down Expand Up @@ -49,7 +50,7 @@ public DelphiCheckContext visit(CompoundStatementNode block, DelphiCheckContext
private static boolean shouldAddViolation(CompoundStatementNode block) {
DelphiNode parent = block.getParent();

if (parent instanceof RoutineBodyNode) {
if (parent instanceof RoutineBodyNode || parent instanceof AnonymousMethodNode) {
// Handled by EmptyRoutineRule
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@

import au.com.integradev.delphi.utils.InterfaceUtils;
import org.sonar.check.Rule;
import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode;
import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
import org.sonar.plugins.communitydelphi.api.ast.RoutineImplementationNode;
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
import org.sonar.plugins.communitydelphi.api.check.FilePosition;
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineDirective;
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration;
import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey;
Expand All @@ -36,15 +38,30 @@ public class EmptyRoutineCheck extends DelphiCheck {

@Override
public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckContext context) {
if (routine.isEmpty() && shouldAddViolation(routine)) {
if (shouldAddViolation(routine)) {
reportIssue(context, routine.getRoutineNameNode(), MESSAGE);
}
return super.visit(routine, context);
}

@Override
public DelphiCheckContext visit(AnonymousMethodNode anonymousMethod, DelphiCheckContext context) {
if (shouldAddViolation(anonymousMethod)) {
context
.newIssue()
.onFilePosition(FilePosition.from(anonymousMethod.getFirstToken()))
.withMessage(MESSAGE)
.report();
}
return super.visit(anonymousMethod, context);
}

private static boolean shouldAddViolation(RoutineImplementationNode routine) {
DelphiNode block = routine.getBlock();
if (!routine.isEmpty()) {
return false;
}

DelphiNode block = routine.getBlock();
if (block != null && block.getComments().isEmpty()) {
// All exclusions aside, an explanatory comment is mandatory
return true;
Expand All @@ -59,4 +76,8 @@ private static boolean shouldAddViolation(RoutineImplementationNode routine) {
&& !declaration.hasDirective(RoutineDirective.VIRTUAL)
&& !InterfaceUtils.implementsMethodOnInterface(declaration);
}

private static boolean shouldAddViolation(AnonymousMethodNode anonymousMethod) {
return anonymousMethod.isEmpty() && anonymousMethod.getStatementBlock().getComments().isEmpty();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ <h2>Why is this an issue?</h2>
<li>Virtual methods (e.g. to provide a no-op default behaviour for child classes)</li>
<li>Override methods (e.g. to not run behaviour implemented in ancestor classes)</li>
<li>Methods that implement an interface</li>
<li>Anonymous methods</li>
</ul>
<h2>How to fix it</h2>
<p>If the empty routine is an omission, remove it.</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,24 @@ void testEmptyRoutineShouldNotAddIssue() {
.appendImpl("end;"))
.verifyNoIssues();
}

@Test
void testEmptyAnonymousMethodShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new EmptyBlockCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TProc = reference to procedure;")
.appendImpl("procedure Foo;")
.appendImpl("var")
.appendImpl(" Bar: TProc;")
.appendImpl("begin")
.appendImpl(" Bar :=")
.appendImpl(" procedure")
.appendImpl(" begin")
.appendImpl(" end;")
.appendImpl("end;"))
.verifyNoIssues();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,47 @@ void testOverloadedMethodShouldNotAddIssue() {
.verifyNoIssues();
}

@Test
void testEmptyAnonymousMethodWithoutCommentShouldAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new EmptyRoutineCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TProc = reference to procedure;")
.appendImpl("procedure Foo;")
.appendImpl("var")
.appendImpl(" Bar: TProc;")
.appendImpl("begin")
.appendImpl(" Bar :=")
.appendImpl(" procedure // Noncompliant")
.appendImpl(" begin")
.appendImpl(" end;")
.appendImpl("end;"))
.verifyIssues();
}

@Test
void testEmptyAnonymousMethodWithCommentShouldNotAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new EmptyRoutineCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendDecl("type")
.appendDecl(" TProc = reference to procedure;")
.appendImpl("procedure Foo;")
.appendImpl("var")
.appendImpl(" Bar: TProc;")
.appendImpl("begin")
.appendImpl(" Bar :=")
.appendImpl(" procedure")
.appendImpl(" begin")
.appendImpl(" // do nothing")
.appendImpl(" end;")
.appendImpl("end;"))
.verifyNoIssues();
}

@Test
void testForwardDeclarationShouldNotAddIssue() {
CheckVerifier.newVerifier()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.antlr.runtime.Token;
import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodHeadingNode;
import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode;
import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode;
import org.sonar.plugins.communitydelphi.api.ast.RoutineParametersNode;
import org.sonar.plugins.communitydelphi.api.ast.RoutineReturnTypeNode;
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineDirective;
Expand Down Expand Up @@ -98,6 +99,16 @@ public boolean isProcedure() {
return getRoutineKind() == RoutineKind.PROCEDURE;
}

@Override
public CompoundStatementNode getStatementBlock() {
return (CompoundStatementNode) getChild(1);
}

@Override
public boolean isEmpty() {
return getStatementBlock().isEmpty();
}

@Override
public String getImage() {
if (image == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,8 @@ public interface AnonymousMethodNode extends ExpressionNode {
boolean isFunction();

boolean isProcedure();

CompoundStatementNode getStatementBlock();

boolean isEmpty();
}
Loading