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.

Sign up for GitHub

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

Jump to bottom

Enable builder inference for optics copy #2792

Merged
merged 4 commits into from Aug 9, 2022
Merged

Enable builder inference for optics copy #2792

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bc2930
Enable builder inference for optics `copy`
serras Aug 7, 2022
1ee8e72
Delete opt-in from the list of compiler options
serras Aug 8, 2022
9a28899
Remove all compiler args
serras Aug 8, 2022
faf725f
Merge branch 'main' into optics-copy-builder-inference
nomisRev Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions arrow-libs/optics/arrow-optics/api/arrow-optics.api
View file
Open in desktop
Expand Up @@ -151,6 +151,9 @@ public final class arrow/optics/ListKt {
public static final fun unsnoc (Ljava/util/List;)Lkotlin/Pair;
}

public abstract interface annotation class arrow/optics/OpticsCopyMarker : java/lang/annotation/Annotation {
}

public final class arrow/optics/OptionalGetterKt {
public static final fun OptionalGetter (Lkotlin/jvm/functions/Function1;)Larrow/optics/POptionalGetter;
}
Expand Down
4 changes: 4 additions & 0 deletions arrow-libs/optics/arrow-optics/build.gradle.kts
View file
Open in desktop
Expand Up @@ -20,6 +20,10 @@ if (enableCompatibilityMetadataVariant) {
}
}

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
kotlinOptions.freeCompilerArgs += listOf("-Xenable-builder-inference", "-opt-in=kotlin.RequiresOptIn")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are both these flags needed?

First one is to allow builder inference inside Arrow, or to be able to expose annotation?
Second one is to allow OptIn without a compiler warning?

I’m always confused about the OptIn, because it works fine without this flag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, the docs at https://kotlinlang.org/docs/using-builders-with-builder-inference.html#writing-your-own-builders are by no means clear. But I'll delete the opt-in one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'll delete the opt-in one.

I'm not sure if we need it yes, or no 😅 Just wanted to clarify, it can be that we're currently missing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said "are by no means clear" :/ I feel the same for the opt-in one.

}

kotlin {
sourceSets {
commonMain {
Expand Down
12 changes: 10 additions & 2 deletions arrow-libs/optics/arrow-optics/src/commonMain/kotlin/arrow/optics/Copy.kt
View file
Open in desktop
@@ -1,5 +1,11 @@
package arrow.optics

import kotlin.experimental.ExperimentalTypeInference

@DslMarker
public annotation class OpticsCopyMarker

@OpticsCopyMarker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do on the interface? I thought it was supposed to go on the functions, the documentation didn’t clarify it for me 😅
Since it’s applicable to interface, I’m wondering what it does.

Is it applied to all DSL functions that have this as a lambda receiver?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand from https://kotlinlang.org/docs/type-safe-builders.html#scope-control-dslmarker, you put the annotation in the interface which will be in scope in the builder, which in this case is Copy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand the @DslMarker is the thing that marks the member of that interface as "keywords", whereas @BuilderInference changes how type checking works to better suit this kind of pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I asked because I used @DslMarker differently in the Arrow 2.0 branch and it worked for me in IDEA 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the docs (emphasis mine):

To make the compiler start controlling scopes you only have to annotate the types of all receivers used in the DSL with the same marker annotation. For instance, for HTML Builders you declare an annotation @HTMLTagMarker:

@DslMarker
annotation class HtmlTagMarker

An annotation class is called a DSL marker if it is annotated with the @DslMarker annotation.

[...]

After you've added this annotation, the Kotlin compiler knows which implicit receivers are part of the same DSL and allows to call members of the nearest receivers only.

public interface Copy<A> {
/**
* Changes the value of the element(s) pointed by the [Setter].
Expand Down Expand Up @@ -33,7 +39,8 @@ public interface Copy<A> {
* }
* ```
*/
public fun <B> inside(field: Traversal<A, B>, f: Copy<B>.() -> Unit): Unit =
@OptIn(ExperimentalTypeInference::class)
public fun <B> inside(field: Traversal<A, B>, @BuilderInference f: Copy<B>.() -> Unit): Unit =
field.transform { it.copy(f) }
}

Expand Down Expand Up @@ -67,5 +74,6 @@ private class CopyImpl<A>(var current: A): Copy<A> {
* }
* ```
*/
public fun <A> A.copy(f: Copy<A>.() -> Unit): A =
@OptIn(ExperimentalTypeInference::class)
public fun <A> A.copy(@BuilderInference f: Copy<A>.() -> Unit): A =
CopyImpl(this).also(f).current