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

Indentation: multiple try with resource not checked #3131

Closed
rnveach opened this Issue Apr 26, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Apr 26, 2016

$ cat TestClass.java
public class TestClass //indent:0 exp:0
{ //indent:0 exp:0
    private static void test(String fn) { //indent:4 exp:4
        try (FileInputStream fis = new FileInputStream(fn); //indent:8 exp:8
JarInputStream jis = new JarInputStream(fis, false)) //indent:0 // line 5
        { //indent:8 exp:8
        }
    } //indent:4 exp:4
} //indent:0 exp:0


$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

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

    <module name="TreeWalker">
<property name="tabWidth" value="4"/>

    <module name="Indentation">
      <property name="arrayInitIndent" value="4"/>
      <property name="basicOffset" value="4"/>
      <property name="braceAdjustment" value="0"/>
      <property name="lineWrappingIndentation" value="4"/>
    </module>
    </module>
</module>

$ java -jar checkstyle-6.17-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

I was expecting some type of violation on line 5, like requiring it to be at column 8 or 12, as it doesn't fit at column 0.

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 6, 2017

Contributor

I am on it.

Contributor

kukreja-vikramaditya commented Apr 6, 2017

I am on it.

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 6, 2017

Contributor

@rnveach
I'll take it as 12 for now. But what should happen if line wrapping in such a case

public class TestClass //indent:0 exp:0
{ //indent:0 exp:0
    private static void test(String fn) { //indent:4 exp:4
        try (FileInputStream fis = new FileInputStream(fn); //indent:8 exp:8
            JarInputStream jis = new JarInputStream(fis, //indent:12 acceptable 8/12
 false)) //indent:0 // line 6
        { //indent:8 exp:8
        }
    } //indent:4 exp:4
} //indent:0 exp:0

In general what can be done about the indentation of line 6? Are there any style guides that give info about such cases? try resources in general

Contributor

kukreja-vikramaditya commented Apr 6, 2017

@rnveach
I'll take it as 12 for now. But what should happen if line wrapping in such a case

public class TestClass //indent:0 exp:0
{ //indent:0 exp:0
    private static void test(String fn) { //indent:4 exp:4
        try (FileInputStream fis = new FileInputStream(fn); //indent:8 exp:8
            JarInputStream jis = new JarInputStream(fis, //indent:12 acceptable 8/12
 false)) //indent:0 // line 6
        { //indent:8 exp:8
        }
    } //indent:4 exp:4
} //indent:0 exp:0

In general what can be done about the indentation of line 6? Are there any style guides that give info about such cases? try resources in general

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 7, 2017

Member

@kukreja-vikramaditya

In general what can be done about the indentation of line 6? Are there any style guides that give info about such cases?

Our only guide are IDEs. Many IDEs offer auto-formatting of code. Users will rely on that more than our check for uniformity, especially since they can auto-format the code on save without any action from user.
Please try formatting the example in the 3 major IDEs (Eclipse, NetBeans, IntelliJ) with their default formatters and see how they handle the wrapping and post the results here.
To me, it looks like it should be requested to be indented by lineWrappingIndentation.

Member

rnveach commented Apr 7, 2017

@kukreja-vikramaditya

In general what can be done about the indentation of line 6? Are there any style guides that give info about such cases?

Our only guide are IDEs. Many IDEs offer auto-formatting of code. Users will rely on that more than our check for uniformity, especially since they can auto-format the code on save without any action from user.
Please try formatting the example in the 3 major IDEs (Eclipse, NetBeans, IntelliJ) with their default formatters and see how they handle the wrapping and post the results here.
To me, it looks like it should be requested to be indented by lineWrappingIndentation.

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 7, 2017

Contributor

(no line wrapping. each diff line)
This is the result of an input file I was working on.
Currently the changes I have made to TryHandler can only detect error in the starting line of a resource as in my soln I am detecting differences in RESOURCE_SPECIFICATION, RESOURCES and RESOURCE tokens. If works fine but only compares RESOURCE with RESOURCES for now. Is something like checkExpressionSubtree required too for checking the children of LITERAL_TRY in cases of try with resources.

