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

Separate types for private and public property access #122

Closed

Conversation

matejdro
Copy link

@matejdro matejdro commented Jun 12, 2018

@matejdro matejdro changed the title Private public property types Separate types for private and public property access Jun 12, 2018
@sakno
Copy link

sakno commented Jun 13, 2018

It is possible to achieve your example in existing version of language.

An example:

val items = mutableListOf<Item>()
        get(): List<Item>

Current version of language:

val items: List<Item> = mutableListOf<Item>()
  get() { /* do what you want */ }

@matejdro
Copy link
Author

matejdro commented Jun 13, 2018

I've tried your code and it does not compile for me (it errors out with Initializer not allowed because this property has no backing field).

In any case, this does not solve the issue. I cannot just type items.add(Item()) inside class, since items will be read-only List<Item> for both inside class and outside class. Whole point of this proposal is that items would be MutableList<Item> when accessing from inside class, allowing modification, but it will be just read-only List<Item> from outside class.

@cypressious
Copy link
Contributor

Have you thought about the consequences for reflection? Would we need new properties on KProperty?

just read-only List<Item> from inside class.

you probably mean "outside"

@matejdro
Copy link
Author

Have you thought about the consequences for reflection?

I have not used Kotlin's reflection in practice so I'm not sure if my assessment is good enough, but if this feature is implemented by just changing the type of the backing field (so Kotlin would produce similar java bytecode than example at the top of the proposal), then there is nothing to change for JVM Kotlin reflection. Only thing that would change is type of the returned javaField backing field object.

I have no idea what that means for Native and JS reflection though (have not looked at them at all).

you probably mean "outside"

Yes. Fixed that, thank you.

@cypressious
Copy link
Contributor

@matejdro Take a look at https://kotlinlang.org/docs/reference/reflection.html#property-references as well as KProperty and the derrived types like KMutableProperty.

@Wasabi375
Copy link
Contributor

Wasabi375 commented Jun 13, 2018

As far as I can tell this has an effect on reflection. Kotlin will have to generate multiple getters and setters and KProperty and KMutableProperty will need to reference all of them.
Therefor we probably need to add privateGetter, internalGetter and protectedGetter as fields to KProperty.
Another thing to consider is also which getter is represented by the old getter field. I think for backwards compatibility it has to be the public version.
Same is true for the setters in KMutableProperty.

@matejdro
Copy link
Author

matejdro commented Jun 14, 2018

That is true if we decide to go "getter for each visibility layer" route. But if this functionality is not needed, we can just have private type (accessed by private backing field) and public type (accessed by getters and setters). Number of generated getters and setters would stay the same.

@sakno
Copy link

sakno commented Jun 14, 2018

@matejdro, there is the valid portion of code from my example:

val items: List<String> = mutableListOf()
        get(){
            return field
        }

Error which you was received means that property in Kotlin may or may not have automatic backing field accessible using field identifier inside of getter or setter. But you are right, there is no way to define different property types for different visibility scopes.

@Wasabi375
Copy link
Contributor

Wasabi375 commented Jun 14, 2018

Ok, correct me if I'm wrong but right now there is no way to access the backing field using reflection, so if we decide to only change the type of the backing field the reflection code still has to be updated.

Currently the easiest way (I know) to get the type of a property using reflection is
property.returnType. This is inherited from KFunction and in this case is the return type of the getter. So I would suggest adding a backingType property to KProperty.


Also I would like to argue for the multiple getter version maybe with a small change. I think most of the time you would only want a different private field therefor I think that the suggested syntax should be supported. But IMO the next logical step would be to support different types for different visibility levels, especially internal would be useful for library authors I think.
The change I would make is this:

val items = ArrayList<Item>()
        protected get()
        internal get() : MutableList<Item>
        /*public*/ get(): List<Item>

have the public modifier be optional, it is everywhere else. That way this syntax still works as expected

val items = ArrayList<Item>()
        get(): List<Item>

@matejdro
Copy link
Author

There is javaField property. but this one is a bit java-centric, so I agree with you that there should be kotlin-native backingField option in the reflection.

I agree with your optional public modifier.

@Wasabi375
Copy link
Contributor

Yes I missed javaField but this would still be needed for Js and Native, also javaField returns a Field which is part of the java reflection so we loose information about Nullability, etc.

@Wasabi375
Copy link
Contributor

I think the backing type needs to be restricted to be a subtype of the public type. I can think of 2 reasons for that which are both kinda important.

class A(val a: Int)
class B(val b: Int){
    val a: Int
        get() = b + 5
}

With the current proposal as far as I understand it, I could create a property like this

val someProperty: A = A(5)
    get(): B = B(field.a)

When accessing this property from the outside everything works as intended, but when accessing it from within the properties scope there are 2 problems:

  1. We have to decide right now whether we return the value of the backing field or the value of the getter. This proposal decides for the value of the backing field (which is different than kotlin does it normally if there is a custom getter). That means however that there is no way of accessing b ( I know that there is no reason to in my example, just imagine we would want to 😉).
  2. someProperty.a returns 2 different values depending on from where we call it. I think this is confusing and just leads to bugs.

Why I think restricting the backing type solves those problems:

  1. By restricting the private type to be a subtype of the public type we ensure that there are no members in the public type we can't access using the private type.
  2. This can't really be solved as the getter could return something completely different.
val list = mutableListOf(1, 2, 3)
    get(): List<Int> = emptyList<Int>()

however, if the getter is implemented as a simple cast, this will make sure that everything works as expected.

@ilya-g
Copy link
Member

ilya-g commented Jul 23, 2018

@Wasabi375 It seems that in a single-getter version it should be prohibited to have custom getter, because the implementation should always be the same: just an upcast.

@pdvrieze
Copy link

pdvrieze commented Nov 3, 2018

I would like to propose a different/additional syntax as well as some restrictions:

val list: MutableList<Int>= mutableListOf(1, 2, 3)
    get(): List<Int>

val nonTrivialList
  private get(): MutableList<Int> { /* Some getter body */ }
  public get(): List<Int> // may not have a body

In this case there are two types, one on the field, one for the getter, but they are allowed to be different (but must be compatible). It requires the getter to be trivial.

In case of a non-trivial getter, this should be the private getter, the public getter would have no body and invoke the non-trivial getter). Note that this does not work in Java if the non-trivial getter is not private (or marked synthetic). While on the JVM the return type is part of the signature, Java ignores the return type in overload resolution. Another reason for a private getter (and name mangling) is in the case of the variance being in a generic parameter and the return types being assignment compatible.

In the very special case of List/MutableList and the like the compiler may just generate a single method as on the JVM level there is no diffence between both types.

@cypressious
Copy link
Contributor

What about solving the problem using smart casts?

When you declare

val user: LiveData<User> = MutableLiveData<User>() 

inside the class, the compiler should be able to infer that the property is always of type MutableLiveData<User>. This way, you wouldn't need to change the language.

@matejdro
Copy link
Author

matejdro commented Nov 7, 2018

This works for val, but not for var.

@pdvrieze
Copy link

pdvrieze commented Nov 7, 2018

@cypressious My main concern with that is that it does not require the programmer to indicate it. In many cases you do not want to rely on the concrete type even in the class. Generally the access to the raw field should be limited to only the getters and setters. If specific access is needed outside the getter/setter (even privately in the class) it is better when that is made explicit.

As such there are many cases where a private property is still the best solution, but in this case the only interesting thing that happens is exposing the implementation type (rather than the interface type). This is very common, and could be supported reasonably with somewhat simple syntax rule changes.

@Zhuinden
Copy link

Zhuinden commented Nov 7, 2018

@cypressious that way you lose the ability to call postValue function on user, no?

@Dico200
Copy link

Dico200 commented Oct 17, 2019

I'm not a fan of

