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

[Kotlin] Returning a lambda with multiline code creates syntax errors #656

Open
K1rakishou opened this issue Apr 3, 2024 · 7 comments
Open

Comments

@K1rakishou
Copy link

In our project we are using a quite unorthodox way to check whether experiments are enabled/disabled. We have a special class, let's name it ExperimentChecker, which has lots of different methods, from simple ones like isOn()/isOff() to some advanced ones.

Consider the following code:

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return expChecker.check(Experiments.ExpName) {
            off { 
               logger.log("off branch called")
               SomeValue.OldValue
            }
            on {
               logger.log("on branch called")
               SomeValue.NewValue
            }
        }
    }
}

If we try to match and remove one of the branches along with expChecker.check(Experiments.ExpName) invocation we will end up with the following code (let's remove the off branch in this example):

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return logger.log("on branch")
               SomeValue.NewValue
    }
}

As you can see it introduces a syntax error which then crashes piranha.

I'm not sure if this is even supported by piranha. If it's not then we will have to figure it out on our own. But if it is supported then please could you point me to an example on how it should be handled? Thanks.

@ketkarameya
Copy link
Collaborator

is the expected code (we want to retain the braces right?)

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return expChecker.check(Experiments.ExpName) {
             {
               logger.log("on branch called")
               SomeValue.NewValue
            }
        }
    }
}

I am assuming you wrote a feature flag api cleanup rule specifically for your custom API, which produced the above change, where the off branch is elided. Could you share that with me?

@K1rakishou
Copy link
Author

Sorry forgot to mention what it's expected to look like:

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        logger.log("on branch called")
        return SomeValue.NewValue
    }
}

Basically the code is extracted from the on lambda and then inserted in-place. The braces are to be removed too because they are not just braces in Kotlin but a lambda function.

But it doesn't work because of return. It will also probably not work if the result of that expression were to be assigned to a variable and probably many other cases that I can't think of right now.

I am assuming you wrote a feature flag api cleanup rule specifically for your custom API, which produced the above change, where the off branch is elided.

Yes, we wrote a custom rule. Here is the full rule.

[[rules]]
name = "expchecker_check"
query = """(
(call_expression
    (call_expression
        (navigation_expression
            (simple_identifier) @expchecker_field
            (navigation_suffix (simple_identifier) @type)
        )
        (call_suffix
            (type_arguments
                (type_projection[
                    (user_type (_)
                    )
                ])
            )?
            (value_arguments
                (value_argument[
                    (navigation_expression(_) (navigation_suffix (simple_identifier) @experimentName)
                    )]
                )
            )
        )
    )
    (call_suffix ( annotated_lambda
        (lambda_literal [
                (statements(call_expression
                    (simple_identifier) @expchecker_block
                    (call_suffix (annotated_lambda
                            (lambda_literal[
                                (statements)@takenNode
                            ]))
                    ))
                )
        ]))
    )
)@ReplacedNode

(#eq? @expchecker_field "expChecker")
(#eq? @experimentName "@experiment_name")
(#eq? @expchecker_block "@taken_expchecker_lambda_name")
(#eq? @type "check")
)"""
replace_node = "ReplacedNode"
replace = "@takenNode"
holes=["taken_expchecker_lambda_name", "experiment_name"]

And then in substitutions taken_expchecker_lambda_name is going to be assigned on and experiment_name is going to be assigned ExpName,

For now, my idea to fix this is to wrap multi line code into a Kotlin's run function (think of it as of a lambda function which is invoked right away and is also then inlined directly into the JVM bytecode by Kotlin compiler). So it will look like this:

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return run {
            logger.log("on branch")
            SomeValue.NewValue
        }
    }
}

Not sure how to do this right now, just an idea in case there is no simple solution.

@ketkarameya
Copy link
Collaborator

sorry! This issue completely slipped off my mind.

(call_expression
    (call_expression
        (navigation_expression
            (simple_identifier) @expchecker_field
            (navigation_suffix (simple_identifier) @type)
        )
        (call_suffix
            (type_arguments
                (type_projection[
                    (user_type (_)
                    )
                ])
            )?
            (value_arguments
                (value_argument[
                    (navigation_expression(_) (navigation_suffix (simple_identifier) @experimentName)
                    )]
                )
            )
        )
    )
    (call_suffix ( annotated_lambda
        (lambda_literal [
                (statements(call_expression
                    (simple_identifier) @expchecker_block
                    (call_suffix (annotated_lambda
                            (lambda_literal[
                                (statements)
                            ])@takenNode )
                    ))
                )
        ]))
    )
)@ReplacedNode
(#eq? @expchecker_field "expChecker")
(#eq? @experimentName "@experiment_name")
(#eq? @expchecker_block "@taken_expchecker_lambda_name")
(#eq? @type "check")
)```

^ This query shud work ( I tried on ts-kt)

@ketkarameya
Copy link
Collaborator

is ur codebase open source?

@K1rakishou
Copy link
Author

No, it's not open-source.

So the above query removes everything except for the lambda body now. So instead of this code:

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return logger.log("on branch")
               SomeValue.NewValue
    }
}

It now produces this code:

class SomeClass(
    private val expChecker: ExpChecker
) {
    fun someFunc(): SomeValue {
        return {
               logger.log("on branch")
               SomeValue.NewValue
        }
    }
}

Which might work in some languages, but not in Kotlin, because in Kotlin this block:

return {
   logger.log("on branch")
   SomeValue.NewValue
}

basically means return a lambda. Which in this case is a compilation error. It's possible to fix this by adding () at the end (invoke lambda right away), like so:

return {
       logger.log("on branch")
       SomeValue.NewValue
}()

But I don't know how to insert new text into the code using piranha rules. Is this possible? If that is possible then I can figure out the rest.

@ketkarameya
Copy link
Collaborator

yes inserting text is possible as an "update" operation.
so wait, do u want :

return {
       logger.log("on branch")
       SomeValue.NewValue
}()

or

 return run {
            logger.log("on branch")
            SomeValue.NewValue
        }

Either shud be possible

u can update ur replace to replace = run @takenNode ? or replace = @takenNode()?

@K1rakishou
Copy link
Author

Yes, this works. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants