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

introduce a compiler plugin #74

Open
robstoll opened this issue Mar 19, 2020 · 29 comments
Open

introduce a compiler plugin #74

robstoll opened this issue Mar 19, 2020 · 29 comments
Labels
do-it it was decided that this issue (or part of it) shall be done

Comments

@robstoll
Copy link
Owner

to circumvent the type inference bugs Kotlin still has (and might still have after 1.4) we could consider to introduce a compiler plugin.
It's not the first time I am thinking about it but came across the following lately:
https://medium.com/@bnorm/exploring-kotlin-ir-bed8df167c23

So it seems that the compiler plugin allows to transform code as we would like to be able to. i.e. that one can write:

expect(person) {
  feature { name }.toBe("Felix")
  feature { children.first { age < 18 }.name }.toBe("Laura")
}

And we transform it to:

expect(person) {
  feature("name") { name }.toBe("Felix")
  feature("children.first { age < 18 }.name") { children.first { age < 18 }.name }.toBe("Laura")
}

We could still improve methodCall formatting with #41

However, a compiler plugin has also drawbacks/risks:

  • something to maintain in addition, especially if there are changes between intellij versions it means that we need to maintain multiple versions
  • Kotlin's IR is not stable yet
  • the user has to install the plugin in addition
  • I am not sure if it works for JS
@vlsi
Copy link
Collaborator

vlsi commented Feb 20, 2023

I incline there might be a simpler compiler plugin that inserts expression texts for a given argument with a custom annotation.
See https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-10.0/caller-argument-expression

Imagine something like

fun <T, R> Expect<T>.feature(
    @ExpressionText("provider") description: String? = null,
    provider: T.() -> R,
    assertionCreator: Expect<R>.() -> Unit
): Expect<T> = ...

Then the plugin would insert the body of provider argument into the description argument.
Unfortunately, I do not see how it can be implemented with KSP since KSP can't edit code. It can generate new code (including extensions), however, what we need here is to patch the call site.

That, however, won't automatically cover scenarios like "method call formatter" :-/

For function calls there might be something like

fun <T> T.logit(
    @ExpressionText("this") description: String? = null
): T = this.also { TODO("log and attach $description=$it") }

fun <T> logit(
    value: T,
    @ExpressionText("value") description: String? = null
): T = value.also { TODO("log and attach $description=$it") }

Then the usage could be like

expect(person) {
  feature { children.logit().first { age < 18 }.name }.toBe("Laura")
}

expect(myClass)
  .feature { calculateXy(logit(1 + z), getYvalue, 2.0) }
  .toBe(283.5)

@robstoll
Copy link
Owner Author

I also don't see a way to patch the call site via KSP. I was thinking about generating one overload per expectation function via KSP where the overload expects something like Named<T> which would then look as follows:

expect(named("my variable", $myVar)).toEqual(named("foo bar", $fooBar))

But I think it clutters too much the API for what it is worth it.

@vlsi
Copy link
Collaborator

vlsi commented Feb 20, 2023

WDYT of @ExpressionText("value") description: String? = null approach?

@robstoll
Copy link
Owner Author

I am not sure yet if it will work out as expected. I used to have a lot of ambiguity bugs in conjunction with feature and default arguments can make it even worse. Either way, I don't have the time right now to look into but I keep your idea into my mind once I play around with the idea of a compiler plugin. Have you already created a compiler plugin for Kotlin? If yes, do you by chance know if the already have a plan in which version the IR will become stable (or is it already by now?)

@vlsi
Copy link
Collaborator

vlsi commented Feb 20, 2023

Have you already created a compiler plugin for Kotlin?

Not yet.
However, I think either reusing kotlin-power-assert, or contributing there might work.
For instance, there might be value in adding @ExpressionText as a standalone compiler plugin.

If yes, do you by chance know if the already have a plan in which version the IR will become stable (or is it already by now?)

