Indentation: Unexpected violation when class declaration is wrapped after public and before static

[kalpadiptyaroy]

Detected at #14149 (comment)

rivanov@p5510:/var/tmp$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="Indentation">
      <property name="tabWidth" value="4"/>
      <property name="basicOffset" value="2"/>
      <property name="braceAdjustment" value="2"/>
      <property name="caseIndent" value="2"/>
      <property name="throwsIndent" value="4"/>
      <property name="lineWrappingIndentation" value="4"/>
      <property name="arrayInitIndent" value="2"/>
    </module>
  </module>
</module>

rivanov@p5510:/var/tmp$ cat Test.java 
public class Test {

  public
        class C2{}

  public
        static class C3{} // violation, 'expected level should be 2.'

}

rivanov@p5510:/var/tmp$ java -jar checkstyle-10.12.4-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /var/tmp/Test.java:7:9: 'class def modifier' has incorrect indentation level 8
   , expected level should be 2. [Indentation]
Audit done.
Checkstyle ends with 1 errors.

Expected: no violations

---

Sample Input:

public class Test {

  public class C1{}

  public
  class C2{}  // violation, expected 6

  public
  static class C3{} // violation, expected 6

  public
      static class C4{}  // this is a violation too, can't make check happy, expects 2

  public
  static // violation, expected 6
  strictfp class C5{} // violation, expected 6

  public
      static // violation, can't make check happy, expects 2
      strictfp class C6{}  // violation, can't make check happy, expects 2

  public
  @X // violation, expected 6
  static class C7{}  // violation, expected 6

  public
  @X // violation, expected 6
  @Y // violation, expected 6
  static class C8{}  // violation, expected 6

  public
      @X // violation, can't make check happy, expects 2
      @Y // violation, can't make check happy, expects 2
      static class C9{}  // violation, can't make check happy, expects 2

  @interface X{}
  @interface Y{}
  @interface Z{}
}

Sample Config:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="tabWidth" value="4"/>
  <property name="charset" value="UTF-8"/>
  <property name="severity" value="warning"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="Indentation">
      <property name="tabWidth" value="4"/>
      <property name="basicOffset" value="2"/>
      <property name="braceAdjustment" value="2"/>
      <property name="caseIndent" value="2"/>
      <property name="throwsIndent" value="4"/>
      <property name="lineWrappingIndentation" value="4"/>
      <property name="arrayInitIndent" value="2"/>
    </module>
  </module>
</module>

Output:

PS D:\opensource\project-checkstyle\checkstyle\target> java -jar .\checkstyle-10.14.1-SNAPSHOT-all.jar -c config.xml Test.java
Starting audit...
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:6:3: 'class' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:9:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:12:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:15:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:16:3: 'strictfp' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:19:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:20:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:23:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:24:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:27:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:28:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:29:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:32:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:33:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:34:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
Audit done.

Possibly Desired/Expected Output: (need to be clarified during imlementaion)

PS D:\opensource\project-checkstyle\checkstyle\target> java -jar .\checkstyle-10.14.1-SNAPSHOT-all.jar -c config.xml Test.java
Starting audit...
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:6:3: 'class' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:9:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:15:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:16:3: 'strictfp' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:23:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:24:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:27:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:28:3: '@' has incorrect indentation level 2, expected level should be 6. [Indentation]
[WARN] D:\opensource\project-checkstyle\checkstyle\target\Test.java:29:3: 'static' has incorrect indentation level 2, expected level should be 6. [Indentation]
Audit done.

Observation: During IndentationCheck, any Class Definition is first scanned by ClassDefHandler, where it does a primitive indentation check. Post that, LineWrappingHandler gets the responsibility for further indentation check.
Now, Line 12 is a case where LineWrappingHandler's decision should prevail, whereas in present case ClassDefHandler's decision is prevailing.

Intuition: If we encounter a Token like ANNOTATION / STATIC / STRICTR within a class def., then ClassDefHandler should not decide on its indentation if this token is preceded by modifiers like PUBLIC / ABSTRACT/ FINAL, so that LineWrappingHandler gets the liberty to do so.

[rnveach]

Expected output says to remove line 12, which is

  public
      static class C4{}  // this is a violation too, can't make check happy, expects 2

I disagree with this issue, specifically the words should exclude. Every line that is code should be violated for indentation. It doesn't make sense to exclude lines completely. We should fix any issues so violations can be resolved.

[kalpadiptyaroy]

@rnveach Updated the title and also added explanation. If there is still a disagreement.

Let's consult with @nrmancuso, and know his opinion as well.

[nrmancuso]

Few things here:

1. Release notes are generated from issue descriptions.
2. Imagine you are a user. You upgrade your checkstyle version and discover that you have a bunch of new violations; you visit the release page and search for some explanation of why these new violations exist. I doubt that the average user knows what a ClassDefHandler or LineWrappingHandler is; I don't know what these classes do and I have been maintaining this project for 3 years now :)
3. User-facing issues (like this one) should typically be defined in terms of expected behavior; I would expect a title like "Indentation Check incorrectly handles modifiers on class definitions" or something similar. Implementation details are handled in PRs usually.
4. Why are some modifiers treated differently than others? I would expect all modifiers to be treated the same, perhaps with the exception of annotations. Please extend your examples to show which modifiers are affected and which aren't.

[romani]

I fixed issue title and description. Top levle problem is approved. All details on clases and all types of modifies and annotations we will discuss in pull request.

[nrmancuso]

Hm, still looks like we don't understand the problem here. Before any work starts on this issue, we should clearly understand the problem, and make sure we are on the same page about the expected behavior of this check. It looks like indentation check cannot properly handle any wrapped modifiers in a class definition at all (or maybe only one??), so the current examples and issue description really don't demonstrate the "high level problem" or give me confidence that we understand and agree on the expected check behavior.

[kalpadiptyaroy]

@nrmancuso - On a high level - as per my view: the problem is, we cannot treat the modifiers annotation, static, strictfp in the same way as we do for public, final and abstract, when they are appearing in a class def and they are wrapped before class keyword.

2nd perspective: Our code has 2 levels of indentation check.

• 1st Level is getting done by ClassDefHandler or equivalent in other cases.
• 2nd Level is getting done by LineWrappingHandler.

Now, I am not sure why this kind of hierarchy was actually made in the code? - in my opinion there should have been only one handler for certifying the indentation of any line.

Anyways - I am raising a PR. Feel free to suggest additional inputs for the test case file.

[nrmancuso]

we cannot treat the modifiers annotation, static, strictfp in the same way as we do for public, final and abstract

Why? Why should some modifiers be indented differently than others?

Anyways - I am raising a PR

I am not interested in reviewing PRs until it is clear what the actual problem we are trying to solve is.

1st Level is getting done by ClassDefHandler or equivalent in other cases.

None of this is important to making the problem with indentation checks behavior clear.

[kalpadiptyaroy]

Can't treat them equally when they are wrapped. Since doing so will enforce wrong indentations to them when they are wrapped in class def.
This is in context to the current code workflow!

If @X is wrapped between public & class then we should let LineWrappingHandler check its indentation.

If it occurs at start of line then indentation check by ClassDefHandler is correct!

Hence unequal treatment depending on whether wrapped or not!

[nrmancuso]

Can't treat them equally when they are wrapped. Since doing so will enforce wrong indentations to them when they are wrapped in class def. This is in context to the current code workflow!

Bottom line: all and any modifiers on a class definition should be treated identically (aside from annotations possibly); if they are in a line wrapped class definition, the second and all subsequent lines should have the correct line wrapping indentation. I don’t know why we keep coming back to certain modifiers. Does this check treat different modifiers differently?

[kalpadiptyaroy]

Bottom line: all and any modifiers on a class definition should be treated identically (aside from annotations possibly); if they are in a line wrapped class definition, the second and all subsequent lines should have the correct line wrapping indentation. I don’t know why we keep coming back to certain modifiers. Does this check treat different modifiers differently?

See my code changes and test input file.
In the PR

[kalpadiptyaroy]

@nrmancuso - See this regression report.
Maybe it will help you understand the problem better and develop confidence.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/14e94b2_2024071312/reports/diff/index.html

