-
Notifications
You must be signed in to change notification settings - Fork 0
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
Comments
I incline there might be a simpler compiler plugin that inserts expression texts for a given argument with a custom annotation. 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 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) |
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
But I think it clutters too much the API for what it is worth it. |
WDYT of |
I am not sure yet if it will work out as expected. I used to have a lot of ambiguity bugs in conjunction with |
Not yet.
I guess K2 compiler is at the top of the priority queue (~ https://blog.jetbrains.com/kotlin/2023/02/k2-kotlin-2-0/) |
@robstoll , look what I got: https://github.com/vlsi/kotlin-argument-expression It is not entirely usable yet as I need to add a Gradle plugin for the configuration and publish it to Central. Here's the IR transformer: https://github.com/vlsi/kotlin-argument-expression/blob/main/argument-expression-plugin/src/main/kotlin/io/github/vlsi/ae/ArgumentExpressionCallTransformer.kt Here are the tests: https://github.com/vlsi/kotlin-argument-expression/blob/main/argument-expression-plugin/src/test/kotlin/io/github/vlsi/ae/ScalarExpressionTest.kt |
nice 👍 how about you turn it into a string provider 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). 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
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 |
A compiler plugin can divert call, so However, I would try 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. |
Why using a provider? |
you're right depends on the use case. I was thinking about your betterRequire and fun in general which are 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 |
I do not see the use case though yet. I've drafted a test case with lambdas: My findings so far are:
I would probably release the plugin shortly so we can try it in a bigger codebase.
I tried downgrading Kotlin version to 1.4.30, and the tests still pass. |
I suggest moving the discussion to |
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: 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) ?: ""
)
}
)
)
}
} |
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. |
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"
} |
Do you know by chance if named arguments are included in the source string? |
Would you rephrase? |
sorry, I meant: if one writes |
|
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 |
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 |
we can maybe work around it by providing a second overload of expect:
But I would need to try it out if it works in all situations. |
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. |
true, forgot about the assertionCreator lambda. That's a pity but good to know |
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
And in reporting I would like to show a clickable link which points to the place where the expectation function was called.
I would make it configurable whether we show only the place where the |
A different data type would indeed resolve the ambiguity. I'm not sure regarding
However, it would be information for a single |
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. |
I understand what you mean there and why you want that.
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).
Well, if having a location for If Imagine: expect(user).toStringifyAs("User(name=Joe, age=42)") Suppose Of course, there might be issues with non-trivial cases like |
Atrium won't need this so maybe easier to start without it and reconsider once someone has an actual need for it.
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.
Could be interesting to explore in the future |
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:
And we transform it to:
We could still improve methodCall formatting with #41
However, a compiler plugin has also drawbacks/risks:
The text was updated successfully, but these errors were encountered: