Add lambda support to indentation check

[pietern]

This is a first pass for lambda support in the indentation check. All unit and integration tests pass. The test case is based on the test case in this branch: master...pirat9600q:IndentationCheck_issue281. Not all cases will be covered in this commit, but at least it's a start. I ran checkstyle against a codebase I'm working on and all the lambda related warnings disappear with this commit.

Also see #281.

[mkordas]

Thanks for the great update, just couple of things:

• There are multiple builds failed at https://travis-ci.org/checkstyle/checkstyle/builds/73946529, just run mvn clean verify before commit
• put issue number to the commit message according to http://checkstyle.sourceforge.net/contributing.html
• Avoid mysterious abreviations in comments likew.r.t.
• please run analysis on project with many lambdas like infinispan/infinispan

[pietern]

Thanks for the feedback. Addressed your comments. Only thing remaining is a cobertura coverage error.

[codecov-io]

Current coverage is 99.65%

Merging #1548 into master will not affect coverage as of 11cd175

Powered by Codecov. Updated on successful CI builds.

[pietern]

The coverage is now fixed by removing an apparently superfluous branch :)

I ran this against infinispan and it didn't crash (it had a zillion warnings/errors though).

Let me know if there is anything else that is blocking this from going in.

[uschi2000]

Hi folks,
In which release is this change expected to be available?
Thanks! R.

[romani]

Most likely at the end of September, we will not have time to review and test till end of August

[romani]

@mkordas , can you finish code-review for this PR?

[romani]

@pietern , sorry for delay , please rebase your PR over latest code we are about to start reviewing your code.

[mkordas]

more common is to say lambda expressions instead of lambda statements

[pietern]

You're right, I started out with a copy of one of the other handlers.

[pietern]

Thank you for reviewing. Addressing the comments in a new commit.

[mkordas]

I don't see any declaration of SomeInterface - please make input compilable in Java 8.

[mkordas]

Please use src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/coding/InputReturnCountLambda.java as a source of ideas for some more complicated lambdas.

For example these are valid lambdas:

    Supplier<Supplier<Integer>> lambdas() {
        return (
        )
                ->
                (

                )
                        ->
                        1
                ;

[mkordas]

@pietern, did you have time to work on this PR?

[pietern]

@mkordas I incorporated your comments, added some more test cases where the lambda's argument list or the lambda token itself needs to be indented, and rebased on top of master.

[mkordas]

Thanks @pietern, I'll look into it shortly.

[mkordas]

@pietern
TeamCity noticed allocation of zero length array and unnecessary 'this' qualifiers - please fix it.

[pietern]

Working on the infinispan test cases.

[romani]

@pietern and @mkordas ,

One thing we should clearly understand: I am completely ok to merge and release non complete lambda support, but I am NOT ok if we introduce violations on code that does not have lambda at all (majority of projects still use jdk6, jdk7).

incomplete Lambda support:

• it is better then nothing, user will benefit from it, even it is not complete.
• we do not close issue, and continue working with lambda false-positives, it is ok to fix issue in few steps(few PRs)
• merging first PR will help to validate all other PRs and changes before and after should be minimal.

So lets focus on non-lambda false positives ! lets test project that do not have lambda at all, and if reports(before/after) are the same we are ok to merge.

[pietern]

@romani Sounds good to me :)

Still looking why the infinispan method def modifier triggers a warning. The others can be attributed to inconsistent line continuation indenting in that repository; sometimes they use 6, other times they use 8. I used 6 in my configuration, so this triggers a warning for 8. Notice that since this is a new check, it will always produce new warnings of this type (lambda arguments).

I pushed a new change with the inspections fixed.

[pietern]

@romani With the latest change we should be good on not breaking existing code. I expanded the set of allowed indentations for children to include line wrapping (which is what was originally intended, over basic indenting, it looks like) and line wrapping based on the parent's suggested level if the right paren is the first on the line. Both additions are tested in the lambda test input files (because it seems the lambda is one of the few that checks the indentation of the arguments, which applies in method call arguments as well). This addresses some of the lambda false positives that @mkordas mentioned for protonpack.