1. IDEA
UPDATE: The indentaion changes if one of the line in written directrly after the ( in try resources

    void method(InputStream is) {
        try (
                BufferedInputStream bis = new BufferedInputStream(is); //indent:16
                BufferedInputStream bis1 = new BufferedInputStream(is,
                        0);
                BufferedInputStream bis2 = new
                        BufferedInputStream
                        (
                                is
                                ,
                                10)
                ;
                BufferedInputStream
                        bis3
                        =
                        new BufferedInputStream(
                                is,
                                Integer
                                        .parseInt
                                                (
                                                        "123"
                                                )
                        );

        ) {
        } catch (IOException
                e
                ) {
            e.printStackTrace();
        }
    }

2. Netbeans
Update: Indentation remains same even if a new resource is added directly after try (

    void method(InputStream is) {
        try (
                BufferedInputStream bis = new BufferedInputStream(is);
                BufferedInputStream bis1 = new BufferedInputStream(is,
                        0);
                BufferedInputStream bis2 = new BufferedInputStream(
                        is,
                         10);
                 BufferedInputStream bis3
                = new BufferedInputStream(
                        is,
                        Integer
                                .parseInt(
                                        "123"
                                )
                );) {
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

3. Eclipse
Update: It adds \t automatically everywhere

	void method(InputStream is) {
		try (BufferedInputStream bis = new BufferedInputStream(is);
				BufferedInputStream bis1 = new BufferedInputStream(is, 0);
				BufferedInputStream bis2 = new BufferedInputStream(is, 10);
				BufferedInputStream bis3 = new BufferedInputStream(is,
						Integer.parseInt("123"));

		) {
		} catch (IOException e) {
			e.printStackTrace();
		}
	}

Please review UPDATE points as well

Contributor

kukreja-vikramaditya commented Apr 7, 2017

(no line wrapping. each diff line)
This is the result of an input file I was working on.
Currently the changes I have made to TryHandler can only detect error in the starting line of a resource as in my soln I am detecting differences in RESOURCE_SPECIFICATION, RESOURCES and RESOURCE tokens. If works fine but only compares RESOURCE with RESOURCES for now. Is something like checkExpressionSubtree required too for checking the children of LITERAL_TRY in cases of try with resources.

1. IDEA
UPDATE: The indentaion changes if one of the line in written directrly after the ( in try resources

    void method(InputStream is) {
        try (
                BufferedInputStream bis = new BufferedInputStream(is); //indent:16
                BufferedInputStream bis1 = new BufferedInputStream(is,
                        0);
                BufferedInputStream bis2 = new
                        BufferedInputStream
                        (
                                is
                                ,
                                10)
                ;
                BufferedInputStream
                        bis3
                        =
                        new BufferedInputStream(
                                is,
                                Integer
                                        .parseInt
                                                (
                                                        "123"
                                                )
                        );

        ) {
        } catch (IOException
                e
                ) {
            e.printStackTrace();
        }
    }

2. Netbeans
Update: Indentation remains same even if a new resource is added directly after try (

    void method(InputStream is) {
        try (
                BufferedInputStream bis = new BufferedInputStream(is);
                BufferedInputStream bis1 = new BufferedInputStream(is,
                        0);
                BufferedInputStream bis2 = new BufferedInputStream(
                        is,
                         10);
                 BufferedInputStream bis3
                = new BufferedInputStream(
                        is,
                        Integer
                                .parseInt(
                                        "123"
                                )
                );) {
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

3. Eclipse
Update: It adds \t automatically everywhere

	void method(InputStream is) {
		try (BufferedInputStream bis = new BufferedInputStream(is);
				BufferedInputStream bis1 = new BufferedInputStream(is, 0);
				BufferedInputStream bis2 = new BufferedInputStream(is, 10);
				BufferedInputStream bis3 = new BufferedInputStream(is,
						Integer.parseInt("123"));

		) {
		} catch (IOException e) {
			e.printStackTrace();
		}
	}

Please review UPDATE points as well

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 7, 2017

Contributor

Also check input file at src/test/resources/com/puppycrawl/tools/checkstyle/checks/indentation/InputValidTryResourcesIndent.java

This file uses indent of 4 for multiline resources (no line wrapping). Is this done because there was no check for something similar? Will this be needed to be updated too?

Contributor

kukreja-vikramaditya commented Apr 7, 2017

Also check input file at src/test/resources/com/puppycrawl/tools/checkstyle/checks/indentation/InputValidTryResourcesIndent.java

This file uses indent of 4 for multiline resources (no line wrapping). Is this done because there was no check for something similar? Will this be needed to be updated too?

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 7, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 7, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 8, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 9, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 10, 2017

Member

It adds \t automatically everywhere

Eclipse by default uses tabs for indentation, instead of spaces.

This is the result of an input file I was working on.

I got different results on my IDEA. It wouldn't break any lines, but it did line up all BufferedInputStream resources like your results.

The basic concensus is, it looks like all IDEs treat the resources as wrapped lines as they are all indented by 8 spaces.
offset of a resource should be try position + lineWrappingIndentation. offset of wrapped resource should be resource position + lineWrappingIndentation.
@romani You agree?

Member

rnveach commented Apr 10, 2017

It adds \t automatically everywhere

Eclipse by default uses tabs for indentation, instead of spaces.

This is the result of an input file I was working on.

I got different results on my IDEA. It wouldn't break any lines, but it did line up all BufferedInputStream resources like your results.

The basic concensus is, it looks like all IDEs treat the resources as wrapped lines as they are all indented by 8 spaces.
offset of a resource should be try position + lineWrappingIndentation. offset of wrapped resource should be resource position + lineWrappingIndentation.
@romani You agree?

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 11, 2017

Contributor

@rnveach

UPDATE: The indentaion changes if one of the line in written directrly after the ( in try resources

For IDEA
When the first resource is placed directly after the try, the indentation of other lines changes as well as shown here. This results in the starting characters to be in a single line. I however still get line breaking as shown.
UPDATE Ofcourse I created that line breaking in the first place, but IDEA doesn't convert them into a single line.

    void method(InputStream is) {
        try (BufferedInputStream bis = new BufferedInputStream(is); //indent:8
             BufferedInputStream bis1 = new BufferedInputStream(is, //indent:13
                     0);
             BufferedInputStream bis2 = new
                     BufferedInputStream
                     (
                             is
                             ,
                             10)
             ;
             BufferedInputStream
                     bis3
                     =
                     new BufferedInputStream(
                             is,
                             Integer
                                     .parseInt
                                             (
                                                     "123"
                                             )
                     );

        ) {
        } catch (IOException
                e
                ) {
            e.printStackTrace();
        }
    }

Do you want me to check for such cases as well? Use first resource's starting column for other resources in that try block ?

UPDATE I used the first example (IDEA) and used them on the other IDEs. Those were the final results

Contributor

kukreja-vikramaditya commented Apr 11, 2017

@rnveach

UPDATE: The indentaion changes if one of the line in written directrly after the ( in try resources

For IDEA
When the first resource is placed directly after the try, the indentation of other lines changes as well as shown here. This results in the starting characters to be in a single line. I however still get line breaking as shown.
UPDATE Ofcourse I created that line breaking in the first place, but IDEA doesn't convert them into a single line.

    void method(InputStream is) {
        try (BufferedInputStream bis = new BufferedInputStream(is); //indent:8
             BufferedInputStream bis1 = new BufferedInputStream(is, //indent:13
                     0);
             BufferedInputStream bis2 = new
                     BufferedInputStream
                     (
                             is
                             ,
                             10)
             ;
             BufferedInputStream
                     bis3
                     =
                     new BufferedInputStream(
                             is,
                             Integer
                                     .parseInt
                                             (
                                                     "123"
                                             )
                     );

        ) {
        } catch (IOException
                e
                ) {
            e.printStackTrace();
        }
    }

Do you want me to check for such cases as well? Use first resource's starting column for other resources in that try block ?

UPDATE I used the first example (IDEA) and used them on the other IDEs. Those were the final results

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Apr 11, 2017

Contributor

offset of a resource should be try position + lineWrappingIndentation. offset of wrapped resource should be resource position + lineWrappingIndentation.

Take example case where the resource itself has wrong indent,
Ex, line 4

        try ( //indent:8 exp:8
                BufferedWriter writer = Files.newBufferedWriter(outputFilePath, charset); //indent:16 exp:16
               ZipFile zf = new ZipFile( //indent:15 exp:16 warn
                   zipFileName) //indent:19 exp:19? or 20?  warn?
        ) { //indent:8 exp:8
            zf.getName(); //indent:12 exp:12
        } //indent:8 exp:8

Similarly if try has wrong indent, Should indent of next line be 16?

Ex

       try ( //indent:7 exp:8 warn
                BufferedWriter writer = Files.newBufferedWriter(outputFilePath, charset); //indent:16 exp:16
               ZipFile zf = new ZipFile( //indent:15 exp:16 warn
                   zipFileName) //indent:19 exp:19? or 20?  warn?
        ) { //indent:8 exp:8
            zf.getName(); //indent:12 exp:12
        } //indent:8 exp:8
Contributor

kukreja-vikramaditya commented Apr 11, 2017

offset of a resource should be try position + lineWrappingIndentation. offset of wrapped resource should be resource position + lineWrappingIndentation.

Take example case where the resource itself has wrong indent,
Ex, line 4

        try ( //indent:8 exp:8
                BufferedWriter writer = Files.newBufferedWriter(outputFilePath, charset); //indent:16 exp:16
               ZipFile zf = new ZipFile( //indent:15 exp:16 warn
                   zipFileName) //indent:19 exp:19? or 20?  warn?
        ) { //indent:8 exp:8
            zf.getName(); //indent:12 exp:12
        } //indent:8 exp:8

Similarly if try has wrong indent, Should indent of next line be 16?

Ex

       try ( //indent:7 exp:8 warn
                BufferedWriter writer = Files.newBufferedWriter(outputFilePath, charset); //indent:16 exp:16
               ZipFile zf = new ZipFile( //indent:15 exp:16 warn
                   zipFileName) //indent:19 exp:19? or 20?  warn?
        ) { //indent:8 exp:8
            zf.getName(); //indent:12 exp:12
        } //indent:8 exp:8
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 11, 2017

Member

Ofcourse I created that line breaking in the first place, but IDEA doesn't convert them into a single line.

That is what I am seeing, IDEA doesn't break/merge lines by default. I assumed yours was which was why I was commenting on how I wasn't seeing your broken up like that, but I understand now that you did it to show its indentation level for various breaks.

         BufferedInputStream bis1 = new BufferedInputStream(is, //indent:13

I am seeing this behavior as well, it is keeping the 2nd+ resources lined up with first resource in the try. Unless @romani specifies otherwise, I am fine for ignoring this for now to keep implementation simple.
Edit: We may have to support since this is what Orekit does in regression.
https://github.com/CS-SI/Orekit/blob/eb95dd1bd5cb49322da3779063a9e98c16c69e13/src/main/java/org/orekit/files/sp3/SP3Parser.java#L189-L191

Use first resource's starting column for other resources in that try block ?

No, see my other comment.

offset of a resource should be try position + lineWrappingIndentation

Subsequent resources should use that same offset that was generated from first.

Take example case where the resource itself has wrong indent,
zipFileName) //indent:19 exp:19? or 20? warn?
Similarly if try has wrong indent, Should indent of next line be 16?

I asked this same question in the PR at #4171 (comment) to @romani . I am fine with implementation where requested resource indent is based on requested indentation of try, not it's actual position. Same with wrapping the resource, based on requested indentation not actual.
We do base indentation in other areas on actual positions, but I am not sure why. You can see this with wrapping throws list in method definition.

Member

rnveach commented Apr 11, 2017

Ofcourse I created that line breaking in the first place, but IDEA doesn't convert them into a single line.

That is what I am seeing, IDEA doesn't break/merge lines by default. I assumed yours was which was why I was commenting on how I wasn't seeing your broken up like that, but I understand now that you did it to show its indentation level for various breaks.

         BufferedInputStream bis1 = new BufferedInputStream(is, //indent:13

I am seeing this behavior as well, it is keeping the 2nd+ resources lined up with first resource in the try. Unless @romani specifies otherwise, I am fine for ignoring this for now to keep implementation simple.
Edit: We may have to support since this is what Orekit does in regression.
https://github.com/CS-SI/Orekit/blob/eb95dd1bd5cb49322da3779063a9e98c16c69e13/src/main/java/org/orekit/files/sp3/SP3Parser.java#L189-L191

Use first resource's starting column for other resources in that try block ?

No, see my other comment.

offset of a resource should be try position + lineWrappingIndentation

Subsequent resources should use that same offset that was generated from first.

Take example case where the resource itself has wrong indent,
zipFileName) //indent:19 exp:19? or 20? warn?
Similarly if try has wrong indent, Should indent of next line be 16?

I asked this same question in the PR at #4171 (comment) to @romani . I am fine with implementation where requested resource indent is based on requested indentation of try, not it's actual position. Same with wrapping the resource, based on requested indentation not actual.
We do base indentation in other areas on actual positions, but I am not sure why. You can see this with wrapping throws list in method definition.

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 12, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 12, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 17, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 29, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Apr 29, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue May 22, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue May 27, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jun 4, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jun 14, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jun 19, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jun 29, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 8, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 9, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 10, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 13, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 13, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 14, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 21, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 25, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 25, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 26, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 27, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 27, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 27, 2017

kukreja-vikramaditya added a commit to kukreja-vikramaditya/checkstyle that referenced this issue Jul 30, 2017

romani added a commit that referenced this issue Jul 31, 2017

@romani romani added the bug label Jul 31, 2017

@romani romani added this to the 8.2 milestone Jul 31, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 31, 2017

Member

@kukreja-vikramaditya , thanks a lot for your fix to this not simple problem to the most complicated Check in whole checkstyle.

Member

romani commented Jul 31, 2017

@kukreja-vikramaditya , thanks a lot for your fix to this not simple problem to the most complicated Check in whole checkstyle.

@romani romani closed this Jul 31, 2017

@kukreja-vikramaditya

This comment has been minimized.

Show comment
Hide comment
@kukreja-vikramaditya

kukreja-vikramaditya Jul 31, 2017

Contributor

@romani @rnveach
Thanks for helping out!

Contributor

kukreja-vikramaditya commented Jul 31, 2017

@romani @rnveach
Thanks for helping out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment