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

A proposal for the assign operator overload #309

Closed
elizarov opened this issue Jun 29, 2022 · 23 comments
Closed

A proposal for the assign operator overload #309

elizarov opened this issue Jun 29, 2022 · 23 comments

Comments

@elizarov
Copy link
Contributor

elizarov commented Jun 29, 2022

This KEEP is a proposal on providing an assign operator overload that would provide DSL for assigning values to mutable container objects. For a full text of proposal see assign-operator-overload.md in the corresponding PR. Please, use this issue for discussion.

@fvasco
Copy link

fvasco commented Jul 4, 2022

This KEEP looks like a rewriting of the delegation, so having two features to solve the same tasks can lead to confusion.

My main unanswered question is: is this solution clearly better than delegation?

Does this code have a meaning, or is it an abuse?

val myOptional = MyOptional()
myOptional = null
if (myOptional == null) println("myOptional is empty")

Should this code compile?

val a = StringProperty() // unmodifiable variable
a = 3 // a.assing(3)

and this not?

var a = StringProperty() // modifiable variable
a = 3 // error

It is unclear to me if there is an interference between this KEEP and (Mutable)Collection.

Should this code compile?

val a = listOf(StringProperty())
a[0] = "a" // a.get(0).assign("a")

or this?

val a = mutableListOf(StringProperty())
a[0] = "a" // a.set(0, "a")

@edrd-f
Copy link

edrd-f commented Jul 4, 2022

Were union types considered as a possible solution? i.e.:

open class MyTask {
  var input: Property<File> = Property<File>()
    set(value: Property<File> | File | Path) {
      field = when (value) {
        is File -> Property(value)
        is Path -> Property(value.toFile())
        is Property<*> -> value
      }
    }
  }
}

(assuming setters were allowed to receive parameters with types different than the field type)

Of course this would be a much bigger effort, but that's a feature that solves lots of use-cases.

@asodja
Copy link

asodja commented Jul 8, 2022

Thanks for your interest in that feature. I will try to answer some questions.

My main unanswered question is: is this solution clearly better than delegation?

Delegation was considered, it's discussed here. In short, the main problem with delegation is, that you can't use = to assign to different types. And if we would want to assign to different types, we would have to implement overloadable setters as discussed here, but that is difficult to implement.

Were union types considered as a possible solution?

They were considered. Unfortunately, this is a really big feature and as I understand it's still far away. It's also somehow related to that topic discussed here. So in short: it's possible, it could solve many issues, but it requires a lot of effort and it needs a lot of design since it affects all parts of the language.


val myOptional = MyOptional()
myOptional = null
if (myOptional == null) println("myOptional is empty")

This can have some meaning, but statement if (myOptional == null) will always be false, since val is final and the reference is never changed. And myOptional = null would work if you would have for example operator fun MyOptional.assign(value: Any?).


val a = StringProperty() // unmodifiable variable
a = 3 // a.assing(3)

This compiles if you have operator fun StringProperty.assign(value: Int).

var a = StringProperty() // modifiable variable
a = 3 // error

This doesn't, var doesn't work with assign operator.


val a = listOf(StringProperty())
a[0] = "a" // a.get(0).assign("a")

This would work the same as now, so it doesn't compile since a[0] = "a" is translated to a.set(0, "a") and not to a.get(0).assign("a")

val a = mutableListOf(StringProperty())
a[0] = "a" // a.set(0, "a")

This would work the same as now for the same reason mentioned above.

@fvasco
Copy link

fvasco commented Jul 9, 2022

Thank you @asodja for pointing to documentation, I already seen that.

In short, the main problem with delegation is, that you can't use = to assign to different types

If I understand well, then the goal should be to reduce "boilerplate", i.e.:

input2 bind calculatedProperty
input2 = calculatedProperty

In my mind, these lines of code do different operations, so it is not easy for me to understand why both should use the same syntax.
Using the = operator doesn't look considerably shorter for me, nor more readable than bind, neither it doesn't seem to reduce the error or misunderstanding.

In the case:

val input: Property<Property<*>>
val calculatedProperty: Property<Property<*>>

The result of the statement:

input = calculatedProperty

is a puzzle for me.
So I am prone to exclude that this proposal reduces the boilerplate or makes the code more readable.

This can have some meaning, but statement if (myOptional == null) will always be false, since val is final and the reference is never changed

Yes, I supposed that, but my question was: Does this code have a meaning, or is it an abuse?

