diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b10ac330..f1a531f9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Support for identifiers prefixed with 2 ampersands (`&&`). - `OleVariant` overloads for `VarArrayRedim` and `VarClear` intrinsics. - `InlineAssembly` analysis rule, which flags inline assembly usage. +- `DigitSeparator` analysis rule, which flags numeric literals that should use digit separators to + improve readability. +- `DigitGrouping` analysis rule, which flags numeric literals that use non-standard digit groupings. ### Changed 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 955033509..1f4a067a2 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 @@ -50,6 +50,8 @@ public final class CheckList { DateFormatSettingsCheck.class, DestructorNameCheck.class, DestructorWithoutInheritedCheck.class, + DigitGroupingCheck.class, + DigitSeparatorCheck.class, EmptyArgumentListCheck.class, EmptyBlockCheck.class, EmptyFieldSectionCheck.class, diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitGroupingCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitGroupingCheck.java new file mode 100644 index 000000000..2daa2320e --- /dev/null +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitGroupingCheck.java @@ -0,0 +1,110 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2023 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.regex.Pattern; +import org.sonar.check.Rule; +import org.sonar.plugins.communitydelphi.api.ast.DecimalLiteralNode; +import org.sonar.plugins.communitydelphi.api.ast.IntegerLiteralNode; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheck; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext; +import org.sonar.plugins.communitydelphi.api.token.DelphiToken; + +@Rule(key = "DigitGrouping") +public class DigitGroupingCheck extends DelphiCheck { + + private static final Pattern VALID_DEC_UNDERSCORE_PATTERN = Pattern.compile("\\d{1,3}(_\\d{3})*"); + private static final Pattern VALID_DEC_FLOAT_UNDERSCORE_PATTERN = + Pattern.compile(VALID_DEC_UNDERSCORE_PATTERN.pattern() + "\\..*"); + + private static final Pattern VALID_HEX_UNDERSCORE_PATTERN = + Pattern.compile( + "(?i)\\$(" + + "([A-F0-9]{1,2}(_[A-F0-9]{2})*)" + + "|" + + "([A-F0-9]{1,4}(_[A-F0-9]{4})*)" + + ")"); + + private static final Pattern VALID_BIN_UNDERSCORE_PATTERN = + Pattern.compile( + "%(" + + "([01]{1,2}(_[01]{2})*)" + + "|" + + "([01]{1,3}(_[01]{3})*)" + + "|" + + "([01]{1,4}(_[01]{4})*)" + + ")"); + + public static final String MESSAGE = "Use standard digit groupings in this numeric literal."; + + @Override + public DelphiCheckContext visit(IntegerLiteralNode literal, DelphiCheckContext context) { + if (isCheckRelevant(literal.getToken()) && invalidIntegerLiteral(literal)) { + reportIssue(context, literal, MESSAGE); + } + + return super.visit(literal, context); + } + + @Override + public DelphiCheckContext visit(DecimalLiteralNode literal, DelphiCheckContext context) { + if (isCheckRelevant(literal.getToken()) && invalidFloat(literal)) { + reportIssue(context, literal, MESSAGE); + } + + return super.visit(literal, context); + } + + private boolean isCheckRelevant(DelphiToken token) { + return token.getImage().contains("_"); + } + + private static boolean invalidIntegerLiteral(IntegerLiteralNode integerLiteral) { + switch (integerLiteral.getRadix()) { + case 2: + return invalidBin(integerLiteral); + case 10: + return invalidDecimal(integerLiteral); + case 16: + return invalidHex(integerLiteral); + default: + return false; + } + } + + private static boolean invalidDecimal(IntegerLiteralNode literalNode) { + return notMatching(VALID_DEC_UNDERSCORE_PATTERN, literalNode.getToken()); + } + + private static boolean invalidFloat(DecimalLiteralNode literalNode) { + return notMatching(VALID_DEC_FLOAT_UNDERSCORE_PATTERN, literalNode.getToken()); + } + + private static boolean invalidHex(IntegerLiteralNode literalNode) { + return notMatching(VALID_HEX_UNDERSCORE_PATTERN, literalNode.getToken()); + } + + private static boolean invalidBin(IntegerLiteralNode literalNode) { + return notMatching(VALID_BIN_UNDERSCORE_PATTERN, literalNode.getToken()); + } + + private static boolean notMatching(Pattern pattern, DelphiToken token) { + return !pattern.matcher(token.getImage()).matches(); + } +} diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitSeparatorCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitSeparatorCheck.java new file mode 100644 index 000000000..4fdbfe3cc --- /dev/null +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/DigitSeparatorCheck.java @@ -0,0 +1,71 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2023 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 org.apache.commons.lang3.StringUtils; +import org.sonar.check.Rule; +import org.sonar.check.RuleProperty; +import org.sonar.plugins.communitydelphi.api.ast.DecimalLiteralNode; +import org.sonar.plugins.communitydelphi.api.ast.IntegerLiteralNode; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheck; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext; + +@Rule(key = "DigitSeparator") +public class DigitSeparatorCheck extends DelphiCheck { + + public static final String MESSAGE = "Separate this long numeric literal with underscores."; + + private static final int DEFAULT_MAX_DIGITS_WITHOUT_UNDERSCORES = 4; + + @RuleProperty( + key = "maxDigitsWithoutUnderscores", + description = "Maximum number of digits allowed without underscores.", + defaultValue = DEFAULT_MAX_DIGITS_WITHOUT_UNDERSCORES + "") + public int maxDigitsWithoutUnderscores = DEFAULT_MAX_DIGITS_WITHOUT_UNDERSCORES; + + @Override + public DelphiCheckContext visit(IntegerLiteralNode literal, DelphiCheckContext context) { + if (isIntMissingUnderscores(literal)) { + reportIssue(context, literal, MESSAGE); + } + + return super.visit(literal, context); + } + + private boolean isIntMissingUnderscores(IntegerLiteralNode literal) { + return literal.getImage().length() > maxDigitsWithoutUnderscores + && !StringUtils.contains(literal.getToken().getImage(), '_'); + } + + @Override + public DelphiCheckContext visit(DecimalLiteralNode literal, DelphiCheckContext context) { + if (isIntegerPartMissingUnderscores(literal.getToken().getImage())) { + reportIssue(context, literal, MESSAGE); + } + + return super.visit(literal, context); + } + + private boolean isIntegerPartMissingUnderscores(String image) { + String[] split = StringUtils.split(image, '.'); + return split.length >= 2 + && split[0].length() > maxDigitsWithoutUnderscores + && !StringUtils.contains(split[0], '_'); + } +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.html new file mode 100644 index 000000000..f0d2f3cb0 --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.html @@ -0,0 +1,54 @@ +

Why is this an issue?

+

From Delphi 11 onwards, digit separators _ can be used to split number literals into + groups.

+

+ To improve readability, this rule verifies that number literals using separators have groups of a + standard, homogeneous size. +

+ + +Noncompliant code example +
+  const
+    WrongGroupSize = 1_0000;     // Noncompliant; using groups of 4 instead of 3
+    MixedGroupSize = $F_7F_FFFF; // Noncompliant; using mixed group sizes
+
+ +Compliant code example +
+  const
+    DecimalInt       = 10_000;
+    HexadecimalInt2  = $7_FF_FF;
+    HexadecimalInt4  = $7_FFFF;
+    BinaryInt2       = %0_10_10;
+    BinaryInt3       = %01_010;
+    BinaryInt4       = %0_1010;
+    LongDecimalFloat = 10_000.0;
+    FloatWithAnySeps = 10.0_0_00_1;
+
+ +

Exceptions

+

+ Only the integer component of floating point numbers is checked for compliance; the fractional and + exponent parts are ignored. +

+ +

How to fix it

+
    +
  1. Pick an appropriate, standard digit group size from the above list.
  2. +
  3. Starting from the right, group the digits in the integer using _ between groups. +
  4. +
+ +

Resources

+ \ No newline at end of file diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.json new file mode 100644 index 000000000..53b2047da --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitGrouping.json @@ -0,0 +1,21 @@ +{ + "title": "Digit separators should be used with standard groupings", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "1min" + }, + "code": { + "attribute": "FORMATTED", + "impacts": { + "MAINTAINABILITY": "LOW" + } + }, + "tags": [ + "convention" + ], + "defaultSeverity": "Minor", + "scope": "ALL", + "quickfix": "unknown" +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.html new file mode 100644 index 000000000..a0064883c --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.html @@ -0,0 +1,47 @@ +

Why is this an issue?

+

From Delphi 11 onwards, digit separators _ can be used to split number literals into + groups.

+

+ To improve readability, this rule verifies that numeric literals above a configurable size + in digits (default: 4) use separators. +

+ +Noncompliant code example +
+  const
+    DecimalInt     = 10000;      // Noncompliant; more than 4 digits without separators
+    HexadecimalInt = $7FFFF;     // Noncompliant; more than 4 digits without separators
+    BinaryInt      = %01010;     // Noncompliant; more than 4 digits without separators
+    Float          = 10000.0;    // Noncompliant; more than 4 (integer) digits without separators
+
+ +Compliant code example +
+  const
+    DecimalInt       = 10_000;
+    HexadecimalInt   = $7_FFFF;
+    BinaryInt        = %0_10_10;
+    LongDecimalFloat = 10_000.0;
+    FloatWithAnySeps = 10.0_0_00_1;
+    FloatWithoutSeps = 10.00001;
+
+ +

Exceptions

+

+ Only the integer component of floating point numbers is checked for compliance; the fractional and + exponent parts are ignored. +

+ +

How to fix it

+
    +
  1. Group the digits in the integer using _ between groups.
  2. +
+ +

Resources

