Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lineWrappingIndentation falsely detects incorrect indentation for text blocks #15939

Closed
xpple opened this issue Nov 18, 2024 · 10 comments · Fixed by #16058
Closed

lineWrappingIndentation falsely detects incorrect indentation for text blocks #15939

xpple opened this issue Nov 18, 2024 · 10 comments · Fixed by #16058

Comments

@xpple
Copy link

xpple commented Nov 18, 2024

I have read check documentation: https://checkstyle.sourceforge.io/checks/misc/indentation.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

/var/tmp $ javac A.java

/var/tmp $ cat checkstyle.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="charset" value="UTF-8"/>

    <property name="severity" value="${org.checkstyle.google.severity}" default="warning"/>

    <property name="fileExtensions" value="java, properties, xml"/>

    <module name="SuppressWarningsFilter"/>

    <module name="TreeWalker">
        <module name="SuppressWarningsHolder"/>

        <module name="Indentation">
            <property name="lineWrappingIndentation" value="4"/>
            <property name="forceStrictCondition" value="true"/>
        </module>
    </module>
</module>

/var/tmp $ cat A.java
public class A {
    private static final String EXAMPLE = """
        Example string""";
}


/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.20.1-all.jar -c checkstyle.xml A.java
Starting audit...
[WARN] A.java:3:23: '"""' has incorrect indentation level 22, expected level should be 8. [Indentation]
Audit done.

For Windows users, please use type instead of cat and run

set RUN_LOCALE="-Duser.language=en -Duser.country=US"
java %RUN_LOCALE% -jar checkstyle-10.20.1-all.jar -c checkstyle.xml A.java

in place of the last 2 commands above.


Describe what you expect in detail.
The indentation is correct, so no warning should be reported. Note that the warning can be suppressed by adding @SuppressWarnings("checkstyle:indentation") to the declaration, given that SuppressWarningsFilter is configured.