val x = ArrayList<String>()
    get(): List<String>

There is no indication that there are 2 types and/or 2 getters here. It seems to come down purely to the assumption that compiler will generate a backing field that is private with a sub type of the getter type, which is preferred if visible.

private val x = ArrayList<String>()
    public get(): List<String>

Makes the expected behaviour a bit more obvious to me.
As for setters, if there is one, the setter must have the most concrete parameter type of all accessors, in this case ArrayList<String>()

Non-trivial getters are a different beast. I think allowing more than one non-trivial getter would be confusing, as there is just one property to access. Try to keep it simple (KISS principle).

@Dico200
Copy link

Dico200 commented Oct 17, 2019

More thoughts on setters. If you cannot do foo.x = bar.x, that should be illegal and the setter shouldn't be visible at all.
In other words, I suggest that the setter can't be more visible than the accessor with the most concrete type, which in the example above is private and ArrayList<String>. So:

private var x = ArrayList<String>()
    public get(): List<String>

Is legal and intuitive. But

private var x = ArrayList<String>()
    public get(): List<String>
    public set

Is illegal. Because setter parameter type MUST be ArrayList<String>, and so you wouldn't be able to do
foo.x = bar.x because List is not assignable to ArrayList.

@Dico200
Copy link

Dico200 commented Oct 17, 2019

You could have

    public set(value: List<String>) {
        field = ArrayList(value)
    }

But it makes properties have very complex rules.

@CharlyLafon37
Copy link

CharlyLafon37 commented Nov 10, 2019

I'm surprised by the lack of recent activity on this issue. The current way of exposing a type as its supertype is really harmful to the code's readability :

private val _user = MutableLiveData<User>()
val user: LiveData<User>
      get() = _user

Can't we come up with a keyword like "exposeas" that would allow us to write : val user = MutableLiveData<User>() exposeas LiveData ? The type on the right-hand of "exposeas" would have the constraint to be a supertype of the type at the left-hand of "exposeas".

This keyword would keep the access to the variable as a MutableLiveData inside the class that declares it, but from the outside then it would be accessed as a LiveData. In Java it would generate a private variable of type MutableLiveData with a getter that returns a LiveData.

Of course this solution has serious pitfalls that I can't think of right now because I'm no Kotlin expert. But the important part is the issue this thread is talking about. It really is about writing boilerplate code for such a common thing (especially in Android), whereas Kotlin's main "selling point" is being concise and lightweight.

Hope this issue will become more active. French here so don't mind the bad english

@Zhuinden
Copy link

Zhuinden commented Nov 10, 2019

I've lately just been opting to use mutable as a prefix so that I can avoid _ prefix, because the _ reads terrible and looks just as bad as it does in Dart.

private val mutableUser = MutableLiveData<User>()
val user: LiveData<User>
      get() = mutableUser

So while it's a tricky limitation, I'd say you can somewhat work around it, but the official recommendation to use a _ prefix is quite harmful and I'm surprised it's a recommendation at all.

Maybe we should just be using fun() for the public accessor, then we could retain the name of the variable too.

@tom-jb
Copy link

tom-jb commented Apr 14, 2020

Is there no update here? This is one area where Kotlin is seriously lacking right now.

I like @CharlyLafon37's idea a lot:

val items = mutableListOf<Item>() exposes List<Item>

@matejdro
Copy link
Author

This now also applies to the new StateFlow from coroutines (we need to keep MutableStateFlow private while exposing StateFlow).

@elizarov
Copy link
Contributor

I'm closing this KEEP as it does not represent the actual design proposal. It essentially restates the use-cases and syntax proposal from the original issue, leaving all the important design issues open (see https://github.com/Kotlin/KEEP/blob/master/README.md for details). Please, continue the discussion of use-cases in the corresponding YT issue: https://youtrack.jetbrains.com/issue/KT-14663

@elizarov elizarov closed this Dec 24, 2020
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

Successfully merging this pull request may close these issues.