This doesn't, var doesn't work with assign operator.

I agree, this KEEP makes a switch from val to var a source incompatible change, this is an expected behaviour?
Personally, I don't consider this a nice to have.

This would work the same as now

This doesn't looks so much POLA for me.

I feel that this feature is too difficult to teach, the meaning of = depends from different factors and it impacts differently on other language features.
The supposed advantages looks really far to gain +100 points - for me.
Maybe, I don't see any real advantage on writing stringProperty = 42 instead of stringProperty = "42".

Similarly, Scala proposed the Implicit Conversion feature, more coherent to the language design. I am not suggesting to implement that, it shares some issues with this proposal without adding significant advantages.

@JavierSegoviaCordoba
Copy link

@asodja this part (if (myOptional == null) will always be false) can't be a problem?, it is usual in Gradle to check properties in conditionals.

@edrd-f
Copy link

edrd-f commented Jul 9, 2022

Were union types considered as a possible solution?

They were considered. Unfortunately, this is a really big feature and as I understand it's still far away. It's also somehow related to that topic discussed here. So in short: it's possible, it could solve many issues, but it requires a lot of effort and it needs a lot of design since it affects all parts of the language.

What if... just like with definitely non-nullable types, a limited version of union types was implemented?

I'd much prefer something that's limited but can be expanded consistently in the future instead of a low-effort solution that will introduce more implicit behavior and be redundant once the language evolves to have things such as union types or setter overloads.

Also, implicitness is not the only problem here. The examples given by @fvasco show how inconsistent some things would look. It gives me chills to think it would be possible to reassign val.

@edrd-f
Copy link

edrd-f commented Jul 10, 2022

Some questions:

  1. Will the assignment overload work for literals?
operator fun Any?.assign(other: Any?) {}

fun main() {
  "x" = 1 // Will this compile?
}
  1. Will overloaded assignments become expressions? Since assignment overloads are resolved to functions and functions can return values, it looks like this would be possible:
operator fun Int.assign(other: Int) = true 

fun main() {
  if (1 = 1) println("Wat")
}
  1. If answer to question 2 is "yes, overloaded assignments become expressions", how can we avoid the common "assignment by mistake" issue? e.g.:
build.gradle.kts
// assume isDevEnv is nullable and thus we cannot simply do `if (application.isDevEnv)`
if (application.isDevEnv = true) { // oops, meant to be `==` but it compiles
  doSomeAdditionalStuff()
}

@fvasco
Copy link

fvasco commented Jul 10, 2022

  1. Will the assignment overload work for literals?

@edrd-f, the proposed operator overloading depends on the declaration site, so both "x" = 1 and System.currentTimeMillis() = 1 should be prohibited.

I suppose that the overload is applied for val but not for const val, so this code should not work:

operator fun Any?.assign(other: Any?) {}

const val STRING = "string"

fun main() {
  STRING = "strange"
}

Function's parameters are not covered, so this code should not work:

fun foo(property: StringProperty) {
   property = 3
}
  1. Will overloaded assignments become expressions?

It looks no, the signature of Java class is void assign(...), so 1 = 1 may return Unit or may throw an exception.

@hoc081098
Copy link

IMO, This proposal adds complexities and causes many problems :))

@Starlight220
Copy link

The way I see it, this adds more complexities and problems than the benefit gained. The only advantage mentioned that this proposal has over overloaded getters and/or union types is that the latter are large features and are harder to implement. I don't think that a quick-and-dirty patch adding a rather niche feature with a lot of complexity and implicit confusion is a good solution, especially when the alternatives are major features that are requested by large parts of the language's user base.

Besides, if someone really wants this feature, they can very easily hack together something nearly identical in the current language state/featureset:

infix fun Int.`=`(i: Int) {
    println(this + i)
}

fun main () {
    1 `=` 2 
     // prints 3
}

(tested in the Kotlin playground with Kotlin 1.7.10)

I don't see a reason to add this feature: it's easy to abuse, adds a lot of confusion and implicitness, and those who desperately want it can implement something nearly identical themselves. I don't see a reason to add it to the language itself.

@hrach
Copy link

hrach commented Jul 11, 2022

It seems that only (?) major use case is the Gradle scripts' DSL. Is that the only motivation here? Wouldn't be possible to implement such "extensions" conditionally as a compiler plugin - similar to all open, etc.? Assign overloads could also work only on predefined/anotated types, etc.

@fvasco
Copy link

