IndentationCheck. issue 281

[Alexey Zuy]

Hi Roman,

issue 281: https://github.com/checkstyle/checkstyle/issues/281

I have prepared UT input for this issue. Check must not generate violations for this code(see attachements) in configuration:
basicOffset = 2
lineWrappingIndentation = 4

please review.

thanks,
Zuy Alexey

[Ruslan Diachenko]

Hi Alexey,

Could you please provide more test cases to cover the following comment:

This gets worse if you have chained lamdas too, as each chaining requires another level of indentation

For example:

public static void fun() {

   String[] filterVals = { "1", "2" };

   Arrays.asList("1","2","3")

       .stream()

       .filter(chain(x-> 

           (y-> 

               y.equals(x)

           ), filterVals))

       .count();

}

public static <T> Predicate<T> chain (Function<T,Predicate<T>> mapFn, T[]args) {

   return Arrays.asList(args)

       .stream()

       .map(x -> 

           mapFn.apply(x))

       .reduce(p -> 

           false, 

           Predicate::or);

}

[Alexey Zuy]

Hi Ruslan,

I prepared one test input for testing (in attachments).

[Alexey Zuy]

I was looking over unit tests for IndentationCheck and found some strange tests.

This test case claims that Check should generate violation "130: 'object def lcurly' have incorrect indentation level 8, expected level should be one of the following: 10, 14.",
however left curly indented correctly:

// ...
final class InputValidClassDefIndent66 extends java.awt.event.MouseAdapter implements java.awt.event.MouseListener {
    // ...
    private void myMethod() { //indent:4 exp:4
        // ...
          new JButton().addActionListener(new ActionListener() //line:129 indent:10 exp:8
        { //line:130 indent:8 exp:8
            public void actionPerformed(ActionEvent e) { //line:131 indent:12 exp:12
                 //line:132
            } //line:133
        }); //line:134

        // ...      
    }
    // ...
}

From my standpoint Check should not generate violation for left curly, so this must be considered false positive.

What do you think ?

[Ruslan Diachenko]

Hi Alexey,

>> I prepared one test input for testing (in attachments)

Test inputs seems ok to me.

>> From my standpoint Check should not generate violation for left curly, so this must be considered false positive

It is actually a tricky case. Violations on lines 130 and 134 depend on the line 129 because they form an inner block. As line 129 isn't indented properly check says that you have to indent lines 130 and 134 relative to 129. So, it is not false positive. If you fix the original violation in line 129 violations on lines 130 and 134 will disappear.

[Alexey Zuy]

Hi Ruslan,

It is actually a tricky case. Violations on lines 130 and 134 depend on the line 129 because they form an inner block. As line 129 isn't indented properly check says that you have to indent lines 130 and 134 relative to 129. So, it is not false positive. If you fix the original violation in line 129 violations on lines 130 and 134 will disappear.

May I change this behavior, so that Check will not generate violation for left curly and right curly braces(on lines 130 and 134) ? I think it possible if Check will always calculate correct indentation for each node relatively to CORRECT indentation level of parent(not the ACTUAL indentation).
What do you think?


thanks,
Zuy Alexey

[Ruslan Diachenko]

It is ok for this case:

1    private void myMethod() { //indent:4 exp:4
2
3          new JButton().addActionListener(new ActionListener() //indent:10 exp:8
4        { //indent:8 exp:8
5            public void actionPerformed(ActionEvent e) { //indent:12 exp:12
6
7            }
8        });
9    
10   }

but how are you going to deal with such a case:

1    private void myMethod() { //indent:4 exp:4
2
3          new JButton().addActionListener(new ActionListener() //indent:10 exp:8
4         { //indent:9 exp:8
5            public void actionPerformed(ActionEvent e) { //indent:12 exp:12
6
7            }
8         }); //indent:9 exp:8
9    
10   }

Here in lines 4 and 8 violations are expected as well as in line 3.

[Alexey Zuy]

Hi Ruslan,

but how are you going to deal with such a case:

1    private void myMethod() { //indent:4 exp:4
2
3          new JButton().addActionListener(new ActionListener() //indent:10 exp:8
4         { //indent:9 exp:8
5            public void actionPerformed(ActionEvent e) { //indent:12 exp:12
6
7            }
8         }); //indent:9 exp:8
9    
10   }

Here in lines 4 and 8 violations are expected as well as in line 3.

I think Check logic should look like this:
"private void myMethod() {" has indent 4, expected 4 -> no violations;
"new JButton()" is child of method definition -> its expected indentation level is expected indent level of method definition + basicOffset(4) = 8. Actual indent != expected indent -> generate violation
"addActionListener(...)" is child of "new JButton()". This node is not at the start of the line its expected indentation is expected indentation of parent. -> no violation, because we dont even check it
"new ActionListener()" is child of method call(addActionListener). This node is not at the start of the line so its expected indentation is expected indentation of parent. -> no violation, because we dont check it
"<statement list>" is child of "new ActionListener()", expected indentation level of curly is expected indentation of parent(8). Actual curly indent is 9, 9 != 8 -> generate violation.
and so on ...

[Ruslan Diachenko]

Is the current issue with lambdas somehow relating with this case? If not, please concentrate on the original problem and I'll think more about the case you found.

[Roman Ivanov]

Hi Alexey,

> I think Check logic should look like this:

Logic looks clear , but at summer 2014 we introduced as option in IndentationCheck to allow non strict linewrapping so it mean that indentation on parent (but not the top parent!) can be 4,5,6,7,8.....any amount of spaces - it is completely up to the user.

How that will work in your algorithm ?

Please provide links to Test inputs that you are going to use for issue 281. 

Please review my comment on your PR at github.

thanks,

Roman Ivanov

​

[Alexey Zuy]

Hi Roman,

Logic looks clear , but at summer 2014 we introduced as option in IndentationCheck to allow non strict linewrapping so it mean that indentation on parent (but not the top parent!) can be 4,5,6,7,8.....any amount of spaces - it is completely up to the user.

How that will work in your algorithm ?

I will think about this case.

Please provide links to Test inputs that you are going to use for issue 281. 

Test Inputs were already provided earlier in this thread (see attached files).

I extended test inputs with some line wrapping cases. Please review lines 41-73.
Input

thanks,
Zuy Alexey

[Roman Ivanov]

Hi Alexey,

Please provide configuration as a comment on a input class that will help validate your expectations.

Thanks,
Roman Ivanov

--
You received this message because you are subscribed to the Google Groups "checkstyle-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to checkstyle-dev...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Michał Michalak]

Hello there. I'm curious is there any progress regarding this issue? I was just interested in it, because discussion seem to dry several months ago, but GitHub issue still receives a lot of +1.
Thanks
Mike

[Roman Ivanov]

Hi Michal,

in last private conversation with Alexey I understood that he can not continue working with us (temporary or ..... ).

We pushed all his results to remote and placed to github issue a link.

Fell free to pick up his work and continue .

thanks,

Roman Ivanov

[Roman Ivanov]

That might be unrelated , but there are other discussion threads :

"IndentationCheck_issue766"  with last commit link to non completed changes

https://groups.google.com/forum/#!searchin/checkstyle-devel/Indentation/checkstyle-devel/kDGBLXpUohk/p6rIzdNIIp8J 

"IndentationCheck. Correct indentation of anonymous classes."

https://groups.google.com/forum/#!searchin/checkstyle-devel/Indentation/checkstyle-devel/vJK2w1vdj58/cuw24C4tIekJ