I guess K2 compiler is at the top of the priority queue (~ https://blog.jetbrains.com/kotlin/2023/02/k2-kotlin-2-0/)

@vlsi
Copy link
Collaborator

vlsi commented Feb 24, 2023

@robstoll
Copy link
Owner Author

nice 👍 how about you turn it into a string provider () -> String.

I am not yet sure if the approach of an additional param is the right one for Atrium. it's nice as it also works without intellij plugin and even if one has not installed the compiler plugin (in the sense of, the you get the fallback mechanism).
However, the additional parameter has also its limitations. for instance fun with vararg. we can work around it by providing the possibility to collect all arguments into a string array/iterable. I guess this should work somehow. but I am not yet sure if we don't run into kotlin ambiguity bugs again. we would need to try it out.

I wonder if you can implement your own type checking in a compiler plugin. if that would be possible, then maybe an approach simulating union types could be a better fit. something like

@Union(T.() -> R, Named<T.() -> R>)
typealias FeatureExtractor<T, R> = Any

fun <T, R> Expect<T>.its(extractor: FeatureExtractor<T, R>) = ...

and then we can rewrite the function (futing the compilation of Atrium) to include the type checks (unchecked but better than nothing) for the case that a one has not installed the compiler plugin and rewrite the call site to Named. but I am not sure if that would work out without an additional intellij plugin, in the sense of, would code completion still work if one writes its { /* show me the functions intellij */ }. if not and it requires an intellij plugin, then I think this approach is too much a burden regarding maintainability.

@vlsi
Copy link
Collaborator

vlsi commented Feb 24, 2023

A compiler plugin can divert call, so its { name } could become compiled as itsNamed(...)

However, I would try .its(@ArgumentExpression("extractor") description: String = "", extractor: T.() -> R) tomorrow.
I guess it might be good enough.

It sounds interesting, to add something like

@ReplaceWith("itsNamed")
fun <T, R> Expect<T>.its(extractor: T.() -> R) = ...

fun <T, R> Expect<T>.itsNamed(@ArgumentExpression("extractor") description: String, extractor: T.() -> R) = ...

Then the API would still be usable without a compiler plugin, and one doesn't run into overload ambiguity issues.

@vlsi
Copy link
Collaborator

vlsi commented Feb 24, 2023

how about you turn it into a string provider () -> String

Why using a provider?
The expression is known at the compile time, so creating a provider would create extra overhead

@robstoll
Copy link
Owner Author

you're right depends on the use case. I was thinking about your betterRequire and fun in general which are inlined. then the provider is never created and the string instantiation only happens if necessary.

I would prefer only one overload if possible. or in other words your argument approach is my favourite if it works for vararg and we don't run into ambiguity issues where there is no workaround. I hope Kotlin 1.4 or 1.5 has improved in this area. At least in a way that the user can disambiguate the problem. In the worst case we require kotlin 1.6 or even 1.7
But I would prefer if we can support 1.4.
that leads me to another question, does your compiler plugin work with 1.4? not that this is a must, I think it is fair enough to say the user requires a higher version if he wants to use the plugin.

@vlsi
Copy link
Collaborator

vlsi commented Feb 25, 2023

you're right depends on the use case. I was thinking about your betterRequire and fun in general which are inlined. then the provider is never created and the string instantiation only happens if necessary.

I do not see the use case though yet.
Well, for now, I try the most trivial integration: user adds String parameter manually, and the compiler plugin passes a string that will anyway be known at the compilation time.
So there's not much sense in passing the same string literal via lambda.

I've drafted a test case with lambdas:

https://github.com/vlsi/kotlin-argument-expression/blob/2a1cc82e61640e3c7e9a635f9861b7ab8202f1bf/argument-expression-plugin/src/test/kotlin/io/github/vlsi/ae/ScalarExpressionTest.kt#L80-L93

My findings so far are:

  1. There might be a need to trim { and } from the lambda expressions
  2. When description: String = "" is the last argument, one can't use operator invoke to create builder-like DSL. In my example I had to integrate expectations: Expect<Value>.() -> Unit = {} into expectations block itself to allow usage like expect(abc) { ... }

I would probably release the plugin shortly so we can try it in a bigger codebase.


does your compiler plugin work with 1.4?

I tried downgrading Kotlin version to 1.4.30, and the tests still pass.

@vlsi
Copy link
Collaborator

vlsi commented Feb 25, 2023

I wonder if you can implement your own type checking in a compiler plugin. if that would be possible, then maybe an approach simulating union types could be a better fit. something like
@Union(T.() -> R, Named<T.() -> R>)
typealias FeatureExtractor<T, R> = Any

I suggest moving the discussion to

@vlsi
Copy link
Collaborator

vlsi commented Feb 26, 2023

compiler plugin work with 1.4?

Is it really important to support versions before 1.7?

I tried building the compiler plugin with Kotlin 1.4 (previously it was silently upgraded to 1.7), and the outcome is:
a) There are less APIs to use.
b) Kotlin 1.4.32 does not seem to execute compiler plugins. In other words, if I run with Kotlin 1.4.32, then the plugin does not seem to be activated
c) Kotlin 1.5.32..1.6.10 do not seem to expose annotations via IR's parameter.annotations. The annotations are available via deprecated ObsoleteDescriptorBasedAPI
d) It becomes harder to test the plugin as the recent com.github.tschuchortdev:kotlin-compile-testing pull Kotlin 1.7 for testing
e) There are less APIs to use.
For instance, the following createIrBuilder, irCall, irString, irBuiltIns.arrayOf, irVararg are not available with 1.4.30:

context.irBuiltIns.createIrBuilder(currentScope!!.scope.scopeOwnerSymbol)
    .run {
        irCall(context.irBuiltIns.arrayOf).apply {
            putValueArgument(
                0,
                irVararg(
                    context.irBuiltIns.stringType,
                    array.elements
                        .map {
                            irString(
                                sourceFile.expressionTextOrNull(it) ?: ""
                            )
                        }
                )
            )
        }
    }

@robstoll
Copy link
Owner Author

Just in the sense that it works more or less without ambiguity issues. IMO it's totally fine to say Atrium requires Kotlin 1.4 and requires 1.7 in case you want to use the compiler plugin.

@vlsi
Copy link
Collaborator

vlsi commented Feb 27, 2023

irbuilder APIs are available in Kotlin 1.6.21+, so I was able to support Kotlin 1.6.21+, and Gradle 6.1.1+ with minimal code changes.

I've published the plugin to Central (it will appear on Gradle Plugin Portal as they review it), so you can already try it via

plugins {
    id("io.github.vlsi.kotlin-argument-expression") version "1.0.0"
}

@robstoll
Copy link
Owner Author

Do you know by chance if named arguments are included in the source string?

@vlsi
Copy link
Collaborator

vlsi commented Feb 27, 2023

Would you rephrase?
For now, I include only expression texts, not the names of the arguments.

@robstoll
Copy link
Owner Author

sorry, I meant: if one writes foo(factor = 2) is factor = included in the description extracted via CallerArgumentExpression?

@vlsi
Copy link
Collaborator

vlsi commented Feb 27, 2023

Named(value = 2 + 3).toString() => Named(value=5, name='2 + 3'), so the extracted expression is the same for both named and positional arguments

@robstoll robstoll added the do-it it was decided that this issue (or part of it) shall be done label Feb 27, 2023
@robstoll
Copy link
Owner Author

Good, means less work in Atrium as I would have filtered it out. I guess I won't have time to look into the ambiguity topic until mid/end of march. In case you have time, the most problems I had in the area of feature but this one we don't need to change IMO (I would only add it to its). The other use case was Expect<Iterable>.toContain.inAny.order.values(..., report={})

@vlsi
Copy link
Collaborator

vlsi commented Mar 15, 2023

Here's an edge case:

fun <T, R> Expect<T>.expect(
    newSubject: R,
    @CallerArgumentExpression("newSubject") description: String = ""
): FeatureExpect<T, R> = ...

expect { 1/0 }.toThrow { ... }

Kotlin assumes the lambda must be the last argument, so it does not compile expect { 1/0 }.
Apparently, expect({ 1/0 }) compiles, however, it is a case when CallerArgumentExpression stands in the way.

@robstoll
Copy link
Owner Author

we can maybe work around it by providing a second overload of expect:

fun <R> expect(@CallerArgumentExpression("newSubject") description: String = "", newSubject: () -> R): Expect<() -> R> = ...

But I would need to try it out if it works in all situations.

@vlsi
Copy link
Collaborator

vlsi commented Mar 16, 2023

It creates ambiguity in case like

expect("hello") { // <-- ambiguity here
    toEqual("world")
}
// see

    fun <T, R> Expect<T>.expect(
        newSubject: R,
        @CallerArgumentExpression("newSubject") description: String = "",
        assertionCreator: Expect<R>.() -> Unit
    ): Expect<R> =

    fun <T, R> Expect<T>.expect(
        @CallerArgumentExpression("newSubject") description: String = "",
        newSubject: () -> R
    ): Expect<() -> R> =

It looks like "redirect call" plugin is needed to properly handle this.

@robstoll
Copy link
Owner Author

true, forgot about the assertionCreator lambda. That's a pity but good to know

@robstoll
Copy link
Owner Author

We could workaround the ambiguity issue by introducing an own type. For instance:

data class ExpectationTrace(val qualifiedName: String, val fileName: String, val lineNumber: Int)
data class CallerInfo(
    val source: String,
    val expectationTrace: ExpectationTrace? = null
)

where we could use the expecationTrace to show where the expectation was made as shown here: robstoll/atrium#805 (comment)

Then we don't need an extra overload any more and the following two should be enough

fun <T> expect(subject: T, @CallerArgumentExpression("subject") callerInfo: CallerInfo? = null): RootExpect<T>

