diff --git a/libs/natlint/src/main/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzer.java b/libs/natlint/src/main/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzer.java new file mode 100644 index 000000000..d2ceee415 --- /dev/null +++ b/libs/natlint/src/main/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzer.java @@ -0,0 +1,71 @@ +package org.amshove.natlint.analyzers; + +import org.amshove.natlint.api.AbstractAnalyzer; +import org.amshove.natlint.api.DiagnosticDescription; +import org.amshove.natlint.api.IAnalyzeContext; +import org.amshove.natlint.api.ILinterContext; +import org.amshove.natparse.DiagnosticSeverity; +import org.amshove.natparse.ReadOnlyList; +import org.amshove.natparse.natural.*; + +public class ConditionAlwaysFalseAnalyzer extends AbstractAnalyzer +{ + public static final DiagnosticDescription CONDITION_ALWAYS_FALSE = DiagnosticDescription.create( + "NL017", + "Condition is always false: %s", + DiagnosticSeverity.WARNING + ); + + @Override + public ReadOnlyList getDiagnosticDescriptions() + { + return ReadOnlyList.of(CONDITION_ALWAYS_FALSE); + } + + @Override + public void initialize(ILinterContext context) + { + context.registerNodeAnalyzer(IDecideOnBranchNode.class, this::analyzeDecideBranch); + } + + private void analyzeDecideBranch(ISyntaxNode node, IAnalyzeContext context) + { + var branch = (IDecideOnBranchNode) node; + var decide = ((IDecideOnNode) branch.parent()); + + if (!(decide.operand()instanceof IVariableReferenceNode varRef) + || !(varRef.reference()instanceof ITypedVariableNode typedTarget) + || typedTarget.type() == null) + { + return; + } + + for (var value : branch.values()) + { + if (!(value instanceof ILiteralNode literal)) + { + continue; + } + + checkLiteralType(context, typedTarget, literal); + } + } + + private static void checkLiteralType(IAnalyzeContext context, ITypedVariableNode typedTarget, ILiteralNode literal) + { + var inferredType = literal.inferType(typedTarget.type().format()); + if (!inferredType.fitsInto(typedTarget.type())) + { + context.report( + CONDITION_ALWAYS_FALSE.createFormattedDiagnostic( + literal.position(), "Inferred type %s does not fit into %s %s" + .formatted( + inferredType.toShortString(), + typedTarget.declaration().symbolName(), + typedTarget.type().toShortString() + ) + ) + ); + } + } +} diff --git a/libs/natlint/src/test/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzerShould.java b/libs/natlint/src/test/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzerShould.java new file mode 100644 index 000000000..aa49dc7e1 --- /dev/null +++ b/libs/natlint/src/test/java/org/amshove/natlint/analyzers/ConditionAlwaysFalseAnalyzerShould.java @@ -0,0 +1,52 @@ +package org.amshove.natlint.analyzers; + +import org.amshove.natlint.linter.AbstractAnalyzerTest; +import org.junit.jupiter.api.Test; + +class ConditionAlwaysFalseAnalyzerShould extends AbstractAnalyzerTest +{ + protected ConditionAlwaysFalseAnalyzerShould() + { + super(new ConditionAlwaysFalseAnalyzer()); + } + + @Test + void reportADecideConditionThatCanNeverBeMet() + { + testDiagnostics( + """ + DEFINE DATA LOCAL + 1 #AVAR (A1) + END-DEFINE + DECIDE ON FIRST VALUE OF #AVAR + VALUE 'Hello' + IGNORE + NONE + IGNORE + END-DECIDE + END + """, + expectDiagnostic(4, ConditionAlwaysFalseAnalyzer.CONDITION_ALWAYS_FALSE) + ); + } + + @Test + void notReportADecideConditionWhichFitsTrimmed() + { + testDiagnostics( + """ + DEFINE DATA LOCAL + 1 #AVAR (A1) + END-DEFINE + DECIDE ON FIRST VALUE OF #AVAR + VALUE 'H ' + IGNORE + NONE + IGNORE + END-DECIDE + END + """, + expectNoDiagnostic(4, ConditionAlwaysFalseAnalyzer.CONDITION_ALWAYS_FALSE) + ); + } +} diff --git a/libs/natparse/src/main/java/org/amshove/natparse/natural/IDataType.java b/libs/natparse/src/main/java/org/amshove/natparse/natural/IDataType.java index 1a6978fa8..70608b92b 100644 --- a/libs/natparse/src/main/java/org/amshove/natparse/natural/IDataType.java +++ b/libs/natparse/src/main/java/org/amshove/natparse/natural/IDataType.java @@ -16,14 +16,17 @@ public interface IDataType boolean hasDynamicLength(); /** - * Determines if this type fits into the given type. Implicit conversion is taken into account. + * Determines if this type fits into the given type. Implicit conversion is taken into account.
+ * This does not compare by byte size */ default boolean fitsInto(IDataType target) { - var bytesFit = this.byteSize() <= target.byteSize(); + var ourLength = this.hasDynamicLength() ? ONE_GIGABYTE : length(); + var theirLength = target.hasDynamicLength() ? ONE_GIGABYTE : target.length(); + var lengthFits = ourLength <= theirLength; var formatIsCompatible = hasCompatibleFormat(target); - return bytesFit && formatIsCompatible; + return lengthFits && formatIsCompatible; } /** @@ -120,7 +123,7 @@ private int calculateNumericSize() return Math.max(1, digitsBeforeDecimalPoint + digitsAfterDecimalPoint); } - private int calculatePackedSize() + default int calculatePackedSize() { return Math.max(1, (int) (Math.round((calculateNumericSize() + 1) / 2.0))); } diff --git a/libs/natparse/src/main/java/org/amshove/natparse/natural/ILiteralNode.java b/libs/natparse/src/main/java/org/amshove/natparse/natural/ILiteralNode.java index eb1bd968d..8ae77db93 100644 --- a/libs/natparse/src/main/java/org/amshove/natparse/natural/ILiteralNode.java +++ b/libs/natparse/src/main/java/org/amshove/natparse/natural/ILiteralNode.java @@ -2,5 +2,5 @@ public interface ILiteralNode extends ITokenNode, IOperandNode { - IDataType dataType(); + IDataType inferType(DataFormat targetType); } diff --git a/libs/natparse/src/main/java/org/amshove/natparse/parsing/AttributeNode.java b/libs/natparse/src/main/java/org/amshove/natparse/parsing/AttributeNode.java index ba5572b12..6cde73fc7 100644 --- a/libs/natparse/src/main/java/org/amshove/natparse/parsing/AttributeNode.java +++ b/libs/natparse/src/main/java/org/amshove/natparse/parsing/AttributeNode.java @@ -15,7 +15,7 @@ public AttributeNode(SyntaxToken token) } @Override - public IDataType dataType() + public IDataType inferType(DataFormat targetFormat) { return DATA_TYPE; } diff --git a/libs/natparse/src/main/java/org/amshove/natparse/parsing/LiteralNode.java b/libs/natparse/src/main/java/org/amshove/natparse/parsing/LiteralNode.java index 5f905051b..1358f46d8 100644 --- a/libs/natparse/src/main/java/org/amshove/natparse/parsing/LiteralNode.java +++ b/libs/natparse/src/main/java/org/amshove/natparse/parsing/LiteralNode.java @@ -7,16 +7,26 @@ class LiteralNode extends TokenNode implements ILiteralNode { - private final IDataType dataType; - public LiteralNode(SyntaxToken token) { super(token); + } - dataType = switch (token.kind()) + @Override + public IDataType inferType(DataFormat targetFormat) + { + var token = token(); + return switch (token.kind()) { - case STRING_LITERAL -> new LiteralType(DataFormat.ALPHANUMERIC, token.stringValue().length()); - case NUMBER_LITERAL -> new LiteralType(DataFormat.NUMERIC, getNumericLiteralLength(token.source())); + case STRING_LITERAL -> new LiteralType(DataFormat.ALPHANUMERIC, token.stringValue().trim().length()); + case NUMBER_LITERAL -> switch (targetFormat) + { + case BINARY -> new LiteralType(DataFormat.BINARY, getIntegerLiteralLength(token.source())); + case INTEGER -> new LiteralType(DataFormat.INTEGER, getIntegerLiteralLength(token.source())); + case FLOAT -> new LiteralType(DataFormat.FLOAT, getIntegerLiteralLength(token.source())); + case PACKED -> new LiteralType(DataFormat.PACKED, getNumericLiteralLength(token.source())); + default -> new LiteralType(DataFormat.NUMERIC, getNumericLiteralLength(token.source())); + }; case DATE_LITERAL -> new LiteralType(DataFormat.DATE, 4); case TIME_LITERAL, EXTENDED_TIME_LITERAL -> new LiteralType(DataFormat.TIME, 7); @@ -46,10 +56,32 @@ private double getNumericLiteralLength(String source) return Double.parseDouble("%s.%s".formatted(split[0].length(), split[1].length())); // there must be a smarter way } - @Override - public IDataType dataType() + private double getIntegerLiteralLength(String source) { - return dataType; + if (source.contains(".") || source.contains(",")) + { + return getNumericLiteralLength(source); + } + + var byteSize = Long.toBinaryString(Long.parseLong(source)).length() / 8.0; + if (byteSize < 1) + { + return 1; + } + + if (byteSize < 2) + { + return 2; + } + + if (byteSize > 4) + { + // for too big literals, round the bytes up. + // I5 isn't a valid type, but will result in the correct type error. + return (int) (byteSize + 1); + } + + return 4; } record LiteralType(DataFormat format, double length) implements IDataType diff --git a/libs/natparse/src/main/java/org/amshove/natparse/parsing/TypeChecker.java b/libs/natparse/src/main/java/org/amshove/natparse/parsing/TypeChecker.java index cb38b322f..950967e0f 100644 --- a/libs/natparse/src/main/java/org/amshove/natparse/parsing/TypeChecker.java +++ b/libs/natparse/src/main/java/org/amshove/natparse/parsing/TypeChecker.java @@ -90,7 +90,7 @@ private void checkSystemFunctionParameter(ISystemFunctionNode sysFuncNode) { for (var parameter : sysFuncNode.parameter()) { - var type = inferDataType(parameter); + var type = inferDataType(parameter, DataFormat.ALPHANUMERIC); if (type == null) { continue; @@ -112,7 +112,7 @@ private void checkSystemFunctionParameter(ISystemFunctionNode sysFuncNode) { var parameter = sysFuncNode.parameter().first(); - var type = inferDataType(parameter); + var type = inferDataType(parameter, DataFormat.ALPHANUMERIC); if (type != null && type.format() != DataFormat.NONE && type.format() != DataFormat.ALPHANUMERIC && type.format() != DataFormat.UNICODE && type.format() != DataFormat.BINARY) { report(ParserErrors.typeMismatch("Parameter to *TRIM must be of type A, B or U but is %s".formatted(type.toShortString()), parameter)); @@ -133,7 +133,7 @@ private void checkDecideOnBranches(IDecideOnNode decideOn) { for (var value : branch.values()) { - var inferredType = inferDataType(value); + var inferredType = inferDataType(value, typedTarget.type().format()); if (inferredType.format() != DataFormat.NONE && !inferredType.hasSameFamily(typedTarget.type())) { report( @@ -421,7 +421,7 @@ private boolean containsDynamicDimension(IRangedArrayAccessNode ranged) || upperLiteral.token().kind() == SyntaxKind.ASTERISK); } - private IDataType inferDataType(IOperandNode operand) + private IDataType inferDataType(IOperandNode operand, DataFormat targetFormat) { if (operand instanceof IVariableReferenceNode variable && variable.reference()instanceof ITypedVariableNode typedRef) { @@ -430,7 +430,7 @@ private IDataType inferDataType(IOperandNode operand) if (operand instanceof ILiteralNode literal) { - return literal.dataType(); + return literal.inferType(targetFormat); } if (operand instanceof ISystemFunctionNode sysFunction) diff --git a/libs/natparse/src/test/java/org/amshove/natparse/parsing/ConditionalParsingTests.java b/libs/natparse/src/test/java/org/amshove/natparse/parsing/ConditionalParsingTests.java index 67a3db83c..5fcf618c9 100644 --- a/libs/natparse/src/test/java/org/amshove/natparse/parsing/ConditionalParsingTests.java +++ b/libs/natparse/src/test/java/org/amshove/natparse/parsing/ConditionalParsingTests.java @@ -723,7 +723,7 @@ void parseConditionsWithDateLiterals() { var criteria = assertParsesCriteria("#VAR < D'1990-01-01'", IRelationalCriteriaNode.class); assertThat(assertNodeType(criteria.right(), ILiteralNode.class).token().kind()).isEqualTo(SyntaxKind.DATE_LITERAL); - assertThat(assertNodeType(criteria.right(), ILiteralNode.class).dataType().format()).isEqualTo(DataFormat.DATE); + assertThat(assertNodeType(criteria.right(), ILiteralNode.class).inferType(DataFormat.DATE).format()).isEqualTo(DataFormat.DATE); } @Test @@ -731,7 +731,7 @@ void parseConditionsWithTimeLiterals() { var criteria = assertParsesCriteria("#VAR < T'15:00:00'", IRelationalCriteriaNode.class); assertThat(assertNodeType(criteria.right(), ILiteralNode.class).token().kind()).isEqualTo(SyntaxKind.TIME_LITERAL); - assertThat(assertNodeType(criteria.right(), ILiteralNode.class).dataType().format()).isEqualTo(DataFormat.TIME); + assertThat(assertNodeType(criteria.right(), ILiteralNode.class).inferType(DataFormat.TIME).format()).isEqualTo(DataFormat.TIME); } @Test @@ -739,7 +739,7 @@ void parseConditionsWithExtendedTimeLiterals() { var criteria = assertParsesCriteria("#VAR < E'2010-02-02 15:00:00'", IRelationalCriteriaNode.class); assertThat(assertNodeType(criteria.right(), ILiteralNode.class).token().kind()).isEqualTo(SyntaxKind.EXTENDED_TIME_LITERAL); - assertThat(assertNodeType(criteria.right(), ILiteralNode.class).dataType().format()).isEqualTo(DataFormat.TIME); + assertThat(assertNodeType(criteria.right(), ILiteralNode.class).inferType(DataFormat.TIME).format()).isEqualTo(DataFormat.TIME); } @Test @@ -747,8 +747,8 @@ void parseConditionsWithHexLiterals() { var criteria = assertParsesCriteria("#VAR = H'0A'", IRelationalCriteriaNode.class); assertThat(assertNodeType(criteria.right(), ILiteralNode.class).token().kind()).isEqualTo(SyntaxKind.HEX_LITERAL); - assertThat(assertNodeType(criteria.right(), ILiteralNode.class).dataType().format()).isEqualTo(DataFormat.ALPHANUMERIC); - assertThat(assertNodeType(criteria.right(), ILiteralNode.class).dataType().length()).isEqualTo(1); + assertThat(assertNodeType(criteria.right(), ILiteralNode.class).inferType(DataFormat.ALPHANUMERIC).format()).isEqualTo(DataFormat.ALPHANUMERIC); + assertThat(assertNodeType(criteria.right(), ILiteralNode.class).inferType(DataFormat.ALPHANUMERIC).length()).isEqualTo(1); } @Test diff --git a/libs/natparse/src/test/java/org/amshove/natparse/parsing/LiteralTypeInferenceShould.java b/libs/natparse/src/test/java/org/amshove/natparse/parsing/LiteralTypeInferenceShould.java new file mode 100644 index 000000000..3d4ba517c --- /dev/null +++ b/libs/natparse/src/test/java/org/amshove/natparse/parsing/LiteralTypeInferenceShould.java @@ -0,0 +1,148 @@ +package org.amshove.natparse.parsing; + +import org.amshove.natparse.lexing.Lexer; +import org.amshove.natparse.lexing.SyntaxKind; +import org.amshove.natparse.natural.DataFormat; +import org.amshove.natparse.natural.DataType; +import org.amshove.natparse.natural.ILiteralNode; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +import java.nio.file.Path; + +import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; + +class LiteralTypeInferenceShould +{ + @ParameterizedTest + @CsvSource( + { + "-126,I1", + "127,I1", + "200,I2", + "12345,I2", + "2147483647,I4", + "2147483648,I4", + } + ) + void inferTheCorrectTypeBasedOnTargetTypeForIntegers(String source, String expectedInferredType) + { + assertInferredType("I4", source, expectedInferredType); + } + + @ParameterizedTest + @CsvSource( + { + "-126,B1", + "127,B1", + "200,B2", + "12345,B2", + "2147483647,B4", + "2147483648,B4", + } + ) + void inferTheCorrectTypeBasedOnTargetTypeForBinary(String source, String expectedInferredType) + { + assertInferredType("B4", source, expectedInferredType); + } + + @ParameterizedTest + @CsvSource( + { + "-126,F1", + "127,F1", + "200,F2", + "12345,F2", + "2147483647,F4", + "2147483648,F4", + } + ) + // TODO: What about actual decimal numbers? + void inferTheCorrectTypeBasedOnTargetTypeForFloats(String source, String expectedInferredType) + { + assertInferredType("F4", source, expectedInferredType); + } + + @ParameterizedTest + @CsvSource( + { + "-126,P3", + "127,P3", + "200,P3", + "12345,P5", + "2147483647,P10", + "2147483648,P10", + "2147483648.123,P10.3", + } + ) + void inferTheCorrectTypeBasedOnTargetTypeForPacked(String source, String expectedInferredType) + { + assertInferredType("P4", source, expectedInferredType); + } + + @ParameterizedTest + @ValueSource(strings = + { + "TRUE", "FALSE" + }) + void inferBooleans(String bool) + { + assertInferredType("L", bool, "L"); + } + + @ParameterizedTest + @CsvSource( + { + "12345,N5", + "2147483647,N10", + "2147483648,N10", + "2147483648.123,N10.3", + } + ) + void inferTheCorrectTypeBasedOnTargetTypeForNumerics(String source, String expectedInferredType) + { + assertInferredType("N1", source, expectedInferredType); + } + + private void assertInferredType(String targetType, String source, String expectedInferredType) + { + var typedTarget = createType(targetType); + var expectedType = createType(expectedInferredType); + + var literal = literal(source); + var inferredType = literal.inferType(typedTarget.format()); + + assertThat(inferredType.format()) + .as("Expected the inferred DataFormat to match") + .isEqualTo(expectedType.format()); + + if (typedTarget.length() > 0.0) + { + assertThat(inferredType.length()) + .as("Expected the inferred length to match") + .isEqualTo(expectedType.length()); + } + } + + private DataType createType(String typeSource) + { + var length = typeSource.length() > 1 + ? Double.parseDouble(typeSource.substring(1)) + : 0; + return new DataType(DataFormat.fromSource(typeSource.charAt(0)), length); + } + + private ILiteralNode literal(String source) + { + var lexer = new Lexer(); + var tokens = lexer.lex(source, Path.of("dummy")); + assertThat(tokens.diagnostics()).as("Expected to lex without diagnostics").isEmpty(); + var token = tokens.peek(); + if (token.kind() == SyntaxKind.MINUS) + { + token = tokens.peek(1); + } + return new LiteralNode(token); + } +} diff --git a/tools/ruletranslator/src/main/resources/rules/NL017 b/tools/ruletranslator/src/main/resources/rules/NL017 new file mode 100644 index 000000000..4987ca380 --- /dev/null +++ b/tools/ruletranslator/src/main/resources/rules/NL017 @@ -0,0 +1,6 @@ +name: Condition is always false +priority: MAJOR +tags: confusing, pitfall +type: CODE_SMELL +description: +This condition always evaluates to ``false``.