Improving support for Closure parameter with Indentation rule

[dmurat]

In Spock interaction tests, I stumbled on some indentation warnings. Here is the code fragment with marked relevant lines:

  @SuppressWarnings("Indentation")
  void "query(query, metaData, responseClass) - should delegate to the query gateway"() {
    given:
    def query = "query"
    Map<String, ?> metaData = [:]

    CompletableFuture<String> queryResultCompletableFuture = new CompletableFuture<>()
    queryResultCompletableFuture.complete("query result")

    when:
    queryGatewayAdapter.query(query, metaData, String)

    then:
    1 * queryGatewayMock.query(
        _ as String,
        { def queryMessage ->
          verifyAll {                                 // Indentation warning
            queryMessage instanceof GenericMessage    // Indentation warning
            queryMessage.payload == query             // Indentation warning
            queryMessage.metaData == metaData         // Indentation warning
          }
        },
        _ as ResponseType
    ) >> queryResultCompletableFuture
  }

After some debugging and reading CodeNarc code, I believe I managed to recreate the issue by adding the following test in IndentationRuleTest:

    @Test
    void test_Statement_ClosureAsNonLastParameter_MultipleLines() {
        final SOURCE = '''
            |class MyClass {
            |    void doStuff(String name) {
            |        String otherName = name
            |        doWith(
            |            name,
            |            { String someParam ->
            |                println someParam 
            |            },
            |            otherName
            |        )
            |    }
            |}
        '''.stripMargin()
        assertNoViolations(SOURCE)
    }

I also added some code in IndentationRule class that resolves the issue, but I'm sure there is a better and more idiomatic way to implement it.
Despite, here is my modification of IndentationRule.visitMethodCallExpression():

    @Override
    void visitMethodCallExpression(MethodCallExpression call) {
        // If the method name starts on a different line, then assume it is a chained method call,
        // and any blocks that are arguments should be indented.
        if (isChainedMethodCallOnDifferentLine(call) && call.arguments instanceof ArgumentListExpression) {
            call.arguments.expressions.each { expr ->
                if (expr instanceof ClosureExpression && expr.code instanceof BlockStatement) {
                    blockIndentLevel[expr.code] = blockIndentLevel[expr.code] + 1
                }
            }
        }

        if (call.arguments instanceof ArgumentListExpression && !call.arguments.expressions.isEmpty() && !(call.arguments.expressions.last() instanceof ClosureExpression)) {
            call.arguments.expressions.each { expr ->
                if (expr instanceof ClosureExpression && expr.code instanceof BlockStatement && (expr.lineNumber != expr.lastColumnNumber)) {
                    blockIndentLevel[expr.code] = blockIndentLevel[expr.code] + 1
                }
            }
        }

        super.visitMethodCallExpression(call)
    }

Tnx

[chrismair]

Ugh, the Indentation rule has been quite the pain in the butt. It started out relatively simple, just trying to go after the low-hanging fruit -- the easy stuff supported by the Groovy AST. But one special case after another has added a bunch of complexity and brittleness.

Thanks for providing the example code that caused the false positive, as well as the possible code fix. That fix did address that one particular example, but not several other related ones that I tried. I ended up adding some more flexibility to the rule so that method call parameters that are Closures with blocks can have those statements within a range of indent levels. We'll see how that holds up to the brutal realities of widespread use.