For the method call handler diff, see: 2224d1e#diff-77dfb9ce50cfab374af2799f71e68cfb

I pushed the new reports and this is the summarized result:

	before	after
RxJava	811	807
infinispan	33604	31617
jOOL	454	454
protonpack	60	23

[pietern]

Regarding the infinispan method def warning: these were caused by restricting the set of allowed indentations for children in the method call handler. I updated this to allow more than it allowed before, so it shouldn't happen anymore.

[romani]

@pietern , please generate reports(before/after) on projects that do not have lambda's (hibernate, jdk7, spring) and confirm that there are no difference in reports.

@mkordas , please confirm that violations for RxJava and protonpack are valid.

I will review 'infinispan'.

As all of us confirm that results are good - I will merge this PR (I can even trigger release 6.11.1 as to speed up testing of this by community).

All false positives in Lambda that we still have should be posted as comments to issue, issue is not going to be closed for now.

[pietern]

@romani Sounds good. Is the broadened set of acceptable indentations acceptable to you? I expect this to be the only material difference in the reports before/after this change, since it applies to all method calls.

[romani]

Please give me example, i do not understand you

[mkordas]

@romani I confirm that both protonpack and RxJava reports look really well. I haven't noticed anything suspicious. All changes are expected and beneficial for the check.

[romani]

I updated my #1548 (comment) with list of false-positives at Lambda support. I did not noticed any other serious changes that could block merge.

@pietern , please confirm your part of validation.

[pietern]

@romani Re: the broadened set of acceptable indentations: that suggestedChildLevel returns more acceptable indentation levels than before. It is not just a single level that it returns.

Some more results for this change:

	before	after
hibernate-orm	68604	68604
spring-framework	18586	18515

The fewer number of warnings in spring is caused by a few of lambdas in the code base that are now correctly checked. I checked the diff for hibernate and the warnings are identical. The only difference is that there is one method call with indented arguments where more indentation levels are acceptable, if you look at the diff of checkstyle.txt for hibernate (per the suggestedChildLevel comment above). I pushed the results for both projects (before and after) to the same repository. The directories contain checkstyle.txt for easy diff'ing (see https://github.com/pietern/checkstyle-reports/blob/gh-pages/gen-txt.sh).

[pietern]

@romani I reviewed the false positives you listed for infinispan, but the first three of them are actually real positives; the code base has mixed use of 6 and 8 characters for line continuation. I ran the test with 6 (given that the basic offset is 3), which explains a few of the false positives for cases where 8 is used.

[romani]

@pietern , please copy paste each false-positive and by trailing comments describe how much spaces code have and what is expected and what is configuration. Any general talk about indentation will never succeeded.

[pietern]

Configuration for infinispan: http://pietern.github.io/checkstyle-reports/lambda-1548/check_infinispan.xml

1. DistributionBulkInterceptor.html#L135 -- indentation is 8, expected: 6. The other method call arguments are not checked at all, only the lambda is checked for correct indentation.

2. AbstractCacheStream.html#L354 -- indentation is 8, expected: 6. Same as (1).

3. DistributedDoubleCacheStream.html#L150 -- indentation is 8, expected: 6. Same as (1).

4. FunctionalListenerAssertions.html#L254 -- agree this is a false positive. This is caused by the behavior expected in InputLambda1.java#L205, where the right paren on the new line triggers an expectation of the child being indented with a line continuation amount (being 6), whereas the indentation here is equal to the basic offset (being 3).

[romani]

merged as FF

[romani]

not finished discussion is moved to #281

[pietern]

Great news, thanks @romani !

[coveralls]

Changes Unknown when pulling 2224d1e on pietern:lambdas into ** on checkstyle:master**.