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

Attach to property #6

Closed
wants to merge 3 commits into from
Closed

Conversation

orangy
Copy link
Member

@orangy orangy commented Apr 22, 2016

This proposal enables instances to which properties are delegated using by keyword to receive "attach" call so that they can do some useful work before getValue/setValue calls will start happening.

@cypressious
Copy link
Contributor

cypressious commented Apr 22, 2016

Part of the proposed functionality can already be accomplished using extension factory functions, e.g.:

class Foo {
    val delegated by factory()

    fun bindToLifecycle(listener: Listener) { ... }
}

fun Foo.factory() = Delegate(this)

class Delegate(foo: Foo) : Listener {
    init {
        foo.bindToLifecycle(this)
    }

    operator fun getValue(...)
}

Not sure if this is an argument against it. Of course you can't access the KProperty instance.

Regarding Interface Delegation, this would be immensely useful because currently we have no way whatsoever to communicate between the delegating instance and the delegate instance.

@orangy
Copy link
Member Author

orangy commented Apr 22, 2016

@cypressious yes, but this doesn't work if you want to delegate to already existing instance, e.g. passed to constructor or otherwise available. You can wrap it with attach(instance) call which will return instance and execute attachment code, but it's more and more boilerplate. And, often, you still need KProperty.

@cypressious
Copy link
Contributor

I think in most cases, you will never delegate two properties to the same delegate, but maybe I'm wrong.

Anyway, I think making this mechanism available to extension functions is the real benefit here because you can delegate to a library class you don't control but can still handle stuff like lifecycle.

@@ -11,6 +11,7 @@ The proposals themselves are colloquially referred to as KEEPs.
| Coroutines | repo: [kotlin-coroutines](https://github.com/Kotlin/kotlin-coroutines) | [Issues](https://github.com/Kotlin/kotlin-coroutines/issues)
| Type aliases | [type-aliases.md](proposals/type-aliases.md) | [Issue #4](https://github.com/Kotlin/KEEP/issues/4)
| Bound callable references | [bound-callable-references.md](proposals/bound-callable-references.md) | [Issue #5](https://github.com/Kotlin/KEEP/issues/5)
| Attach to property | [attach-to-property.md](proposals/attach-to-property.md) |
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd still need an issue for discussion

Copy link
Member Author

Choose a reason for hiding this comment

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

My impression was that we discuss PRs in PRs, which is also an issue, no?

@bashor
Copy link
Member

bashor commented Apr 22, 2016

Can we slightly rethink "delegation by" semantic?

Let's say that val foo by expression is equivalent to:

private val foo$delegate = expression.getDelegate(this, ::foo)
val foo: T
    get() = foo$delegate.getValue(this, ::foo)

And add to stdlib:

operator inline fun <D> D.getDelegate(instance: Any?, property: KProperty<*>): D = this

IMHO it slightly more elegant, and allow to avoid mutable state in delegate class.

@orangy
Copy link
Member Author

orangy commented Apr 22, 2016

@bashor that would eliminate possibility to optimise delegate's storage in the future. We were considering optimising delegate fields when possible, e.g. when delegate is an object or when it's pretty primitive expression like another property value or constant value. Since we cannot reason about what getDelegate would return, that pretty much means we have to have a field for a property, even when it's actually not needed.

@bashor
Copy link
Member

bashor commented Apr 22, 2016

Good point! 👍
For the record:
With getDelegate we still can optimize some cases:

  • all your cases when it's our simple version
  • when return type of getDelegate is object

@orangy
Copy link
Member Author

orangy commented Apr 22, 2016

@bashor yep, I'll add it to alternatives.

@bashor bashor added the language label Jun 6, 2016
@abreslav
Copy link
Contributor

This YouTrack issue should be mentioned somewhere in the proposal: [KT-7925](KT-7925 Notify property delegate if it's attached to an object) Notify property delegate if it's attached to an object

@abreslav abreslav modified the milestone: 1.1 Oct 10, 2016
@Groostav
Copy link

Groostav commented Mar 6, 2017

Can I ask what the expected criteria for inheritance in the implementation of provideDelegate are?

I've got:

  • a delegate factory function prop(): PropInterface (for idiomatic use val whatever by prop()),
  • the type PropInterface: ReadOnlyProperty
  • a PropImpl: PropInterface,
  • a method provideDelegate method on PropImpl

this setup results in provideDelegate not being called --I need to write an extension function for PropInterface, in order to get it called. It would seem there is no vtable lookup on the resulting object, only a compiler-inserted (static) check for the provideDelegate method.

Is this by design? Should it not be documented in the docs: https://kotlinlang.org/docs/reference/whatsnew11.html#interception-of-delegated-property-binding

@abreslav
Copy link
Contributor

abreslav commented Mar 6, 2017

@Groostav A vtable lookup can only happen when some statically known type has a corresponding member declared. In Kotlin all conventions are resolved statically, and your static types don't have provideDelegate, so the behavior you describe looks legitimate.

I don't think this needs documenting, because this is the way all convention work: if the operator function is not present at compile time, it can't be called at runtime. The only difference here is that the compiler does not complain about the absent operator function, but simply skips the call to provideDelegate instead. This is unfortunate, but I see no way around it ATM

@Groostav
Copy link

Groostav commented Mar 6, 2017

the only difference here is that the compiler does not complain about the absent operator function

nor does it complain about an extra operator function --that is, it doesn't complain when you have too few provideDelegate implementations and it doesnt complain when you have too many provideDelegate functions. This is the kind of wiggle-room that madness is born from.

Similarly visibility is important: if the provideDelegate operator-function is internal or private it is not called, but no warnings about its declaration are given.

Perhaps then an IDEA "suspicious" check: If IDEA can find a provideDelegate function on the inheritence tree for the returned property type, but not one that is statically available, it would give you a suspiciousness warning. In my particular case it could be even more restrictive, if the inferred return type is more specific than the declared return type, and the inferred return type has a provideDelegate method but the declared one does not, then IDEA would raise a suspicion:

fun prop(): PropInterface = PropImpl()

results in

warning: the type of the returned instance has a provideDelegate method but the declared return type does not.


ps: I love that IDEA is suspicious of my code.

@elizarov
Copy link
Contributor

@orangy Shall we close it now?

@orangy
Copy link
Member Author

orangy commented Jan 15, 2018

Obsolete

@orangy orangy closed this Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants