Consider adding support for context receivers in PropertySpec #1244
Comments
|
The spec says
Which are implemented as |
|
Makes sense of course! I was just too naïve and thought they would be implemented as another property of the property spec builder. Silly me. |
|
Sorry to reopen, but I tried adding context receivers via getters/setters and it doesn't seem to work properly. The context is declared on the actual getter/setter and not on the property. Example: PropertySpec
.builder(name = "loudGreeting", String::class.asTypeName())
.getter(
FunSpec
.getterBuilder()
.contextReceivers(String::class.asTypeName())
.addStatement("return \"Hello, \${uppercase()}\"")
.build()
)
.build()Yields: public val loudGreeting: String
context(String) // Error
get() = "Hello, ${uppercase()}"While I believe it should yield this instead: context(String)
public val loudGreeting: String
get() = "Hello, ${uppercase()}"This is a very simple example which could surely have been simply solved as an extension property on In fact, I believe the context declaration (if present) must be the first statement on the property declaration (including annotations). For example, lets assume we have: annotation class MyFancyAnnotationThis: PropertySpec
.builder(name = "loudGreeting", String::class.asTypeName())
.addAnnotation(MyFancyAnnotation::class.asTypeName())
.getter(/*...*/)
.buildShould yield: context(String)
@MyFancyAnnotation
public val loudGreeting: String
get() = "Hello, ${uppercase()}" |
|
Can you explain what you mean by "doesn't work properly"? I understand you expect it to be on the property itself and not the functions, but is that just a syntax/style thing or is it functionally incorrect to place them on the getter/setter methods? Please be specific and link specs as I could not find anything in the corresponding KEEP that indicated otherwise |
|
The generated code is invalid. The context declaration must come first (as per the examples given). Also, if a property is annotated, declaring the context after the annotation is also invalid. When you mentioned setting context receivers using the function spec for getters and setters, I though KotlinPoet would check all the declared type names and compute the actual context for the property as the union of all those types names and add it as the first statement for the property declaration. So if you did something like: PropertySpec
.builder(name = "someProperty", ReturnType::class.asTypeName())
.receiver(TypeA::class.asTypeName())
.getter(
FunSpec
.getterBuilder()
.contextReceivers(TypeB::class.asTypeName())
.addStatement(\*...*\)
.build()
)
.setter(
FunSpec
.setterBuilder()
.contextReceivers(TypeC::class.asTypeName())
.addStatement(\*...*\)
.build()
)
.build()KotlinPoet would generate something like: context(TypeB, TypeC)
var TypeA.someProperty: ReturnType
get {\*...*\}
set {\*...*\}Anyways, from a language point of view, I don't think it would make much sense to be able to set the context for setter and getter separately. That would mean that the same symbol would have different access levels depending on the context. Also, it seems reasonable that the language would enforce the context declaration as the first statement (including annotations) since that defines the scope in which the symbol definition below is available. Unfortunately I don't have any relevant links to the corresponding KEEP, but you can check that the generated code won't compile. |
|
Can you paste an example snippet with a compiler failure stacktrace? |
|
Sure, no prob. This PropertySpec
.builder(name = "greeting", String::class.asTypeName())
.getter(
FunSpec.getterBuilder()
.contextReceivers(String::class.asTypeName())
.addStatement("return \"Hello, \${uppercase()}!\"")
.build()
)
.build()generates this: public val greeting: String
context(String)
get() = "Hello, ${uppercase()}!"and the compiler is sad because:
I filtered out the stack trace because right now the error messages regarding context receivers are still a bit cryptic. |
|
Please paste the full stacktrace. The one you quoted doesn't make much sense |
|
It actually makes total sense. The
I can post the full stack trace, but you won't get anything more useful out of it. All the relevant info is already there. AndroidStudio is actually quite smart and able to give you these error messages on the appropriate lines as you type. Maybe the more straightforward approach would be to prevent setting context receivers altogether in setters and getters specs and simply providing them at Anyways, here you have the not-so-useful task log: The even less useful full stack trace: |
|
Played a bit with it and it seems that you can't add a context receiver to a property without a custom accessor - is that right? I think the API should be similar to how we model inline properties: if context receivers are added to accessors,
Sorry, I don't think I understand the last sentence - can you elaborate please and provide an example? |
Yes, that sounds about right. It's the exact same issue we have with extension properties (no backing field). Actually, a property with a single context receiver, I believe, is exactly the same thing as an extension property.
It doesn't make sense to add context receivers to accessors and the language currently forbids it.
To elaborate on why it doesn't make sense to add context receivers to accessors consider the following (invalid) code: // Confusing.kt
interface Water
interface Oil
var meaningOfLife: Int
context(Water)
get() = 42
context(Oil)
set(value) { field = value }What would this mean?
I think this is the intended usage: interface DogHardware {
val appearance
get() = "look like a dog"
}
interface CatSoftware {
val behaviour
get() = "pounce like a cat"
}
context(DogHardware, CatSoftware)
val description
get() = "I $appearance and $behaviour!"
class SomeAnimal : DogHardware, CatSoftware {
override fun toString() = "I am a Fox: $description"
}My suggestion for the API would be: PropertySpec
.builder(name = "loudGreeting", String::class.asTypeName())
.contextReceivers(String::class.asTypeName() // how context receivers should be set for properties
.getter(
FunSpec
.getterBuilder()
.contextReceivers(String::class.asTypeName()) // forbidden
.addStatement("return \"Hello, \${uppercase()}\"")
.build()
)
.build() |
I'll rephrase, my suggestion was to add context receivers to That said, I'm leaning towards actually adding the method on the Wanna send us a PR? |
Oh, I see. That would indeed be an option. However, as you mentioned, having them scattered across the accessor's
I can't really commit myself beacuse I'm quite tight on time and I also literally started using KP last week, so I am still unfamiliar. However, I can have a look at the lib code and if I am able to come up with something soon I'll send a PR. |
|
Closed by #1247 |
I opened this regarding support for context receivers yesterday and received the reply that they got supported with #1232. I checked out
1.12.0-SNAPSHOTbut it seems there is currently no support for context receivers onPropertySpec, which is exactly the use case I have for the library I am writing 😅. Please consider adding support for this use case.The text was updated successfully, but these errors were encountered: