From b40cedf9065f898e1c67bf7c63f28c0cb83d8d15 Mon Sep 17 00:00:00 2001 From: "Denis.Zhdanov" Date: Mon, 2 Jul 2012 08:58:56 +0400 Subject: [PATCH] IDEA-87415 Formatter: cyclic alignment resolution 1. Skip cycled backward alignments instead of reporting an error; 2. Green code policy; --- .../psi/formatter/java/AbstractJavaBlock.java | 4 ++-- .../psi/formatter/java/LeafBlock.java | 5 +++-- .../psi/formatter/java/SimpleJavaBlock.java | 3 ++- .../AbstractBlockAlignmentProcessor.java | 19 ++-------------- .../intellij/formatting/AlignmentImpl.java | 4 ++-- .../formatting/BlockAlignmentProcessor.java | 3 +++ .../intellij/formatting/FormatProcessor.java | 22 +++++++++++++++++-- 7 files changed, 34 insertions(+), 26 deletions(-) diff --git a/java/java-impl/src/com/intellij/psi/formatter/java/AbstractJavaBlock.java b/java/java-impl/src/com/intellij/psi/formatter/java/AbstractJavaBlock.java index 232247dd5277..74de38e04af3 100644 --- a/java/java-impl/src/com/intellij/psi/formatter/java/AbstractJavaBlock.java +++ b/java/java-impl/src/com/intellij/psi/formatter/java/AbstractJavaBlock.java @@ -536,7 +536,7 @@ public abstract class AbstractJavaBlock extends AbstractBlock implements JavaBlo } else { AlignmentStrategy alignmentStrategyToUse = AlignmentStrategy.wrap(arrangeChildAlignment(child, alignmentStrategy)); - if (myAlignmentStrategy != null && myAlignmentStrategy.getAlignment(nodeType, childType) != null + if (myAlignmentStrategy.getAlignment(nodeType, childType) != null && (nodeType == JavaElementType.IMPLEMENTS_LIST || nodeType == JavaElementType.CLASS)) { alignmentStrategyToUse = myAlignmentStrategy; @@ -991,7 +991,7 @@ public abstract class AbstractJavaBlock extends AbstractBlock implements JavaBlo // The whole idea of variable declarations alignment is that complete declaration blocks which children are to be aligned hold // reference to the same AlignmentStrategy object, hence, reuse the same Alignment objects. So, there is no point in checking // if it's necessary to align sub-blocks if shared strategy is not defined. - if (myAlignmentStrategy == null || !mySettings.ALIGN_GROUP_FIELD_DECLARATIONS) { + if (!mySettings.ALIGN_GROUP_FIELD_DECLARATIONS) { return null; } diff --git a/java/java-impl/src/com/intellij/psi/formatter/java/LeafBlock.java b/java/java-impl/src/com/intellij/psi/formatter/java/LeafBlock.java index 660f3f7d1777..fddd8615b646 100644 --- a/java/java-impl/src/com/intellij/psi/formatter/java/LeafBlock.java +++ b/java/java-impl/src/com/intellij/psi/formatter/java/LeafBlock.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2009 JetBrains s.r.o. + * Copyright 2000-2012 JetBrains s.r.o. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -36,7 +36,8 @@ public class LeafBlock implements ASTBlock{ public LeafBlock(final ASTNode node, final Wrap wrap, final Alignment alignment, - Indent indent) { + Indent indent) + { myNode = node; myWrap = wrap; myAlignment = alignment; diff --git a/java/java-impl/src/com/intellij/psi/formatter/java/SimpleJavaBlock.java b/java/java-impl/src/com/intellij/psi/formatter/java/SimpleJavaBlock.java index 6ebf67bf0ff5..0a4c1054d9b3 100644 --- a/java/java-impl/src/com/intellij/psi/formatter/java/SimpleJavaBlock.java +++ b/java/java-impl/src/com/intellij/psi/formatter/java/SimpleJavaBlock.java @@ -68,7 +68,8 @@ public class SimpleJavaBlock extends AbstractJavaBlock { while (child != null) { if (!FormatterUtil.containsWhiteSpacesOnly(child) && child.getTextLength() > 0){ final ASTNode astNode = child; - AlignmentStrategy alignmentStrategyToUse = ALIGN_IN_COLUMNS_ELEMENT_TYPES.contains(myNode.getElementType()) ? myAlignmentStrategy + AlignmentStrategy alignmentStrategyToUse = ALIGN_IN_COLUMNS_ELEMENT_TYPES.contains(myNode.getElementType()) + ? myAlignmentStrategy : AlignmentStrategy.wrap(chooseAlignment(myReservedAlignment, myReservedAlignment2, child)); child = processChild(result, astNode, alignmentStrategyToUse, childWrap, indent, offset); if (astNode != child && child != null) { diff --git a/platform/lang-impl/src/com/intellij/formatting/AbstractBlockAlignmentProcessor.java b/platform/lang-impl/src/com/intellij/formatting/AbstractBlockAlignmentProcessor.java index d166573bdb97..4cea2ab53d74 100644 --- a/platform/lang-impl/src/com/intellij/formatting/AbstractBlockAlignmentProcessor.java +++ b/platform/lang-impl/src/com/intellij/formatting/AbstractBlockAlignmentProcessor.java @@ -84,25 +84,10 @@ public abstract class AbstractBlockAlignmentProcessor implements BlockAlignmentP // to block 'i3' and reformatting starts back after 'i1'. Now 'i2' is shifted to left as well in order to align to the // new 'i1' position. That changes 'i3' position as well that causes 'i1' to be shifted right one more time. // Hence, we have endless cycle here. We remember information about blocks that caused indentation change because of - // alignment of blocks located before them and post error every time we detect endless cycle. + // alignment of blocks located before them and skip alignment every time we detect an endless cycle. Set blocksCausedRealignment = context.backwardShiftedAlignedBlocks.get(offsetResponsibleBlock); if (blocksCausedRealignment != null && blocksCausedRealignment.contains(context.targetBlock)) { - StringBuilder messageBuilder = new StringBuilder(); - TextRange targetRange = context.targetBlock.getTextRange(); - messageBuilder.append( - String.format("Formatting error - code block %s is set to be shifted right because of its alignment with " - + "block %s more than once. I.e. moving the former block because of alignment algorithm causes " - + "subsequent block to be shifted right as well - cyclic dependency.", - offsetResponsibleBlock.getTextRange(), targetRange - )); - messageBuilder.append(context.targetBlock.getDebugInfo()); - messageBuilder.append("\nBlock content: '") - .append(context.document.getText().substring(targetRange.getStartOffset(), targetRange.getEndOffset())) - .append("'\n"); - messageBuilder.append("Note: document text is attached to this report."); - LogMessageEx.error(LOG, messageBuilder.toString(), context.document.getText()); - blocksCausedRealignment.add(context.targetBlock); - return Result.UNABLE_TO_ALIGN_BACKWARD_BLOCK; + return Result.RECURSION_DETECTED; } WhiteSpace previousWhiteSpace = offsetResponsibleBlock.getWhiteSpace(); diff --git a/platform/lang-impl/src/com/intellij/formatting/AlignmentImpl.java b/platform/lang-impl/src/com/intellij/formatting/AlignmentImpl.java index ae244c2cb394..a641c51fa2f2 100644 --- a/platform/lang-impl/src/com/intellij/formatting/AlignmentImpl.java +++ b/platform/lang-impl/src/com/intellij/formatting/AlignmentImpl.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2009 JetBrains s.r.o. + * Copyright 2000-2012 JetBrains s.r.o. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -79,7 +79,7 @@ class AlignmentImpl extends Alignment { * Try to find out result from those filtered blocks using the following algorithm: *
    *
  1. - * Use last block (block which has the greatest start offset) after the block which + * Use the last block (block which has the greatest start offset) after the block which * {@link AbstractBlockWrapper#getWhiteSpace() white space} contains line feeds; *
    2. *
  2. diff --git a/platform/lang-impl/src/com/intellij/formatting/BlockAlignmentProcessor.java b/platform/lang-impl/src/com/intellij/formatting/BlockAlignmentProcessor.java index c00bdd6da16c..c1debaab6909 100644 --- a/platform/lang-impl/src/com/intellij/formatting/BlockAlignmentProcessor.java +++ b/platform/lang-impl/src/com/intellij/formatting/BlockAlignmentProcessor.java @@ -45,6 +45,9 @@ public interface BlockAlignmentProcessor { /** Already processed block was realigned because of {@link AlignmentImpl#isAllowBackwardShift() backward alignment}. */ BACKWARD_BLOCK_ALIGNED, + /** Detected that backward alignment dependency graph is cycled. */ + RECURSION_DETECTED, + /** * It was necessary to align already processed block because of {@link AlignmentImpl#isAllowBackwardShift() backward alignment} * but that can't be done (e.g. that backward block {@link AbstractBlockWrapper#getWhiteSpace() white space} is read-only). diff --git a/platform/lang-impl/src/com/intellij/formatting/FormatProcessor.java b/platform/lang-impl/src/com/intellij/formatting/FormatProcessor.java index 246b4cfd8915..df86eacb3f0e 100644 --- a/platform/lang-impl/src/com/intellij/formatting/FormatProcessor.java +++ b/platform/lang-impl/src/com/intellij/formatting/FormatProcessor.java @@ -667,6 +667,8 @@ class FormatProcessor { myCurrentBlock = offsetResponsibleBlock.getNextBlock(); onCurrentLineChanged(); return false; + case RECURSION_DETECTED: + myCurrentBlock = offsetResponsibleBlock; // Fall through to the 'register alignment to skip'. case UNABLE_TO_ALIGN_BACKWARD_BLOCK: myAlignmentsToSkip.add(alignment); return false; @@ -1179,8 +1181,8 @@ class FormatProcessor { } private static int calcShift(final IndentInside lastLineIndent, final IndentInside whiteSpaceIndent, - final CommonCodeStyleSettings.IndentOptions options - ) { + final CommonCodeStyleSettings.IndentOptions options) + { if (lastLineIndent.equals(whiteSpaceIndent)) return 0; if (options.USE_TAB_CHARACTER) { if (lastLineIndent.whiteSpaces > 0) { @@ -1200,6 +1202,22 @@ class FormatProcessor { } } + /** + * Utility method to use during debugging formatter processing. + * + * @return text that contains intermediate formatter-introduced changes (even not committed yet) + */ + @SuppressWarnings("UnusedDeclaration") + @NotNull + private String getCurrentText() { + StringBuilder result = new StringBuilder(); + for (LeafBlockWrapper block = myFirstTokenBlock; block != null; block = block.getNextBlock()) { + result.append(block.getWhiteSpace().generateWhiteSpace(getIndentOptionsToUse(block, myDefaultIndentOption))); + result.append(myDocument.getCharsSequence().subSequence(block.getStartOffset(), block.getEndOffset())); + } + return result.toString(); + } + private abstract class State { private final FormattingStateId myStateId;