Kotlin / KEEP

Kotlin Evolution and Enhancement Process
Apache License 2.0
3.3k stars 356 forks source link

Scope control for implicit receivers #57

Closed dzharkov closed 6 years ago

dzharkov commented 7 years ago

Discussion of https://github.com/Kotlin/KEEP/blob/master/proposals/scope-control-for-implicit-receivers.md See also PR #38

voddan commented 7 years ago

The wording of the proposal is quite tricky. Is there a prototype of such a DSL with the markers?

abreslav commented 7 years ago

@elizarov https://github.com/Kotlin/KEEP/pull/38#issuecomment-261769864

Proposed rule: All members inherited by DSL class from non-DSL classes are not available in extension functions of this DSL class.

Looks very interesting, let's discuss it further at a design meeting

Proposed rule: DSL class reveiver cannot be accessed by unqualified or qualified 'this' from its extension functions, moreover it does not shadow 'this' from outer scope.

Looks like this blocks simple Extract Function refactoring on DSL code, no?

dzharkov commented 7 years ago

@voddan You can follow this change in kotlinx.html

Also if you have an idea how to reword the proposal to make it clear, feel free to contribute

voddan commented 7 years ago

Thank you @dzharkov, the line is very helpful.

Yet I still wish there was a smaller, more sophisticated example embedded into the proposal. For one, kotlinx.html does not cover the multiple tags case, which seems to be a major factor in your design.

ilya-g commented 7 years ago

Minor inconvenience: annotating the receiver type in a parameter having function-with-receiver type always requires to surround the receiver type with parentheses:

fun build1(builder: (@TestDsl DslReceiver1).() -> Unit) {}

That might be hard to explore and inconvenient to type if you need to apply this annotation to a number of such dsl functions.

The suggestion is to add another marking rule — the implicit receiver is annotated if the functional type with that receiver is annotated, so that it would become possible to mark the receiver the following way:

fun build1(builder: @TestDsl DslReceiver1.() -> Unit) {}
voddan commented 7 years ago

I have a strange corner case:

@DslMarker 
@Target(AnnotationTarget.TYPE)
annotation class DSL

interface I

class K() : I

open class F(val name: String) : I{
    fun foo() = println(name)
}

fun fish(op: (@DSL F).() -> Unit) = F("top").op()
fun kish(op: (@DSL K).() -> Unit) = K().op()

class A : F("class A") {
    init {
        foo() // A

        fish {
            foo() // fish

            kish {
               // Error: fun foo(): Unit' cant be called in this context by implicit receiver. Use the explicit one if necessary 
               foo()  // A ???
            }
        }
    }
}

fun main(args: Array<String>) {
    A()
}

I would expect the 3rd foo() to be resolved to the class itself, since it is not marked with @DSL

Is this a bug or intended behaviour?

voddan commented 7 years ago

BTW, what's "a type's classifier"?

dzharkov commented 7 years ago

@voddan This behavior is intended since we don't want to change resolution rules because of DSL markers, i.e. without those markers we'd choose the receiver from the fish lamba and we do just the same when the annotations are applied. The difference is that additional checks are performed for each such call.

A "type's classifier" is a class or a type parameter from which the type is constructed

r-a-o commented 6 years ago

@voddan how did you solve this corner case? I also have a similar construct and its failing to call the "3rd foo()"

voddan commented 6 years ago

@voddan how did you solve this corner case? I also have a similar construct and its failing to call the "3rd foo()"

I don't think I did. My complaint was more about the counterintuitive behavior (which ,according to @dzharkov, it has to be because of backward compatibility) than about the problem itself.

r-a-o commented 6 years ago

Ok, so how do I make this nested construct work? I am open to any suggestions at all, if there is no way around this, I will have to move back to using XML for layouting.

override fun createView(ui: AnkoContext<SignInActivity>) = with(ui) {
        ankoContext = ui

        verticalLayout {
            this.gravity = Gravity.CENTER
            lparams(width = matchParent, height = matchParent)

            scrollView {
                lparams(width = matchParent, height = wrapContent)

                verticalLayout {
                    lparams(width = matchParent, height = matchParent)

                    verticalLayout {
                        id = R.id.formLogin
                        gravity = Gravity.CENTER
                        padding = dip(20)
                        lparams(width = dip(300), height = matchParent) {
                            this.gravity = Gravity.CENTER
                            // API >= 16
                            doFromSdk(version = Build.VERSION_CODES.JELLY_BEAN) {
                                background = ContextCompat.getDrawable(ctx, android.R.color.white)
                            }
                            clipToPadding = false
                            bottomMargin = dip(16)
                        }

                        val username = editText {
                            lparams(width = matchParent, height = wrapContent) // **lparams invocation fails** 
                            id = R.id.usernameEditText
                            hintResource = R.string.sign_in_username
                        }

                        val password = editText {
                            lparams(width = matchParent, height = wrapContent) // **lparams invocation fails** 
                            id = R.id.passwordEditText
                            hintResource = R.string.signIn_password
                        }

                        button {
                            lparams(width = matchParent, height = wrapContent) // **lparams invocation fails** 
                            id = R.id.signIn_button
                            textResource = R.string.signIn_button

                            onClick { // **onClick invocation fails** 
                                handleOnSignInButtonPressed(username = username.text.toString(), password = password.text.toString())
                            }
                        }
                    }.applyRecursively { view ->
                        when (view) {
                            is EditText -> view.textSize = 24f
                        }
                    }
                }
            }
        }
    }
abreslav commented 6 years ago

@r-a-o Looks like an Anko question, so probably wrong repo?

abreslav commented 6 years ago

@dzharkov Shouldn't we close this PR?

r-a-o commented 6 years ago

@abreslav Are you saying this error is Anko specific?

Error:(x, x) 'inline fun <T : View> EditText.lparams(width: Int = ..., height: Int = ...): EditText' can't be called in this context by implicit receiver. Use the explicit one if necessary

Anko uses Kotlin DSL, and I'm pretty sure this is DSL issue.

abreslav commented 6 years ago

@yanex ^

yanex commented 6 years ago

@r-a-o As described in the documentation, lparams {} should be called outside the lambda argument, e.g.:

val username = editText {
    id = R.id.usernameEditText
    hintResource = R.string.sign_in_username
}.lparams(width = matchParent, height = wrapContent)

Calling lparams() inside the view block was explicitly forbidden because it was error-prone for multiple reasons. We might support the old syntax in a safe way in the future, but that would definitely be an another KEEP.

r-a-o commented 6 years ago

@abreslav @yanex Thanks a bunch! I didn't notice this usage change in the docs. I like this chaining approach better.

dzharkov commented 6 years ago

The feature has been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.