fun <T> expect(
    subject: T,  
    @CallerArgumentExpression("subject") callerInfo: CallerInfo? = null, 
    assertionCreator: Expect<T>.() -> Unit
): Expect<T>

You might wonder why I want a previousCallerInfo and what it contains. I would imagine the following. Say you use Atrium and have written the custom expectation function fun Expect<Int>.toBeFoo() = .... which itself makes use of several pre-defined expectation functions. In your test you then use it:

expect(result).toBeFoo()

And in reporting I would like to show a clickable link which points to the place where the expectation function was called.

I expected the subject: "lorem ipsum...."           🔎 com.example.MyTest.methodXy(MyTest.kt45)
* to start with:  "asdf"                            🔎 com.example.atrium.foo(AtriumExtensions.kt:26)
* to contain: "foo"                                 🔎 com.example.atrium.foo(AtriumExtensions.kt:27)
* > length: 3874
   * to be less than: 1000                          🔎 com.example.atrium.foo(AtriumExtensions.kt:28)      

I would make it configurable whether we show only the place where the expect was defined or all places.

@vlsi
Copy link
Collaborator

vlsi commented Mar 22, 2023

A different data type would indeed resolve the ambiguity. I'm not sure regarding ExpectationTrace inside CallerInfo though.

@CallerArgumentExpression("subject") callerInfo: CallerInfo? = null, could mean like "pass information on subject via this CallerInfo argument".
Then CallerInfo could capture "argument expression":

  • it should somehow accommodate for both String and List<String>
  • file path (relative to project root? relative to module root?)
  • file name
  • start, and end offsets (or line numbers+positions)

However, it would be information for a single subject argument, and it does not really specify where expect is located, so it looks odd naming it with ExpectationTrace.

@robstoll
Copy link
Owner Author

I'm not sure regarding ExpectationTrace inside CallerInfo though.

That was just out of the blue and would certainly not make a lot of sense in a more general context as your compile plugin. There it could maybe be named CallDiagnostic or the like. com.example.MyTest.methodXy(MyTest.kt45) is rendered as link by Intellij. That's why I came up with qualifiedName. Maybe it should be split into qualifiedClassName and methodName. fileName would just be the name but you could store the absolute path and the library which is using it can shorten, make it relative etc.
Intellij jumps to the correct line if you provide it as shown but maybe it would even jump to the position if provided in addition. Something to figure out.

@vlsi
Copy link
Collaborator

vlsi commented Mar 22, 2023

com.example.MyTest.methodXy(MyTest.kt45) is rendered as link by Intellij. That's why I came up with qualifiedName

I understand what you mean there and why you want that.

fileName would just be the name but you could store the absolute path and the library which is using it can shorten, make it relative etc.

Having the path and file name separate would help in reusing the strings (the string with the file name could be reused for all the expectations within the file).
Then, having a full path might unintentionally leak file layout.
Of course, it might be a non-issue for test code, however, imagine you use it in Atrium code for between() { lessThan(); greaterThan() }. Apparently, lessThan would hard-code the absolute path on the machine you build Atrium. That is of little use to the consumers. It won't hurt that much, however, I would refrain from leaking filesystem layouts by default.

That was just out of the blue and would certainly not make a lot of sense in a more general context as your compile plugin

Well, if having a location for subject (or other arguments) is enough, then it is fine. I just thought there might be a need for "location of expect call".

If CallerInfo (or ArgumentInfo or whatever) exposes file and positions, then Atrium can even help with snapshot testing.

Imagine:

expect(user).toStringifyAs("User(name=Joe, age=42)")

Suppose User#toString changes. With current setup, developer have to manually update the tests.
However, if we add CallerInfo to expectation values, Atrium could deduce the location of "User(name=Joe, age=42)", and it could patch the source code, so the expected value is updated after test execution.

Of course, there might be issues with non-trivial cases like between, or contains, however, the most common case of toEqual(...), toStringifyAs(..) could be supported.

@robstoll
Copy link
Owner Author

Then, having a full path might unintentionally leak file layout.

Atrium won't need this so maybe easier to start without it and reconsider once someone has an actual need for it.
Also, I am not sure intellij will print a link for locations in jars, that's something we could try out in addition

Well, if having a location for subject (or other arguments) is enough, then it is fine. I just thought there might be a need for "location of expect call".

Sorry, because the location of subject and expect are most of the time on the same line, I interchanged them. But you are of course right, I meant the location of the subject.

Atrium can even help with snapshot testing

Could be interesting to explore in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-it it was decided that this issue (or part of it) shall be done
Projects
None yet
Development

No branches or pull requests

2 participants