From b890b27c3f73d0a2a44865529650c0f0e3f5ee3f Mon Sep 17 00:00:00 2001 From: Elliot Hillary Date: Tue, 22 Apr 2025 15:46:37 +1000 Subject: [PATCH] Implement `ExhaustiveEnumCase` check --- CHANGELOG.md | 1 + .../integradev/delphi/checks/CheckList.java | 1 + .../checks/ExhaustiveEnumCaseCheck.java | 140 ++++++++++ .../community-delphi/ExhaustiveEnumCase.html | 82 ++++++ .../community-delphi/ExhaustiveEnumCase.json | 19 ++ .../checks/ExhaustiveEnumCaseCheckTest.java | 250 ++++++++++++++++++ 6 files changed, 493 insertions(+) create mode 100644 delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheck.java create mode 100644 delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.html create mode 100644 delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.json create mode 100644 delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheckTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index b3a61717f..49c2684b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `ExhaustiveEnumCase` analysis rule, which flags `case` statements that do not handle all values in an enumeration. - **API:** `EnumeratorOccurrence` type. - **API:** `EnumeratorOccurrence::getGetEnumerator` method. - **API:** `EnumeratorOccurrence::getMoveNext` method. diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java index fc99023ee..1d30e23c4 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java @@ -66,6 +66,7 @@ public final class CheckList { EmptyRoutineCheck.class, EmptyVisibilitySectionCheck.class, EnumNameCheck.class, + ExhaustiveEnumCaseCheck.class, ExplicitDefaultPropertyReferenceCheck.class, ExplicitTObjectInheritanceCheck.class, FieldNameCheck.class, diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheck.java new file mode 100644 index 000000000..59a164382 --- /dev/null +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheck.java @@ -0,0 +1,140 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2025 Integrated Application Development + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package au.com.integradev.delphi.checks; + +import java.util.List; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; +import org.sonar.check.Rule; +import org.sonar.plugins.communitydelphi.api.ast.CaseItemStatementNode; +import org.sonar.plugins.communitydelphi.api.ast.CaseStatementNode; +import org.sonar.plugins.communitydelphi.api.ast.DelphiNode; +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.check.FilePosition; +import org.sonar.plugins.communitydelphi.api.symbol.Invocable; +import org.sonar.plugins.communitydelphi.api.symbol.declaration.EnumElementNameDeclaration; +import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration; +import org.sonar.plugins.communitydelphi.api.symbol.declaration.TypedDeclaration; +import org.sonar.plugins.communitydelphi.api.type.Type; +import org.sonar.plugins.communitydelphi.api.type.Type.EnumType; + +@Rule(key = "ExhaustiveEnumCase") +public class ExhaustiveEnumCaseCheck extends DelphiCheck { + + @Override + public DelphiCheckContext visit(CaseStatementNode node, DelphiCheckContext context) { + if (node.getElseBlockNode() == null) { + EnumType enumType = getSelectorExpressionType(node); + if (enumType != null) { + Set enumElements = getEnumElements(enumType); + + List expressions = + node.getCaseItems().stream() + .map(CaseItemStatementNode::getExpressions) + .flatMap(List::stream) + .map(ExpressionNode::skipParentheses) + .collect(Collectors.toList()); + + if (expressions.stream().allMatch(PrimaryExpressionNode.class::isInstance)) { + expressions.stream() + .map(this::getElementNameDeclaration) + .filter(Objects::nonNull) + .forEach(enumElements::remove); + } else { + // If there are more complex expressions (e.g. subrange), with the information we have + // we can't determine if all elements are handled. + enumElements.clear(); + } + + if (!enumElements.isEmpty()) { + context + .newIssue() + .onFilePosition(FilePosition.from(node.getToken())) + .withMessage( + String.format( + "Make this case statement exhaustive (%d unhandled value%s)", + enumElements.size(), enumElements.size() == 1 ? "" : "s")) + .report(); + } + } + } + + return super.visit(node, context); + } + + private NameDeclaration unpackExpressionDeclaration(ExpressionNode expression) { + expression = expression.skipParentheses(); + if (!(expression instanceof PrimaryExpressionNode)) { + return null; + } + + DelphiNode maybeNameReference = expression.getChild(0); + if (!(maybeNameReference instanceof NameReferenceNode)) { + return null; + } + + NameReferenceNode nameReference = ((NameReferenceNode) maybeNameReference).getLastName(); + return nameReference.getNameDeclaration(); + } + + private EnumElementNameDeclaration getElementNameDeclaration(ExpressionNode expression) { + var declaration = unpackExpressionDeclaration(expression); + if (declaration instanceof EnumElementNameDeclaration) { + return (EnumElementNameDeclaration) declaration; + } else { + return null; + } + } + + private EnumType getSelectorExpressionType(CaseStatementNode node) { + var declaration = unpackExpressionDeclaration(node.getSelectorExpression()); + Type type; + + if (declaration instanceof Invocable) { + var invocable = (Invocable) declaration; + if (invocable.getRequiredParametersCount() != 0) { + return null; + } + + type = invocable.getReturnType(); + } else if (declaration instanceof TypedDeclaration) { + type = ((TypedDeclaration) declaration).getType(); + } else { + return null; + } + + if (type == null || !type.isEnum()) { + return null; + } + + return (EnumType) type; + } + + private Set getEnumElements(EnumType enumType) { + return enumType.typeScope().getAllDeclarations().stream() + .filter(EnumElementNameDeclaration.class::isInstance) + .map(EnumElementNameDeclaration.class::cast) + .collect(Collectors.toSet()); + } +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.html new file mode 100644 index 000000000..159135d2a --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.html @@ -0,0 +1,82 @@ +

Why is this an issue?

+

+ When using a case statement to alternate between different values of an enumeration, + all values should be handled. This could be done explicitly by including all values in the + case arms, or implicitly by adding a default branch. +

+

+ An exhaustive case statement makes it clear that all behaviour is intentionally + defined for all values, and guards against accidental omissions - for example, forgetting to + update the case statement when a new value is added to the enumeration. +

+

+ Note that this rule currently ignores any case statement with a subrange + expression due to analysis constraints. +

+

How to fix it

+

+ Add the missing enumeration values to the case: +

+
+type
+  TBeverageKind = (bvCold, bvFrozen, bvHot, bvRoomTemp);
+
+procedure PrepareBeverage(Kind: TBeverageKind);
+begin
+  case Kind of
+    bvCold, bvFrozen:
+      Refrigerate;
+    bvHot:
+      Microwave;
+  end;
+end;
+
+
+type
+  TBeverageKind = (bvCold, bvFrozen, bvHot, bvRoomTemp);
+
+procedure PrepareBeverage(Kind: TBeverageKind);
+begin
+  case Kind of
+    bvCold, bvFrozen:
+      Refrigerate;
+    bvHot:
+      Microwave;
+    bvRoomTemp:
+      // No action required
+  end;
+end;
+
+

+ Alternatively, add an else block to implicitly handle all remaining values: +

+
+type
+  TBeverageKind = (bvCold, bvFrozen, bvHot, bvRoomTemp);
+
+procedure PrepareBeverage(Kind: TBeverageKind);
+begin
+  case Kind of
+    bvCold, bvFrozen:
+      Refrigerate;
+    bvHot:
+      Microwave;
+  end;
+end;
+
+
+type
+  TBeverageKind = (bvCold, bvFrozen, bvHot, bvRoomTemp);
+
+procedure PrepareBeverage(Kind: TBeverageKind);
+begin
+  case Kind of
+    bvCold, bvFrozen:
+      Refrigerate;
+    bvHot:
+      Microwave;
+  else
+    // No action required
+  end;
+end;
+
diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.json new file mode 100644 index 000000000..945c951bd --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExhaustiveEnumCase.json @@ -0,0 +1,19 @@ +{ + "title": "Case statements should be exhaustive", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "3min" + }, + "code": { + "attribute": "COMPLETE", + "impacts": { + "MAINTAINABILITY": "MEDIUM" + } + }, + "tags": ["pitfall"], + "defaultSeverity": "Major", + "scope": "ALL", + "quickfix": "unknown" +} diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheckTest.java new file mode 100644 index 000000000..12ac12a90 --- /dev/null +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExhaustiveEnumCaseCheckTest.java @@ -0,0 +1,250 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2025 Integrated Application Development + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package au.com.integradev.delphi.checks; + +import au.com.integradev.delphi.builders.DelphiTestUnitBuilder; +import au.com.integradev.delphi.checks.verifier.CheckVerifier; +import org.junit.jupiter.api.Test; + +class ExhaustiveEnumCaseCheckTest { + @Test + void testNonEnumCaseShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyVal: Integer);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" 1: Writeln('one');") + .appendImpl(" 2: Writeln('two');") + .appendImpl(" 3: Writeln('three');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testExhaustiveEnumShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meThree);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" meTwo: Writeln('two');") + .appendImpl(" meThree: Writeln('three');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testExhaustiveEnumMultipleElementsPerArmShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meThree);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meOne, meTwo: Writeln('one');") + .appendImpl(" meThree: Writeln('three');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testNonEnumElementsShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meThree);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meOne, meTwo, 5: Writeln('one');") + .appendImpl(" meThree, Integer.MaxValue: Writeln('three');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testNonExhaustiveEnumShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meIgnored, meAlsoIgnored);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of // Noncompliant") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" meTwo: Writeln('two');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testExhaustiveSubrangeExpressionShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meFirst, meIncluded, meLast, meIgnored, meAlsoIgnored);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meFirst..meLast: Writeln('foo');") + .appendImpl(" meIgnored, meAlsoIgnored: Writeln('bar');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testNonExhaustiveSubrangeExpressionShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meFirst, meIncluded, meLast, meIgnored, meAlsoIgnored);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meFirst..meLast: Writeln('foo');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testFunctionCallSelectorExpressionShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meIgnored, meAlsoIgnored);") + .appendDecl(" TMyObject = class(TObject)") + .appendDecl(" function GetMyVal: TMyEnum;") + .appendDecl(" end;") + .appendImpl("procedure Test(MyObj: TMyObject);") + .appendImpl("begin") + .appendImpl(" case MyObj.GetMyVal of // Noncompliant") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" meTwo: Writeln('two');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testNotInvokedFunctionCallSelectorExpressionShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meIgnored, meAlsoIgnored);") + .appendDecl(" TMyObject = class(TObject)") + .appendDecl(" function GetMyVal(Arg: string): TMyEnum;") + .appendDecl(" end;") + .appendImpl("procedure Test(MyObj: TMyObject);") + .appendImpl("begin") + .appendImpl(" case MyObj.GetMyVal of") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" meTwo: Writeln('two');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testPropertySelectorExpressionShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meIgnored, meAlsoIgnored);") + .appendDecl(" TMyObject = class(TObject)") + .appendDecl(" FVal: TMyEnum;") + .appendDecl(" property MyVal: TMyEnum read FVal write FVal;") + .appendDecl(" end;") + .appendImpl("procedure Test(MyObj: TMyObject);") + .appendImpl("begin") + .appendImpl(" case MyObj.MyVal of // Noncompliant") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" meTwo: Writeln('two');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testNonExhaustiveEnumMultipleElementsPerArmShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meIgnored, meAlsoIgnored);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of // Noncompliant") + .appendImpl(" meOne, meTwo: Writeln('one or two');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testCaseWithDefaultShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExhaustiveEnumCaseCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("type") + .appendDecl(" TMyEnum = (meOne, meTwo, meThree);") + .appendImpl("procedure Test(MyVal: TMyEnum);") + .appendImpl("begin") + .appendImpl(" case MyVal of") + .appendImpl(" meOne: Writeln('one');") + .appendImpl(" else") + .appendImpl(" Writeln('anything else');") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } +}