fix UnusedVariablesRule and FinalVariableRule for macro usage (SAP#43)

jmgrassau · Jun 5, 2023 · dca6e5a · dca6e5a
1 parent ccf7945
commit dca6e5a
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 0 deletions.
diff --git a/com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulebase/RuleForDeclarations.java b/com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulebase/RuleForDeclarations.java
@@ -130,6 +130,12 @@ else if (!command.isAbap())
 					else
 						executeOnOtherCommand(command, localVariables);
 
+					// note down if macros are used in this method; in such a case, rules like the UnusedVariablesRule 
+					// must skip this method
+					if (command.usesMacro()) {
+						localVariables.setMethodUsesMacros();
+					}
+
 				} catch (UnexpectedSyntaxBeforeChanges ex) {
 					ex.addToLog();
 					skipMethod = true;

diff --git a/com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulehelpers/LocalVariables.java b/com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulehelpers/LocalVariables.java
@@ -33,6 +33,7 @@ public static String getObjectName(String identifier) {
 
    private Rule rule;
    private MethodInfo methodInfo;
+   private boolean methodUsesMacros;
 
 	public HashMap<String, VariableInfo> locals = new HashMap<String, VariableInfo>();
 
@@ -154,4 +155,12 @@ public VariableInfo addUsage(Token identifier, String name, boolean isAssignment
 		varInfo.addUsage(identifier, isAssignment, isUsageInSelfAssignment, isCommentedOut, writesToReferencedMemory);
 		return varInfo;
 	}
+
+	public void setMethodUsesMacros() {
+		methodUsesMacros = true;
+	}
+
+	public boolean getMethodUsesMacros() {
+		return methodUsesMacros;
+	}
 }
diff --git a/...sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/declarations/FinalVariableRule.java b/...sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/declarations/FinalVariableRule.java
@@ -99,6 +99,11 @@ protected void executeOn(Code code, ClassInfo classInfo, int releaseRestriction)
 
 	@Override
 	protected void executeOn(Code code, Command methodStart, LocalVariables localVariables, int releaseRestriction) throws UnexpectedSyntaxAfterChanges {
+		// skip this method if macros are used inside the method to avoid making variables FINAL that are used inside 
+		// the macros (note that macro code may be local or 'out of sight')
+		if (localVariables.getMethodUsesMacros())
+			return;
+
 		for (VariableInfo varInfo : localVariables.getLocalsInDeclarationOrder()) {
 			if (!varInfo.isDeclaredInline || varInfo.isAssignedAfterDeclaration())
 				continue;

diff --git a/...p.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/declarations/UnusedVariablesRule.java b/...p.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/declarations/UnusedVariablesRule.java
@@ -155,6 +155,11 @@ protected void executeOn(Code code, ClassInfo classInfo, int releaseRestriction)
 
 	@Override
 	protected void executeOn(Code code, Command methodStart, LocalVariables localVariables, int releaseRestriction) throws UnexpectedSyntaxAfterChanges, IntegrityBrokenException {
+		// skip this method if macros are used inside the method to avoid deletion of variables that are used inside 
+		// the macros (note that macro code may be local or 'out of sight')
+		if (localVariables.getMethodUsesMacros())
+			return;
+
 		for (VariableInfo varInfo : localVariables.getLocalsInDeclarationOrder()) {
 			Command command = varInfo.declarationToken.getParentCommand();
 			// .isBlocked must be considered only at this point, NOT earlier when usage information is gathered 

diff --git a/...dt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/FinalVariableTest.java b/...dt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/FinalVariableTest.java
@@ -479,4 +479,40 @@ void testDataUnchangedForPerformUsing() {
 
 		testRule();
 	}
+
+	@Test
+	void testDataUnchangedDueToMacroUsage() {
+		// expect seemingly unchanged variables to be kept as DATA, because they might be used in the macro
+
+		buildSrc("    DATA(lv_any) = 1.");
+		buildSrc("    DATA(lv_other) = 2.");
+		buildSrc("");
+		buildSrc("    any_macro.");
+		buildSrc("");
+		buildSrc("    rv_result = lv_any + lv_other.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
+	@Test
+	void testDataUnchangedDueToChainedMacroUsage() {
+		// expect seemingly unchanged variables to be kept as DATA, because they might be used in the macro
+
+		buildSrc("    DATA(lv_any) = 1.");
+		buildSrc("    DATA(lv_other) = 2.");
+		buildSrc("");
+		buildSrc("    other_macro: iv_any_param, iv_other_param.");
+		buildSrc("");
+		buildSrc("    rv_result = lv_any + lv_other.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
 }
diff --git a/....abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/UnusedVariablesTest.java b/....abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/UnusedVariablesTest.java
@@ -1717,4 +1717,42 @@ void testDefineSectionSkipped() {
 
 		testRule();
 	}
+
+	@Test
+	void testUnchangedDueToMacroUsage() {
+		// expect seemingly unused variables to be kept, because they might be used in the macro
+
+		buildSrc("    DATA: lv_unused         TYPE string,");
+		buildSrc("          lv_another_unused TYPE i.");
+		buildSrc("");
+		buildSrc("    FIELD-SYMBOLS: <ls_unused> TYPE ty_s_any_struc.");
+		buildSrc("");
+		buildSrc("    any_macro.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
+	@Test
+	void testUnchangedDueToChainedMacroUsage() {
+		// expect seemingly unused variables to be kept, because they might be used in the macro
+
+		buildSrc("    DATA: lv_unused         TYPE string,");
+		buildSrc("          lv_another_unused TYPE i.");
+		buildSrc("");
+		buildSrc("    FIELD-SYMBOLS: <ls_unused> TYPE ty_s_any_struc.");
+		buildSrc("");
+		buildSrc("    other_macro: 'any_macro_parameter',");
+		buildSrc("                 'other_macro_parameter'.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
 }