fvasco commented Jul 11, 2022

Assign overloads could also work only on predefined/anotated types, etc.

In such case the effect of assignment depends on the declaration site, the assign operator presence and the type's annotations.
Another variable into the puzzle.

@Starlight220
Copy link

Perhaps a set or `=` infix operator for the Gradle container types would be a good midpoint?

@asodja
Copy link

asodja commented Jul 18, 2022

From the comments, I believe that one of the concerns is that this feature will change the meaning of =. So this is my view on that:
I believe that in user code you normally don't see a lot of operator overloads. I would guess users implement custom operator overloads rarely and only in the case of some specialized type (e.g. mutable collections), so for types that operator overload makes sense. They can already implement weird cases like 5 += "five", but that makes little to no sense.

̇With that in mind, it makes no sense to implement a custom assign operator except if you have an actual specialized type that operator overload makes sense for. So implementing assign for Int or String or List or any general type just doesn't make sense. And examples like "a" = 5 are very extreme.

And of course, abuse is always possible. But as you normally wouldn't use operator overload unless for some specialized types, my argument is that this feature won't be normally used in user code unless for some specialized type and due to that it won't change the meaning of the =. But it gives an option to frameworks with DSL (of course Gradle DSL, another example that can be fixed with that is DSL described in KT-4075) to make DSL easier to use.


If I understand well, then the goal should be to reduce "boilerplate", i.e.:
input2 bind calculatedProperty
input2 = calculatedProperty
In my mind, these lines of code do different operations, so it is not easy for me to understand why both should use the same syntax.

While writing normal Kotlin app code, your view is probably the one to follow. But for the DSL I am not so sure it is always desired.
One Gradle example, that is very verbose with Property wrapper type that we have is:

task.apply {
   // input file is: val inputFile: Property<File>
   inputFile.set(File("some/file"))
   inputFile.set(otherTask.flatMap { it.outputFile }) // otherTask.flatMap { it.outputFile } returns Property<File> 
}

And this reads just: set inputFile to File("some/file") and set inputFile to outputFile of other task. So from DSL point of view, I would argue that it's the same operation, you just want to assign some value to it, but you don't want to think if you need to call bind or = or .set or some other method if the type on the right side is different.

With assign operator feature, it could be just simply (without annoying .set and ()):

task.apply {
   inputFile = File("some/file")
   inputFile = otherTask.flatMap { it.outputFile }
}

Currently best what we can do without .set() is:

task.apply {
   inputFile.value = File("some/file")
   inputFile bind otherTask.flatMap { it.outputFile }
}

So it's very inconsistent depending on the type you assign and it makes DSL more complicated with a more cognitive load.

@asodja
Copy link

asodja commented Jul 18, 2022

I'd much prefer something that's limited but can be expanded consistently in the future instead of a low-effort solution that will introduce more implicit behavior and be redundant once the language evolves to have things such as union types or setter overloads.

Valid concern. I believe that this feature won't be redundant after/if the union type or setter overloads land. I see that there is some overlap in some cases but they probably solve different problems. For example, if you look at:

open class MyTask {
  var input: Property<File> = Property<File>()
    set(value: Property<File> | File | Path) {
      field = when (value) {
        is File -> Property(value)
        is Path -> Property(value.toFile())
        is Property<*> -> value
      }
    }
  }
}

Now, this makes sense for that small example and when you are the owner of MyTask type. But as a "library provider" I don't want that every user of my library would have to reinvent such setter for every field of type Property<File> in their tasks. As a "library provider" I want that Property<File> works out of the box for all user in the same way, and that they can assign types to it that I believe make sense.

But I can always be wrong here since I don't know in detail how union types would work and if you could solve a particular issue with some limited implementation of union types or not.


Regarding the implementation, the assign return type is always Unit, this is something @fvasco mentioned. So abuses like hacky if is not possible.

Now if you want to try what would be possible you can take += operator and play with it. Rules would be the same (except that += works also on var in some way).

@fvasco
Copy link

fvasco commented Jul 19, 2022

I realized that this KEEP clashes with my mental model.
For me, the "assignment" isn't an operator on an object but an operator on the variable. So it looks strange that a class can redefine an operator of a different entity (val in this case).

A similar feature is const of C variable, in this case defining a const Property limits the visibility of the Property's function, but in this case, this behavior is enabled at the declaration site.

Kotlin delegation is similar, it declares a different variable handling at the declaration site.
I notify that it is impossible to allow "a" = 3 using the delegation.
So my main question remains: is this solution clearly better than delegation?

