Indentation: Unexpected violation when class declaration is wrapped after public and before static about checkstyle HOT 15 OPEN

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.

from checkstyle.

@kalpadiptyaroy There is literally no information about what this issue is about. Issue title is not enough.
https://checkstyle.org/report_issue.html#How_to_report_a_bug.3F

from checkstyle.

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.

from checkstyle.

@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.

from checkstyle.

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.

from checkstyle.

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.

from checkstyle.

@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.

from checkstyle.

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.

from checkstyle.

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!

from checkstyle.

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?

from checkstyle.

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

from checkstyle.

@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

from checkstyle.

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.

from checkstyle.

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.

from checkstyle.

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

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.

from checkstyle.