+ \ No newline at end of file diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.json new file mode 100644 index 000000000..51ec6f7ff --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/DigitSeparator.json @@ -0,0 +1,21 @@ +{ + "title": "Long number literals should be separated with underscores", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "1min" + }, + "code": { + "attribute": "FORMATTED", + "impacts": { + "MAINTAINABILITY": "LOW" + } + }, + "tags": [ + "convention" + ], + "defaultSeverity": "Minor", + "scope": "ALL", + "quickfix": "unknown" +} diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitGroupingCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitGroupingCheckTest.java new file mode 100644 index 000000000..7685887ba --- /dev/null +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitGroupingCheckTest.java @@ -0,0 +1,72 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2023 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.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class DigitGroupingCheckTest { + + @ParameterizedTest + @ValueSource( + strings = { + "1_0", + "1_.0e5", + "1_000000", + "100_000_0", + "100_000_", + "$ABC_DEF", + "$ABC_DE_F", + "%1_001_01", + }) + void testShouldAddIssue(String actual) { + CheckVerifier.newVerifier() + .withCheck(new DigitGroupingCheck()) + .onFile(new DelphiTestUnitBuilder().appendImpl("const A = " + actual + ";")) + .verifyIssueOnLine(7); + } + + @ParameterizedTest + @ValueSource( + strings = { + "1_000", + "100_000", + "1_000_000", + "1_000_000.0", + "1_000_000.0001", + "1_000_000.0_0_01", + "10_000.0e5", + "10_000.0e50001", + "$AB_CDEF", + "$ab_cdef", + "$AB_CD_EF", + "%1_0101", + "%1_01_01", + "%10_101", + "%10_1010", + }) + void testShouldNotAddIssue(String actual) { + CheckVerifier.newVerifier() + .withCheck(new DigitGroupingCheck()) + .onFile(new DelphiTestUnitBuilder().appendImpl("const A = " + actual + ";")) + .verifyNoIssues(); + } +} diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitSeparatorCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitSeparatorCheckTest.java new file mode 100644 index 000000000..dde3b9c00 --- /dev/null +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/DigitSeparatorCheckTest.java @@ -0,0 +1,86 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2023 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.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class DigitSeparatorCheckTest { + + @ParameterizedTest + @ValueSource( + strings = { + "100000", + "1000000", + "1000000.0", + "10000.0e5", + "10000.0_0", + "$ABCDEF", + "%101010", + }) + void testShouldAddIssueDefaultMaximum(String actual) { + CheckVerifier.newVerifier() + .withCheck(new DigitSeparatorCheck()) + .onFile(new DelphiTestUnitBuilder().appendImpl("const A = " + actual + ";")) + .verifyIssueOnLine(7); + } + + @ParameterizedTest + @ValueSource( + strings = { + "1000000000", + "$A000000000", + "%1000000000", + }) + void testShouldNotAddIssueGreaterMaximum(String actual) { + var check = new DigitSeparatorCheck(); + check.maxDigitsWithoutUnderscores = 10; + + CheckVerifier.newVerifier() + .withCheck(check) + .onFile(new DelphiTestUnitBuilder().appendImpl("const A = " + actual + ";")) + .verifyNoIssues(); + } + + @ParameterizedTest + @ValueSource( + strings = { + "1000", + "10_00", + "10_000", + "1_0000", + "1000.0", + "10000_.0", + "1.00001", + "1.0000_1", + "1.0e10001", + "$A000", + "$A_0000", + "%1000", + "%1_0000", + }) + void testShouldNotAddIssueDefaultMaximum(String actual) { + CheckVerifier.newVerifier() + .withCheck(new DigitSeparatorCheck()) + .onFile(new DelphiTestUnitBuilder().appendImpl("const").appendImpl(" A = " + actual + ";")) + .verifyNoIssues(); + } +} diff --git a/delphi-frontend/src/main/java/au/com/integradev/delphi/antlr/ast/node/DecimalLiteralNodeImpl.java b/delphi-frontend/src/main/java/au/com/integradev/delphi/antlr/ast/node/DecimalLiteralNodeImpl.java index 0666c512b..f381154e0 100644 --- a/delphi-frontend/src/main/java/au/com/integradev/delphi/antlr/ast/node/DecimalLiteralNodeImpl.java +++ b/delphi-frontend/src/main/java/au/com/integradev/delphi/antlr/ast/node/DecimalLiteralNodeImpl.java @@ -20,6 +20,7 @@ import au.com.integradev.delphi.antlr.ast.visitors.DelphiParserVisitor; import org.antlr.runtime.Token; +import org.apache.commons.lang3.StringUtils; import org.sonar.plugins.communitydelphi.api.ast.DecimalLiteralNode; import org.sonar.plugins.communitydelphi.api.type.IntrinsicType; import org.sonar.plugins.communitydelphi.api.type.Type; @@ -36,7 +37,7 @@ public T accept(DelphiParserVisitor visitor, T data) { @Override public double getValueAsDouble() { - return Double.parseDouble(getImage()); + return Double.parseDouble(StringUtils.remove(getImage(), '_')); } @Override