[kalpadiptyaroy]

Hi. @rnveach @nrmancuso.

I have written a number of wrapping scenarios and audited them with checkstyle.
Let me know your thoughts for these violations - which ones you feel are genuine should be there and those which shouldn't be there.

Config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="tabWidth" value="4"/>
  <property name="charset" value="UTF-8"/>
  <property name="severity" value="warning"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="Indentation">
      <property name="tabWidth" value="4"/>
      <property name="basicOffset" value="2"/>
      <property name="braceAdjustment" value="2"/>
      <property name="caseIndent" value="2"/>
      <property name="throwsIndent" value="2"/>
      <property name="lineWrappingIndentation" value="4"/>
      <property name="arrayInitIndent" value="2"/>
    </module>
  </module>
</module>

public class InputIndentationClassDeclarationWrapped {
  public class NoWrap {};

  public
      static class WrapBeforeStatic {};

  public static
      class WrapAfterStatic {};

  public static
      final class WrapAfterStaticAndBeforeFinal {};

  public
      static
          final class WrapBeforeStaticAndBeforeFinal {};

  public
      static
          final
              class WrapBeforeStaticAndAfterFinal {};

  public @X class NoWrapWithAnnotation {};

  public @X @Y @Z class NoWrapWithManyAnnotations {};

  public
      @X @Y @Z class ManyAnnotationsWrapped {};

  public @X
      @Y @Z class OneWrapWithManyAnnotations {};

  public @X 
      @Y
        @Z class ManyAnnotationsWithManyWraps {};

  public static @X @Y final class NoWrap2 {};

  public static
      @X @Y final class  WrapBetweenStaticAndAnnotation {};

  public static @X @Y
      final class WrapBetweenAnnotationAndFinal {};

  public static @X
      @Y final
          class ManyWrap2 {};

  public
      static @X @Y
          final class ManyWraps3 {}; 
  
  @interface X {};
  @interface Y {};
  @interface Z {};
}

Audit Report - master br.

kalpadiptya@Kalpadiptya:~/work/checkstyle$ java -jar target/checkstyle-10.15.0-SNAPSHOT-all.jar -c config.xml InputIndentationClassDeclarationWrapped.java
Starting audit...
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:5:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:11:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:14:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:15:11: 'class def modifier' has incorrect indentation level 10, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:18:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:19:11: 'class def modifier' has incorrect indentation level 10, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:27:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:30:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:33:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:34:9: 'class def modifier' has incorrect indentation level 8, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:39:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:42:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:45:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:49:7: 'class def modifier' has incorrect indentation level 6, expected level should be 2. [Indentation]
[WARN] /home/kalpadiptya/work/checkstyle/InputIndentationClassDeclarationWrapped.java:50:11: 'class def modifier' has incorrect indentation level 10, expected level should be 2. [Indentation]
Audit done.

[rnveach]

If config is set correctly, the java code looks formatted fine, so no violations. @nrmancuso 's concern is all modifiers get equal treatment, not what correct formatting looks like.

[kalpadiptyaroy]

@rnveach - Hmm. I see.
So in my opinion this method is the guy who is discriminating the modifiers for equal treatment.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/AbstractExpressionHandler.java

Lines 493 to 508 in e30260d

	/**
	* Check the indentation level of modifiers.
	*/
	protected void checkModifiers() {
	final DetailAST modifiers =
	mainAst.findFirstToken(TokenTypes.MODIFIERS);
	for (DetailAST modifier = modifiers.getFirstChild();
	modifier != null;
	modifier = modifier.getNextSibling()) {
	if (isOnStartOfLine(modifier)
	&& !getIndent().isAcceptable(expandedTabsColumnNo(modifier))) {
	logError(modifier, "modifier",
	expandedTabsColumnNo(modifier));
	}
	}
	}

And interestingly this function is emitting violations. which as per LineWrappingHandler should not be a violation.
One more observation is this checkModifier func. is strictly enforcing class defs modifiers to be on the same line and it is not allowing wrapping!

How should we deal with checkModifier func.? one natural way is overriding - which we cannot do since LineWrappingHandler doesn't extend AbstractExpressionHandler.