enhance ComparisonOperatorRule with option for preferred set (#273)

Author: jmgrassau

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/base/ABAP.java

@@ -658,7 +658,20 @@ public static String getTextualComparisonOperator(String text) {
 		}
 	}
 
-	public static boolean isComparisonOperator(String text) {
+	public static String getNonObsoleteComparisonOperator(String text) {
+		switch (text) {
+			case "=<":
+				return "<=";
+			case "=>":
+				return ">=";
+			case "><":
+				return "<>";
+			default:
+				return text;
+		}
+	}
+
+	public static boolean isComparisonOperator(String text, boolean includeObsoleteVariants) {
 		switch (AbapCult.toUpper(text)) {
 			// Comparison operators for all data types
 			case "<":
@@ -699,6 +712,11 @@ public static boolean isComparisonOperator(String text) {
 			case "BYTE-NS":
 				return true;
 
+			case "=<":
+			case "=>":
+			case "><":
+				return includeObsoleteVariants;
+				
 			// TODO: Comparison operators for bit patterns (O, Z, M) are missing,
 			// see https://help.sap.com/doc/abapdocu_latest_index_htm/latest/en-US/index.htm?file=abenlogexp_op.htm
 

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/parser/Command.java

@@ -3235,7 +3235,7 @@ public final boolean matchesPattern() {
 		//   return changesSyField(ABAP.SyField.SUBRC) && SyFieldAnalyzer.getSyFieldReadersFor(ABAP.SyField.SUBRC, this).size() >= 2;
 		//   - getCommandsRelatedToPatternMatch() can then return SyFieldAnalyzer.getSyFieldReadersFor(ABAP.SyField.SUBRC, this);
 
-		return false;
+      return false;
 	}
 
 	public final ArrayList<Command> getCommandsRelatedToPatternMatch() {

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/parser/Token.java

@@ -250,7 +250,7 @@ public final boolean isStringTemplate() {
 	/**
 	 * Returns true if the Token text equals the supplied comparison text.  
 	 * To check the Token type at the same time, use {@link #isKeyword(String)} or {@link #isComparisonOperator(String)} 
-	 * (see list of tokens classified as comparison operators in {@link ABAP#isComparisonOperator(String)}) 
+	 * (see list of tokens classified as comparison operators in {@link ABAP#isComparisonOperator(String, boolean)}) 
 	 */
 	public final boolean textEquals(String compareText) {
 		return AbapCult.stringEquals(text, compareText, true);
@@ -371,7 +371,7 @@ private static TokenType inferTypeFromAbapToken(String text) {
 			// "=" will later be differentiated between assignment and comparison operator in Command.finishBuild() / .distinguishOperators()
 			return TokenType.ASSIGNMENT_OP;
 
-		} else if (ABAP.isComparisonOperator(text)) {
+		} else if (ABAP.isComparisonOperator(text, true)) {
 			return TokenType.COMPARISON_OP;
 
 		} else if (ABAP.isAbapUpperCaseKeyword(text)) {
@@ -1260,7 +1260,7 @@ public final void ensureWhitespace() {
 	/**
 	 * Returns true if the Token text equals any of the supplied comparison texts.  
 	 * To check the Token type at the same time, use {@link #isAnyKeyword(String...)} or {@link #isAnyComparisonOperator(String...)} 
-	 * (see list of tokens classified as comparison operators in {@link ABAP#isComparisonOperator(String)}) 
+	 * (see list of tokens classified as comparison operators in {@link ABAP#isComparisonOperator(String, boolean)}) 
 	 */
 	public final boolean textEqualsAny(String... compareTexts) {
 		for (String compareText : compareTexts) {
@@ -1889,7 +1889,7 @@ else if (token.matchesOnSiblings(true, "INSTANCE", "OF")) {
 				throw new UnexpectedSyntaxException(token, "predicate expression expected");
 			}
 
-		} else if (ABAP.isComparisonOperator(token.getText())) { // includes BETWEEN and IN
+		} else if (ABAP.isComparisonOperator(token.getText(), true)) { // includes BETWEEN and IN
 			// Comparison Expressions (rel_exp), see https://ldcier1.wdf.sap.corp:44300/sap/public/bc/abap/docu?object=abenlogexp_comp
 			boolean isTernaryOp = token.isComparisonOperator("BETWEEN");
 			Term term2 = Term.createArithmetic(token.getNextCodeSibling());

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/syntax/ComparisonOperatorRule.java

@@ -12,6 +12,8 @@ public class ComparisonOperatorRule extends RuleForTokens {
 			new RuleReference(RuleSource.ABAP_CLEANER) };
 
 	private final static String[] textualComparisonOps = new String[] { "LT", "LE", "EQ", "NE", "GE", "GT" };
+	private final static String[] symbolicComparisonOps = new String[] { "<", "<=", "=", "<>", ">=", ">" };
+	private final static String[] obsoleteComparisonOps = new String[] { "><", "=<", "=>" };
 
 	@Override
 	public RuleID getID() { return RuleID.COMPARISON_OPERATOR; }
@@ -20,10 +22,10 @@ public class ComparisonOperatorRule extends RuleForTokens {
 	public RuleGroupID getGroupID() { return RuleGroupID.SYNTAX; }
 
 	@Override
-	public String getDisplayName() { return "Prefer =, <>, <= etc. to EQ, NE, LE etc."; }
+	public String getDisplayName() { return "Use consistent set of comparison operators"; }
 
 	@Override
-	public String getDescription() { return "Replaces keywords (LT, LE, EQ, NE, GT, GE) with symbolic comparison operators (<, <=, =, >=, >, <>)."; }
+	public String getDescription() { return "Replaces textual comparison operators (LT, LE, EQ, NE, GE, GT) with symbolic comparison operators (<, <=, =, <>, >=, >) or vice versa."; }
 
 	@Override
 	public LocalDate getDateCreated() { return LocalDate.of(2021, 1, 17); }
@@ -32,23 +34,41 @@ public class ComparisonOperatorRule extends RuleForTokens {
 	public RuleReference[] getReferences() { return references; }
 
 	@Override
-	public RuleID[] getDependentRules() { return new RuleID[] { RuleID.ALIGN_LOGICAL_EXPRESSIONS } ; }
+	public RuleID[] getDependentRules() { return new RuleID[] { RuleID.UPPER_AND_LOWER_CASE, RuleID.ALIGN_LOGICAL_EXPRESSIONS } ; }
 
 	@Override
    public String getExample() {
       return "" 
-			+ LINE_SEP + "  METHOD prefer_symbolic_comparison_ops." 
+      	+ LINE_SEP + "CLASS any_class IMPLEMENTATION." 
+			+ LINE_SEP + "  METHOD use_consistent_comparison_ops." 
 			+ LINE_SEP + "    IF a EQ b OR c NE d." 
-			+ LINE_SEP + "      IF a LT c AND b GT d" 
-			+ LINE_SEP + "                AND b LT e." 
+			+ LINE_SEP + "      IF a < c AND b > d" 
+			+ LINE_SEP + "               AND b < e." 
 			+ LINE_SEP + "        IF a LE d AND c GE b." 
-			+ LINE_SEP + "          \" do something" 
+			+ LINE_SEP + "          result = xsdbool( a <= d OR a GE b )." 
 			+ LINE_SEP + "        ENDIF." 
 			+ LINE_SEP + "      ENDIF." 
 			+ LINE_SEP + "    ENDIF." 
-			+ LINE_SEP + "  ENDMETHOD.";
+			+ LINE_SEP + "  ENDMETHOD."
+   		+ LINE_SEP + "ENDCLASS."
+			+ LINE_SEP 
+			+ LINE_SEP + "FORM any_form."
+			+ LINE_SEP + "  \" these obsolete variants are only possible outside of the object-oriented context:"
+			+ LINE_SEP + "  IF a >< b AND b => c OR c =< d."
+			+ LINE_SEP + "    RETURN."
+			+ LINE_SEP + "  ENDIF."
+			+ LINE_SEP + "ENDFORM."; 
    }
 
+	final ConfigEnumValue<ComparisonOperatorType> configPreferredOperatorSet = new ConfigEnumValue<ComparisonOperatorType>(this, "PreferredOperatorSet", "Preferred set of comparison operators:", new String[] { "symbolic (<, <=, =, <>, >=, >)", "textual (LT, LE, EQ, NE, GE, GT)" }, ComparisonOperatorType.values(), ComparisonOperatorType.SYMBOLIC, ComparisonOperatorType.SYMBOLIC, LocalDate.of(2024, 4, 12));
+	final ConfigBoolValue configReplaceRegularOperators = new ConfigBoolValue(this, "ReplaceRegularOperators", "Replace regular comparison operators with preferred variant", true);
+	final ConfigBoolValue configReplaceObsoleteOperators = new ConfigBoolValue(this, "ReplaceObsoleteOperators", "Replace obsolete comparison operators (><  =>  =<) with preferred variant", true, false, LocalDate.of(2024, 4, 12));
+
+	private final ConfigValue[] configValues = new ConfigValue[] { configPreferredOperatorSet, configReplaceRegularOperators, configReplaceObsoleteOperators };
+
+	@Override
+	public ConfigValue[] getConfigValues() { return configValues; }
+
 	public ComparisonOperatorRule(Profile profile) {
 		super(profile);
 		initializeConfiguration();
@@ -62,7 +82,7 @@ protected boolean skipCommand(Command command) {
 
 	@Override
 	protected boolean executeOn(Code code, Command command, Token token, int releaseRestriction) {
-		if (!token.isComparisonOperator() || !token.isAnyComparisonOperator(textualComparisonOps))
+		if (!token.isComparisonOperator())
 			return false;
 
 		// do NOT change 'opt' in: 'SELECT-OPTIONS selcrit FOR dobj DEFAULT val1 [TO val2] [OPTION opt] ....'
@@ -73,7 +93,24 @@ protected boolean executeOn(Code code, Command command, Token token, int release
 			}
 		}
 
-		String newOperator = ABAP.getSymbolicComparisonOperator(token.getText());
+		ComparisonOperatorType targetType = ComparisonOperatorType.forValue(configPreferredOperatorSet.getValue());
+		String newOperator;
+		if (configReplaceObsoleteOperators.getValue() && token.isAnyComparisonOperator(obsoleteComparisonOps) && !command.isInOOContext()) {
+			newOperator = ABAP.getNonObsoleteComparisonOperator(token.getText());
+			if (targetType == ComparisonOperatorType.TEXTUAL) {
+				newOperator = ABAP.getTextualComparisonOperator(newOperator);
+			}
+			
+		} else if (targetType == ComparisonOperatorType.SYMBOLIC && configReplaceRegularOperators.getValue() && token.isAnyComparisonOperator(textualComparisonOps)) {
+			newOperator = ABAP.getSymbolicComparisonOperator(token.getText());
+		
+		} else if (targetType == ComparisonOperatorType.TEXTUAL && configReplaceRegularOperators.getValue() && token.isAnyComparisonOperator(symbolicComparisonOps)) {
+			newOperator = ABAP.getTextualComparisonOperator(token.getText());
+		
+		} else {
+			return false;
+		}
+		
 		token.setText(newOperator, true);
 		return true;
 	}

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/syntax/ComparisonOperatorType.java

@@ -0,0 +1,12 @@
+package com.sap.adt.abapcleaner.rules.syntax;
+
+public enum ComparisonOperatorType {
+	SYMBOLIC,
+	TEXTUAL;
+	
+	public int getValue() { return this.ordinal(); }
+	
+	public static ComparisonOperatorType forValue(int value) {
+		return values()[value];
+	}
+}

test/com.sap.adt.abapcleaner.test/src/com/sap/adt/abapcleaner/base/AbapTest.java

@@ -415,16 +415,27 @@ void testReadTillEndOfTypeName() {
 
 	@Test
 	void testIsComparisonOperator() {
-		assertTrue(ABAP.isComparisonOperator("lt"));
-		assertTrue(ABAP.isComparisonOperator("LT"));
+		assertTrue(ABAP.isComparisonOperator("lt", false));
+		assertTrue(ABAP.isComparisonOperator("LT", false));
 
-		assertTrue(ABAP.isComparisonOperator("ca"));
-		assertTrue(ABAP.isComparisonOperator("CA"));
+		assertTrue(ABAP.isComparisonOperator("ca", false));
+		assertTrue(ABAP.isComparisonOperator("CA", false));
 
-		assertTrue(ABAP.isComparisonOperator("byte-CA"));
-		assertTrue(ABAP.isComparisonOperator("BYTE-CA"));
+		assertTrue(ABAP.isComparisonOperator("byte-CA", false));
+		assertTrue(ABAP.isComparisonOperator("BYTE-CA", false));
 	}
 
+	@Test
+	void testIsComparisonOperatorForObsoleteVariants() {
+		assertFalse(ABAP.isComparisonOperator("><", false));
+		assertFalse(ABAP.isComparisonOperator("=>", false));
+		assertFalse(ABAP.isComparisonOperator("=<", false));
+
+		assertTrue(ABAP.isComparisonOperator("><", true));
+		assertTrue(ABAP.isComparisonOperator("=>", true));
+		assertTrue(ABAP.isComparisonOperator("=<", true));
+}
+
 	@Test
 	void testParameterNull() {
 		// cover null-case branches that are never covered otherwise in tests

test/com.sap.adt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/syntax/ComparisonOperatorTest.java

@@ -1,40 +1,81 @@
 package com.sap.adt.abapcleaner.rules.syntax;
 
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import com.sap.adt.abapcleaner.rulebase.RuleID;
 import com.sap.adt.abapcleaner.rulebase.RuleTestBase;
 
 class ComparisonOperatorTest extends RuleTestBase {
+	private ComparisonOperatorRule rule;
+	
 	ComparisonOperatorTest() {
 		super(RuleID.COMPARISON_OPERATOR);
+		rule = (ComparisonOperatorRule)getRule();
+	}
+	
+	@BeforeEach
+	void setUp() {
+		// setup default test configuration (may be modified in the individual test methods)
+		rule.configPreferredOperatorSet.setEnumValue(ComparisonOperatorType.SYMBOLIC);
+		rule.configReplaceRegularOperators.setValue(true);
+		rule.configReplaceObsoleteOperators.setValue(true);
 	}
 
 	@Test
-	void testSimpleCases() {
+	void testPreferSymbolicSimpleCases() {
 		buildSrc("    IF a EQ b OR c NE d.");
-		buildSrc("      IF a LT c AND b GT d.");
+		buildSrc("      IF a < c AND b > d");
+		buildSrc("               AND b < e.");
 		buildSrc("        IF a LE d AND c GE b.");
-		buildSrc("          \" do something");
+		buildSrc("          result = xsdbool( a <= d OR a GE b ).");
 		buildSrc("        ENDIF.");
 		buildSrc("      ENDIF.");
 		buildSrc("    ENDIF.");
 
 		buildExp("    IF a = b OR c <> d.");
-		buildExp("      IF a < c AND b > d.");
+		buildExp("      IF a < c AND b > d");
+		buildExp("               AND b < e.");
 		buildExp("        IF a <= d AND c >= b.");
-		buildExp("          \" do something");
+		buildExp("          result = xsdbool( a <= d OR a >= b ).");
 		buildExp("        ENDIF.");
 		buildExp("      ENDIF.");
 		buildExp("    ENDIF.");
 
 		putAnyMethodAroundSrcAndExp();
-		
+
+		testRule();
+	}
+
+	@Test
+	void testPreferTextualSimpleCases() {
+		rule.configPreferredOperatorSet.setEnumValue(ComparisonOperatorType.TEXTUAL);
+
+		buildSrc("    IF a EQ b OR c NE d.");
+		buildSrc("      IF a < c AND b > d");
+		buildSrc("               AND b < e.");
+		buildSrc("        IF a LE d AND c GE b.");
+		buildSrc("          result = xsdbool( a <= d OR a GE b ).");
+		buildSrc("        ENDIF.");
+		buildSrc("      ENDIF.");
+		buildSrc("    ENDIF.");
+
+		buildExp("    IF a EQ b OR c NE d.");
+		buildExp("      IF a LT c AND b GT d");
+		buildExp("                AND b LT e.");
+		buildExp("        IF a LE d AND c GE b.");
+		buildExp("          result = xsdbool( a LE d OR a GE b ).");
+		buildExp("        ENDIF.");
+		buildExp("      ENDIF.");
+		buildExp("    ENDIF.");
+
+		putAnyMethodAroundSrcAndExp();
+
 		testRule();
 	}
 
 	@Test
-	void testMultiLine() {
+	void testPreferSymbolicMultiLine() {
 		// ensure that the indent of the second line is adjusted to the shortened = operator
 
 		buildSrc("    IF a EQ b OR a LT 0");
@@ -64,4 +105,77 @@ void testSelectOptionsOptionKept() {
 
 		testRule();
 	}
+
+
+	@Test
+	void testObsoleteToSymbolic() {
+		buildSrc("FORM any_form.");
+		buildSrc("  IF a >< b AND b => c OR c =< d.");
+		buildSrc("    RETURN.");
+		buildSrc("  ENDIF.");
+		buildSrc("ENDFORM.");
+
+		buildExp("FORM any_form.");
+		buildExp("  IF a <> b AND b >= c OR c <= d.");
+		buildExp("    RETURN.");
+		buildExp("  ENDIF.");
+		buildExp("ENDFORM.");
+
+		testRule();
+	}
+
+
+	@Test
+	void testObsoleteToTextual() {
+		rule.configPreferredOperatorSet.setEnumValue(ComparisonOperatorType.TEXTUAL);
+
+		buildSrc("FORM any_form.");
+		buildSrc("  IF a >< b AND b => c OR c =< d.");
+		buildSrc("    RETURN.");
+		buildSrc("  ENDIF.");
+		buildSrc("ENDFORM.");
+
+		buildExp("FORM any_form.");
+		buildExp("  IF a NE b AND b GE c OR c LE d.");
+		buildExp("    RETURN.");
+		buildExp("  ENDIF.");
+		buildExp("ENDFORM.");
+
+		testRule();
+	}
+
+	@Test
+	void testKeepRegularOperators() {
+		rule.configReplaceRegularOperators.setValue(false);
+
+		buildSrc("    IF a EQ b OR c NE d.");
+		buildSrc("      IF a < c AND b > d");
+		buildSrc("               AND b < e.");
+		buildSrc("        IF a LE d AND c GE b.");
+		buildSrc("          result = xsdbool( a <= d OR a GE b ).");
+		buildSrc("        ENDIF.");
+		buildSrc("      ENDIF.");
+		buildSrc("    ENDIF.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
+	@Test
+	void testKeepObsoleteOperators() {
+		rule.configReplaceObsoleteOperators.setValue(false);
+
+		buildSrc("FORM any_form.");
+		buildSrc("  IF a >< b AND b => c OR c =< d.");
+		buildSrc("    RETURN.");
+		buildSrc("  ENDIF.");
+		buildSrc("ENDFORM.");
+
+		copyExpFromSrc();
+
+		testRule();
+	}
 }