@edrd-f
Copy link

edrd-f commented Jul 26, 2022

With that in mind, it makes no sense to implement a custom assign operator except if you have an actual specialized type that operator overload makes sense for. So implementing assign for Int or String or List or any general type just doesn't make sense. And examples like "a" = 5 are very extreme.

Actually, a year ago this exact example (Int assignment to String) was suggested by a community member:

image

(https://kotlinlang.slack.com/archives/C0B9K7EP2/p1610457810240300)

So it's not an extreme example and it shows yet another form of abuse this proposal allows.

Now, this makes sense for that small example and when you are the owner of MyTask type. But as a "library provider" I don't want that every user of my library would have to reinvent such setter for every field of type Property in their tasks. As a "library provider" I want that Property works out of the box for all user in the same way, and that they can assign types to it that I believe make sense.

Makes sense, however this use-case looks so specific I'm not sure the reduction in code duplication outweights the downsides.

@Peanuuutz
Copy link

Peanuuutz commented Oct 13, 2022

This proposal goes too far on the operator road that leads to such confusion as mentioned. In my opinion the advantage is way smaller than the disadvantage, as it just saves something that doesn't require much effort by the use of delegation. It's only reasonable in DSL, but is provided to be used globally.

interface Container {
    val property: Property<String>
}

val container = stub()
var property by container.property
property = "Hello"

The more I think about it, the more I dislike the use of Xxx.xxxAssign in the general scope, because it breaks the meaning of val. Assignment is so important in programing that it deserves dedicated usage.

@asodja
Copy link

asodja commented Oct 13, 2022

Update: we (Gradle build tool) had a discussion with the Kotlin team some time ago due to concerns from the community. It was later decided that assign operator overload won't be a language feature. But we will implement a Kotlin compiler plugin that will allow similar behavior (note: it was not possible to implement such a plugin at the time the proposal was written).

I think that resolves all concerns and also works great for the Kotlin Gradle DSL. Thanks to the Kotlin team for finding a solution and to the Kotlin community for raising valid concerns.

Note: since I am not a member of the Kotlin team, I will leave the final official resolution of this issue to them.

@edrd-f
Copy link

edrd-f commented Oct 14, 2022

That's awesome. Thanks @asodja, Gradle team and Kotlin team for listening to the community. ❤️

@mgroth0
Copy link

mgroth0 commented Nov 29, 2022

@asodja this compiler plugin idea sounds awesome. Is there anywhere we can go to see progress on this plugin or to test it out?

I am curious how similar it would be to the original proposal. I have a couple of questions:

  • Will it allow for alternative assignment operators like :=?
  • Will it allow for custom assignment operators to be used when declaring properties in a class?

My motivation for caring about this right now is to see if I can make lazy properties more concise.

I like to make classes where all properties are lazy. The issue is, I dislike the way this looks in my code. I think it becomes tedious to read and write code like this.

For example, here is a non-lazy class. It's perfectly concise.

class Person {
  val name = "bob"
  val age = 50
  val height = 70.5
}

Now, here is the lazy version:

class LazyPerson {
  val name by lazy { "bob" }
  val age by lazy { 50 }
  val height by lazy { 70.5 }
}

In my opinion, repeating the by lazy {} gets redundant and looks sloppy.

If I were only using lazy occasionally, this would be fine. However, I use it all the time everywhere. And it doesn't look great having by lazy {} everywhere.

What I would love is some sort of custom delegation operator that is a shortcut for by lazy {}. For example, maybe I could make := mean by lazy.

class ConciseLazyPerson {
  val name := "bob"
  val age := 50
  val height := 70.5
}

Does anyone else feel similarly that a feature like this is missing? Or is there some alternative solution that I am missing?

@elizarov
Copy link
Contributor Author

mgroth0 The compiler plugin does not allow adding new operators (like :=) to the compiler, it only allows customization of the exsistng assingment (=).

@elizarov
Copy link
Contributor Author

Since we've rejected the idea of adding the corresponding convention, as described in the KEEP, to the language directly, but are providing an extension point that allows the similar implementation as a compiler plugin to be used in Gradle, I'm closing this issue and PR.

The documentation for the feature shall change (as only a specific Gradle DSL behavior needs to be documented) and shall be hosted elsewhere. The documentation of the extension points itself will be performed in the future during compiler API stabilization.

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

10 participants