fix LocalDeclarationOrderRule etc. for CONSTANTS used in TYPES (SAP#350)

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

@@ -954,7 +954,7 @@ public final void appendParenthesesUpTo(Token tokenAfterParentheses, boolean kee
 	 * @throws IntegrityBrokenException 
 	 */
 	public final Token insertLeftSibling(Token newToken) throws IntegrityBrokenException {
-		return insertLeftSibling(newToken, false, false);
+		return insertLeftSibling(newToken, false, null, false);
 	}
 
 	/**
@@ -965,7 +965,7 @@ public final Token insertLeftSibling(Token newToken) throws IntegrityBrokenExcep
 	 * @throws IntegrityBrokenException 
 	 */
 	public final Token insertLeftSibling(Token newToken, boolean moveFollowingLinesRight) throws IntegrityBrokenException {
-		return insertLeftSibling(newToken, moveFollowingLinesRight, false);
+		return insertLeftSibling(newToken, moveFollowingLinesRight, null, false);
 	}
 
 	/**
@@ -2115,16 +2115,18 @@ private MemoryAccessType determineMemoryAccessType() {
 		} 
 
 		// non-inline declarations
-		if (firstToken.isAnyKeyword(Command.declarationKeywordsReservingMemory)) {
-			if (prevToken.isAnyKeyword(Command.declarationKeywords) || prevToken.isComma()) {
+		if (firstToken.isAnyKeyword(Command.declarationKeywords)) {
+			if (prevToken.isAnyKeyword(Command.declarationKeywordsReservingMemory) 
+					|| firstToken.isAnyKeyword(Command.declarationKeywordsReservingMemory) && prevToken.isComma()) {
+				// except for TYPES, the declared identifier is in a write or assignment position
 				return ABAP.isFieldSymbol(text) ? MemoryAccessType.ASSIGN_TO_FS_OR_DREF : MemoryAccessType.WRITE;
 			} else if (prevToken.isIdentifier() && prevToken.getOpensLevel() && prevPrevToken != null && (prevPrevToken.isAnyKeyword(Command.declarationKeywords) || prevPrevToken.isComma() || prevPrevToken.isChainColon())) { 
 				// e.g. "lc_length" in "DATA lv_text(lc_length) VALUE 'abcde'."
 				return MemoryAccessType.READ;
 			} else if (prevToken.isAnyKeyword("LENGTH", "VALUE")) { // constants can be used for both
 				// e.g. "lc_length" and "lc_text" in "DATA lv_text TYPE c LENGTH lc_length VALUE lc_text."
 				return MemoryAccessType.READ;
-			} else { // e.g. a class name after TYPE REF TO
+			} else { // e.g. a class name after TYPE REF TO or a type name after TYPES
 				return MemoryAccessType.NONE;
 			}
 		}

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulebase/RuleForDeclarations.java

@@ -21,6 +21,26 @@ public abstract class RuleForDeclarations extends Rule {
    private final static String[] typesDeclarationKeywords = new String[] {"TYPES"};
    private final static String[] constantsDeclarationKeywords = new String[] {"CONSTANTS"};
 
+   private static class BlockInfo {
+   	private int blockLevel = 0;
+   	private VariableInfo topBlockVarInfo = null;
+
+   	private void beginBlock() {
+   		++blockLevel;
+   	}
+
+   	private void endBlock() {
+   		--blockLevel;
+   		if (blockLevel == 0) {
+   			topBlockVarInfo = null;
+   		}
+   	}
+
+   	private void setTopBlockVarInfo(VariableInfo topBlockVarInfo) {
+   		this.topBlockVarInfo = topBlockVarInfo;
+   	}
+	}
+
    protected static String getNameKey(String name) {
       return AbapCult.toUpper(name);
    }
@@ -50,7 +70,7 @@ public void executeOn(Code code, int releaseRestriction) throws UnexpectedSyntax
 		MethodVisibility methodVisibility = MethodVisibility.PUBLIC;
 		boolean isInMethod = false; 
 		boolean skipMethod = skipLocalVariableContexts();
-		int blockLevel = 0;
+		BlockInfo blockInfo = new BlockInfo();
 
 		while (command != null) {
 			// determine the current class or interface to add to its method definitions, 
@@ -155,7 +175,7 @@ public void executeOn(Code code, int releaseRestriction) throws UnexpectedSyntax
 					if (command.firstCodeTokenIsAnyKeyword(declarationKeywords)) {
 						boolean isTypeDeclaration = command.firstCodeTokenIsAnyKeyword(typesDeclarationKeywords);
 						boolean isConstantsDeclaration = command.firstCodeTokenIsAnyKeyword(constantsDeclarationKeywords);
-						blockLevel = executeOnDeclarationCommand(command, methodStart, localVariables, isTypeDeclaration, isConstantsDeclaration, blockLevel);
+						executeOnDeclarationCommand(command, methodStart, localVariables, isTypeDeclaration, isConstantsDeclaration, blockInfo);
 					} else if (command.isAsteriskCommentLine()) {
 						executeOnCommentLine(command, localVariables, commentIdentifier);
 					} else if (!command.isAbap()) {
@@ -329,28 +349,28 @@ else if (token.isKeyword("RETURNING"))
 		}
 	}
 
-	private int executeOnDeclarationCommand(Command command, Command methodStart, LocalVariables localVariables, boolean isTypeDeclaration, boolean isConstantsDeclaration, int blockLevel) throws UnexpectedSyntaxBeforeChanges {
+	private void executeOnDeclarationCommand(Command command, Command methodStart, LocalVariables localVariables, boolean isTypeDeclaration, boolean isConstantsDeclaration, BlockInfo blockInfo) throws UnexpectedSyntaxBeforeChanges {
 		Token token = command.getFirstCodeToken().getNextCodeToken();
 		boolean isChain = token.isChainColon();
 		if (isChain)
 			token = token.getNextCodeToken();
 
 		boolean isInOOContext = command.isInOOContext();
 		while (token != null) {
-			boolean skipLine = false;
 			boolean isBoundStructuredData = false;
+			boolean isTopBlockStart = false;
+			boolean isBlockEnd = false;
+
 			if (token.matchesOnSiblings(true, "END", "OF")) {
-				--blockLevel;
-				skipLine = true;
+				blockInfo.endBlock();
+				isBlockEnd = true;
 
 			} else if (token.matchesOnSiblings(true, "BEGIN", "OF")) {
-				++blockLevel;
+				blockInfo.beginBlock();
 
-				if (blockLevel > 1) {
-					// in case of nested BEGIN OF, do not enter names of inner structures  
-					skipLine = true;
-				} else {
+				if (blockInfo.blockLevel == 1) {
 					// move token to the name of the structure to add its declaration below
+					isTopBlockStart = true;
 					token = token.getNextCodeSibling();
 					token = token.getNextCodeSibling();
 					isBoundStructuredData = true;
@@ -359,14 +379,18 @@ private int executeOnDeclarationCommand(Command command, Command methodStart, Lo
 						token = token.getNextCodeSibling();
 					}
 				}
-			} else { 	
-				skipLine = (blockLevel > 0);
 			}
 
-			if (!skipLine) {
+			if (blockInfo.blockLevel > 0 && !isTopBlockStart) {
+				// declarations inside BEGIN OF ... blocks may contain usages of constants in LIKE ... or LENGTH ... clauses
+				token = executeOnDeclarationLine(methodStart, localVariables, token, isInOOContext, blockInfo.topBlockVarInfo);
+
+			} else if (!isBlockEnd) {
 				// add declaration
 				String varName = token.getText();
 				VariableInfo varInfo = localVariables.addDeclaration(token, false, isTypeDeclaration, isConstantsDeclaration, isBoundStructuredData, isInOOContext, false);
+				if (isTopBlockStart) 
+					blockInfo.setTopBlockVarInfo(varInfo);
 
 				// if the pragma ##NEEDED or the pseudo comment "#EC NEEDED is defined for this variable, add a "usage" 
 				// to prevent it from being commented out or deleted
@@ -379,22 +403,23 @@ private int executeOnDeclarationCommand(Command command, Command methodStart, Lo
 			}
 
 			if (!isChain || token == null) 
-				return blockLevel;
+				return;
 
 			// move to the next identifier
 			token = token.getLastTokenOnSiblings(true, TokenSearch.ASTERISK, ",|.");
 			if (token != null) {
 				token = token.getNextCodeToken();
 			}
 		}
-		return blockLevel;
+		return;
 	}
 
 	private Token executeOnDeclarationLine(Command methodStart, LocalVariables localVariables, Token token, boolean isInOOContext, VariableInfo varInfo) {
 		// length may be supplied in parentheses: "DATA lv_text(lc_length)."
-		if (token.getOpensLevel() && token.hasChildren() && token.getFirstChild().isAttached() && token.getFirstChild().isIdentifier()) {
-			String usedObjectName = LocalVariables.getObjectName(token.getFirstChild().getText(), isInOOContext);
-			localVariables.addUsageInLikeOrValueClause(token, usedObjectName, methodStart, varInfo);
+		Token firstChild = token.getFirstChild();
+		if (token.getOpensLevel() && firstChild != null && firstChild.isAttached() && firstChild.isIdentifier()) {
+			String usedObjectName = LocalVariables.getObjectName(firstChild.getText(), isInOOContext);
+			localVariables.addUsageInLikeOrValueClause(firstChild, usedObjectName, methodStart, varInfo);
 
 			// continue behind the parenthesis
 			token = token.getNextCodeSibling();

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rulehelpers/VariableInfo.java

@@ -86,6 +86,11 @@ public VariableInfo(ParameterInfo parameterInfo) {
       this.parameterInfo = parameterInfo;
    }
 
+   @Override
+   public String toString() { // for debugging
+   	return (declarationToken == null) ? "" : declarationToken.getText();
+   }
+
    public void setNeeded() {
    	isNeeded = true;
    }

com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/declarations/ImplicitTypeRule.java

@@ -277,6 +277,7 @@ private Token executeOn(Code code, Command command, Token identifier, int baseIn
 			} catch (UnexpectedSyntaxException e) {
 				throw new UnexpectedSyntaxAfterChanges(this, e);		
 			}
+			oldIdentifierWithLength.firstToken.setWhitespace(); // continue after the new identifier, otherwise .removeFromCommand will put a line break after oldIdentifierWithLength 
 			oldIdentifierWithLength.removeFromCommand(false);
 			identifier = newIdentifier; 
 

test/com.sap.adt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/ImplicitTypeTest.java

@@ -389,4 +389,21 @@ void testConstantLengthInParenthesis() {
 
 		testRule();
 	}
+
+	@Test
+	void testConstantLengthInParenthesisOfStructure() {
+		buildSrc("  CONSTANTS lc_length TYPE i VALUE 10.");
+		buildSrc("  TYPES: BEGIN OF ty_s_implicit,");
+		buildSrc("           b(lc_length),");
+		buildSrc("         END OF ty_s_implicit.");
+
+		buildExp("  CONSTANTS lc_length TYPE i VALUE 10.");
+		buildExp("  TYPES: BEGIN OF ty_s_implicit,");
+		buildExp("           b TYPE c LENGTH lc_length,");
+		buildExp("         END OF ty_s_implicit.");
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
 }

test/com.sap.adt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/LocalDeclarationOrderTest.java

@@ -1582,4 +1582,62 @@ void testMoveDataWithAsteriskCommentToInnerMostBlock() {
 
 		testRule();
 	}
+
+	@Test
+	void testTestContantsUsedInTypes() {
+		// ensure that only DATA is moved, while the three constant stay before TYPES, where they are used
+
+		buildSrc("  CONSTANTS lc_length1 TYPE i VALUE 10.");
+		buildSrc("  DATA lv_any TYPE i VALUE lc_length1.");
+		buildSrc("  CONSTANTS lc_length2 TYPE i VALUE 10.");
+		buildSrc("  CONSTANTS lc_string TYPE string VALUE `any`.");
+		buildSrc("");
+		buildSrc("  TYPES: BEGIN OF ty_s_implicit,");
+		buildSrc("           bb(lc_length1),");
+		buildSrc("           cc TYPE c LENGTH lc_length2,");
+		buildSrc("           dd LIKE lc_string,");
+		buildSrc("         END OF ty_s_implicit.");
+
+		buildExp("  CONSTANTS lc_length1 TYPE i VALUE 10.");
+		buildExp("  CONSTANTS lc_length2 TYPE i VALUE 10.");
+		buildExp("  CONSTANTS lc_string TYPE string VALUE `any`.");
+		buildExp("");
+		buildExp("  DATA lv_any TYPE i VALUE lc_length1.");
+		buildExp("");
+		buildExp("  TYPES: BEGIN OF ty_s_implicit,");
+		buildExp("           bb(lc_length1),");
+		buildExp("           cc TYPE c LENGTH lc_length2,");
+		buildExp("           dd LIKE lc_string,");
+		buildExp("         END OF ty_s_implicit.");
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
+	@Test
+	void testConstantsUsedInData() {
+		buildSrc("  CONSTANTS lc_length1 TYPE i VALUE 10.");
+		buildSrc("  DATA lv_text1(lc_length1) ##NEEDED.");
+		buildSrc("  CONSTANTS lc_other(lc_length1) VALUE 'abcde' ##NEEDED.");
+		buildSrc("  CONSTANTS lc_length2 TYPE i VALUE 10.");
+		buildSrc("  DATA lv_text2(lc_length2) ##NEEDED.");
+		buildSrc("  CONSTANTS lc_length3 TYPE i VALUE 10.");
+		buildSrc("  DATA lv_length3 TYPE i VALUE lc_length3 ##NEEDED.");
+		buildSrc("  CONSTANTS lc_length4 TYPE i VALUE 10.");
+
+		buildExp("  CONSTANTS lc_length1 TYPE i VALUE 10.");
+		buildExp("  CONSTANTS lc_other(lc_length1) VALUE 'abcde' ##NEEDED.");
+		buildExp("  CONSTANTS lc_length2 TYPE i VALUE 10.");
+		buildExp("  CONSTANTS lc_length3 TYPE i VALUE 10.");
+		buildExp("  CONSTANTS lc_length4 TYPE i VALUE 10.");
+		buildExp("");
+		buildExp("  DATA lv_text1(lc_length1) ##NEEDED.");
+		buildExp("  DATA lv_text2(lc_length2) ##NEEDED.");
+		buildExp("  DATA lv_length3 TYPE i VALUE lc_length3 ##NEEDED.");
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
 }

test/com.sap.adt.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/declarations/UnusedVariablesTest.java

@@ -2332,4 +2332,38 @@ void testIgnoreMessageInto() {
 
 		testRule();
 	}
+
+	@Test
+	void testConstantUsedInTypes() {
+		// ensure that the constant is NOT commented out
+
+		buildSrc("  CONSTANTS lc_length TYPE i VALUE 10.");
+		buildSrc("");
+		buildSrc("  TYPES: BEGIN OF ty_s_implicit,");
+		buildSrc("           b TYPE c LENGTH lc_length,");
+		buildSrc("         END OF ty_s_implicit.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
+
+	@Test
+	void testConstantUsedInTypesImplicitType() {
+		// ensure that the constant is NOT commented out
+
+		buildSrc("  CONSTANTS lc_length TYPE i VALUE 10.");
+		buildSrc("");
+		buildSrc("  TYPES: BEGIN OF ty_s_implicit,");
+		buildSrc("           b(lc_length),");
+		buildSrc("         END OF ty_s_implicit.");
+
+		copyExpFromSrc();
+
+		putAnyMethodAroundSrcAndExp();
+
+		testRule();
+	}
 }