It seems to me that Checkstyle wants the ending """ on a new line. This is not a notational request, though; it'd be a different string as an extra new line character would be added.

See JEP 378 for more information on text blocks.

@romani
Copy link
Member

romani commented Nov 18, 2024

Note that the warning can be suppressed by adding @SuppressWarnings("checkstyle:indentation"

@xpple , please consider to use https://checkstyle.org/filters/suppressionxpathsinglefilter.html#SuppressionXpathSingleFilter
to make suppression ones and not pollute code with SuppressWarnings.

$ java -jar checkstyle-10.20.0-all.jar -t Test.java 
COMPILATION_UNIT -> COMPILATION_UNIT [1:0]
`--CLASS_DEF -> CLASS_DEF [1:0]
    |--MODIFIERS -> MODIFIERS [1:0]
    |   `--LITERAL_PUBLIC -> public [1:0]
    |--LITERAL_CLASS -> class [1:7]
    |--IDENT -> A [1:13]
    `--OBJBLOCK -> OBJBLOCK [1:15]
        |--LCURLY -> { [1:15]
        |--VARIABLE_DEF -> VARIABLE_DEF [2:4]
        |   |--MODIFIERS -> MODIFIERS [2:4]
        |   |   |--LITERAL_PRIVATE -> private [2:4]
        |   |   |--LITERAL_STATIC -> static [2:12]
        |   |   `--FINAL -> final [2:19]
        |   |--TYPE -> TYPE [2:25]
        |   |   `--IDENT -> String [2:25]
        |   |--IDENT -> EXAMPLE [2:32]
        |   |--ASSIGN -> = [2:40]
        |   |   `--EXPR -> EXPR [2:42]
        |   |       `--TEXT_BLOCK_LITERAL_BEGIN -> """ [2:42]
        |   |           |--TEXT_BLOCK_CONTENT -> \n        Example string [2:45]
        |   |           `--TEXT_BLOCK_LITERAL_END -> """ [3:22]
        |   `--SEMI -> ; [3:25]
        `--RCURLY -> } [4:0]


$ 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">

    <property name="charset" value="UTF-8"/>

    <property name="severity" value="${org.checkstyle.google.severity}" default="warning"/>

    <property name="fileExtensions" value="java, properties, xml"/>

    <module name="SuppressWarningsFilter"/>

    <module name="TreeWalker">
        <module name="SuppressWarningsHolder"/>

        <module name="Indentation">
            <property name="lineWrappingIndentation" value="4"/>
            <property name="forceStrictCondition" value="true"/>
        </module>

    </module>
</module>

$ java -jar checkstyle-10.20.0-all.jar -c config.xml Test.java 
Starting audit...
[WARN] /var/tmp/Test.java:3:23: '"""' has incorrect indentation level 22, expected level should be 8. [Indentation]
Audit done.

$ java -jar checkstyle-10.20.0-all.jar -g -c config.xml Test.java 
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suppressions PUBLIC
    "-//Checkstyle//DTD SuppressionXpathFilter Experimental Configuration 1.2//EN"
    "https://checkstyle.org/dtds/suppressions_1_2_xpath_experimental.dtd">
<suppressions>
<suppress-xpath
       files="Test.java"
       checks="IndentationCheck"
       query="/COMPILATION_UNIT/CLASS_DEF[./IDENT[@text='A']]/OBJBLOCK/VARIABLE_DEF[./IDENT[@text='EXAMPLE']]/ASSIGN/EXPR/TEXT_BLOCK_LITERAL_BEGIN[./TEXT_BLOCK_CONTENT[@text='\n        Example string']]/TEXT_BLOCK_LITERAL_END"/>
</suppressions>

$ 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">

    <property name="charset" value="UTF-8"/>

    <property name="severity" value="${org.checkstyle.google.severity}" default="warning"/>

    <property name="fileExtensions" value="java, properties, xml"/>

    <module name="SuppressWarningsFilter"/>

    <module name="TreeWalker">
        <module name="SuppressWarningsHolder"/>

        <module name="Indentation">
            <property name="lineWrappingIndentation" value="4"/>
            <property name="forceStrictCondition" value="true"/>
        </module>
<!-- ADD THIS to YOUR CONFIG --->
    <module name="SuppressionXpathSingleFilter">
      <property name="checks" value="Indentation"/>
      <property name="query" value="//TEXT_BLOCK_LITERAL_END"/>
    </module>

    </module>
</module>

$ java -jar checkstyle-10.20.0-all.jar -c config.xml Test.java 
Starting audit...
Audit done.

@romani
Copy link
Member

romani commented Nov 18, 2024

Check should completely skip indentation validation for all nodes under TEXT_BLOCK_LITERAL_BEGIN

@mohitsatr
Copy link
Contributor

hi @romani ,I'm trying to debug the input.

However I'm getting language level error :

Screenshot from 2024-12-22 09-58-55

and it does not go away even after I have set level to 21

Screenshot from 2024-12-22 09-53-41

@mohitsatr
Copy link
Contributor

also,
// for verification becomes the part of the block-Text/String. see the below image :

Screenshot from 2024-12-22 10-24-20

is there a workaround to avoid this ?
thanks in advance.

@romani
Copy link
Member

romani commented Dec 23, 2024

Idea should be using jdk11
All inputs should be out of Ideal compilation as all of them are for different jdk, see comment at first line of Input file.

Just exclude Input from sources to compile.

FYI: Indentation module is most complicated , if see it is hard, do not hesitate to drop issue and help us with something else.

@mohitsatr
Copy link
Contributor

mohitsatr commented Dec 25, 2024

@romani like you said, I made it skip TEXT_BLOCK_LITERAL_BEGIN node and it's children and logic is working. error is gone.
I assume we are not interesed in checking indentations inside TEXT_BLOCK_LITERALs whether it's right or wrong, right?

@xpple
Copy link
Author

xpple commented Dec 25, 2024

Just FYI, ignoring the check for TEXT_BLOCK_LITERAL_BEGIN nodes is not a fix to this issue. If you set lineWrappingIndentation to four, the following would be correct:

public class A {
    private static final String EXAMPLE = """
        Example string""";
}

The following would not:

public class A {
    private static final String EXAMPLE = """
            Example string"""; // violation 8 != 4
}

The first nonwhitespace character of a text block determines the offset of the text block; in both cases the resulting string is the same.

@romani
Copy link
Member

romani commented Dec 25, 2024

@mohitsatr, please make PR.
We will add and think on all such nuances there .

I assume we are not interesed in checking indentations inside TEXT_BLOCK_LITERALs whether it's right or wrong, right?

All should be skipped for now, as it is just big string value, all spacing is user content. Not a target of this Check for now. Some other Check should be created to handle content of text blocks.

@romani
Copy link
Member

romani commented Dec 27, 2024

@xpple , do you see my point that this Check should avoid text blocks at all?

mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Dec 28, 2024
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Dec 28, 2024
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Dec 30, 2024
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Dec 30, 2024
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Dec 30, 2024
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 2, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 2, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 4, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 8, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 8, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 8, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 8, 2025
mohitsatr added a commit to mohitsatr/checkstyle that referenced this issue Jan 8, 2025
@nrmancuso nrmancuso added the bug label Jan 25, 2025
@github-actions github-actions bot added this to the 10.21.2 milestone Jan 25, 2025
@xpple
Copy link
Author

xpple commented Jan 25, 2025

I see now that this issue was closed, but I forgot to respond.

@xpple , do you see my point that this Check should avoid text blocks at all?

No, I don't. Notice that the following strings are equal:

private static final String A = """
    a
    """;

private static final String B = """
        a
        """;

That is, A.equals(B). However, the indentation is different. In the first string, the indentation is 4, and in the second string the indentation is 8. When the user has defined lineWrappingIndentation at 4, only the first example should pass the check.

dongjoon-hyun pushed a commit to apache/spark that referenced this issue Feb 12, 2025
### What changes were proposed in this pull request?

This PR aims to upgrade `checkstyle` from 10.20.2 to 10.21.2.

### Why are the changes needed?

To pick up bug fixes:
- checkstyle-10.21.2
checkstyle/checkstyle#15939 - lineWrappingIndentation falsely detects incorrect indentation for text blocks
checkstyle/checkstyle#16101 - ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck
- checkstyle-10.21.1
checkstyle/checkstyle#11374 - UnusedLocalVariable: False Positive when inner class has same field as variable

Full release notes:

- https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.2
- https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.1

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Manually by:
```
bash ./dev/lint-java
```

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #49899 from wayneguow/checkstyle.

Authored-by: Wei Guo <guow93@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
trinhnx pushed a commit to trinhnx/checkstyle that referenced this issue Feb 26, 2025
kazemaksOG pushed a commit to kazemaksOG/spark-custom-scheduler that referenced this issue Mar 27, 2025
### What changes were proposed in this pull request?

This PR aims to upgrade `checkstyle` from 10.20.2 to 10.21.2.

### Why are the changes needed?

To pick up bug fixes:
- checkstyle-10.21.2
checkstyle/checkstyle#15939 - lineWrappingIndentation falsely detects incorrect indentation for text blocks
checkstyle/checkstyle#16101 - ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck
- checkstyle-10.21.1
checkstyle/checkstyle#11374 - UnusedLocalVariable: False Positive when inner class has same field as variable

Full release notes:

- https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.2
- https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.1

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Manually by:
```
bash ./dev/lint-java
```

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#49899 from wayneguow/checkstyle.

Authored-by: Wei Guo <guow93@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants