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

NeedBraces: lambda with no braces not reported when contents span multiple lines #3837

Closed
rnveach opened this issue Feb 16, 2017 · 14 comments

Comments

@rnveach
Copy link
Member

commented Feb 16, 2017

$ cat TestClass.java
public class TestClass {
    void method() {
        final Set<FullIdent> imports = new HashSet<>();

        imports.stream().filter(full -> test(1, 2,
            3, 4));
    }

    boolean test(int p1, int p2, int p3, int p4) { return true; }
}

$ 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">
    <module name="NeedBraces">
      <property name="tokens" value="LAMBDA"/>
      <property name="allowSingleLineStatement" value="true"/>
    </module>
    </module>
</module>

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

allowSingleLineStatement is turned on, but this lambda isn't a single line as the method call for test spans 2 lines and is inside the lambda.
I was expecting a violation to be reported on line 5.

Example is taken from checkstyle at

imports.stream().filter(full -> imp.getText().equals(full.getText()))
.forEach(full -> log(ast.getLineNo(), ast.getColumnNo(),
MSG_DUPLICATE, full.getLineNo(),
imp.getText()));
. We are using this same check and configuration.

@romani

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

pre-assigned to @johnyleebrown .

@johnyleebrown

This comment has been minimized.

Copy link

commented Mar 15, 2017

I'm on it.

@johnyleebrown

This comment has been minimized.

Copy link

commented Mar 19, 2017

@rnveach I've been trying to find a solution, so i started by editing method isSingleLineLambda(DetailAST lambda):
screen shot 2017-03-20 at 01 01 18
But when i'm trying to test, this error comes up, can you tell me what does it relate to?Thanks.
screen shot 2017-03-20 at 00 58 01
Without line 420 everything tests fine.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

@johnyleebrown This is how we test changes to the check.


We tell the results we are expecting and it verifies those violations come up.
If your changes cause no violations, than the expected results need to change so tests pass.

Looking at input file,

static Runnable r3 = () ->
String.CASE_INSENSITIVE_ORDER.equals("Hello world two!");
, to me it looks like that violation should stay as lambda spans 2 lines.

We cannot verify your changes until the PR as we need to be able to see what has changed and what regression will show us.

@johnyleebrown

This comment has been minimized.

Copy link

commented Mar 20, 2017

@rnveach maybe I should create a test explicitly for multiple lines lambda, woth the code I added?

I'll create a pr later today.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

maybe I should create a test explicitly for multiple lines lambda

Yes, we expect good cases (no violation) and bad cases (violation). Example from issue should be in the test cases and any others that you can generate that are pertinent to the issue.

@johnyleebrown

This comment has been minimized.

Copy link

commented Mar 20, 2017

@rnveach ,got it. Thanks.

@romani

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

issue is assigned to @sharang108 .

@sharang108

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

@romani I am on it!

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2017

@romani How do we find the end of the line for the lambda inside the expression/statement?
Based on the AST tree, it is embedded in the method call.
code:

        imports.stream().filter(full -> test(1, 2,
            3, 4));

Print:

    |      |      |--ELIST -> ELIST [7:37]
    |      |      |  `--LAMBDA -> -> [7:37]
    |      |      |      |--IDENT -> full [7:32]
    |      |      |      `--EXPR -> EXPR [7:44]
    |      |      |          `--METHOD_CALL -> ( [7:44]
    |      |      |              |--IDENT -> test [7:40]
    |      |      |              |--ELIST -> ELIST [7:45]
    |      |      |              |  |--EXPR -> EXPR [7:45]
    |      |      |              |  |  `--NUM_INT -> 1 [7:45]
    |      |      |              |  |--COMMA -> , [7:46]
    |      |      |              |  |--EXPR -> EXPR [7:48]
    |      |      |              |  |  `--NUM_INT -> 2 [7:48]
    |      |      |              |  |--COMMA -> , [7:49]
    |      |      |              |  |--EXPR -> EXPR [8:12]
    |      |      |              |  |  `--NUM_INT -> 3 [8:12]
    |      |      |              |  |--COMMA -> , [8:13]
    |      |      |              |  `--EXPR -> EXPR [8:15]
    |      |      |              |      `--NUM_INT -> 4 [8:15]
    |      |      |              `--RPAREN -> ) [8:16]
    |      |      `--RPAREN -> ) [8:17]
@romani

This comment has been minimized.

Copy link
Member

commented Nov 19, 2017

@rnveach ,

    |      |      |  `--LAMBDA -> -> [7:37]
.......
    |      |      |              `--RPAREN -> ) [8:16]

start at line 7 finished at 8, look simple.
am I missing smth ?

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2017

At the time I assumed --RPAREN -> ) [8:17] was the end since it wraps the lambda.
So we just keep getting the last child of LAMBDA until the end?

@romani

This comment has been minimized.

Copy link
Member

commented Nov 19, 2017

yes

strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 11, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 11, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 11, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 19, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 23, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 1, 2019
rnveach added a commit that referenced this issue Aug 1, 2019
@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Fix was merged

@rnveach rnveach closed this Aug 1, 2019

@rnveach rnveach added the bug label Aug 1, 2019

@rnveach rnveach added this to the 8.24 milestone Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.