@RejectSam : Rejecting SAM conversions in parameters

Language Design
1

I’m trying to gather feedback on an idea that was discussed in an Android Slack group and to see if the community thinks it’s worth me raising a KEEP for it.

The problem is that, in some cases, automatic SAM conversion can lead to subtle bugs and unintended behaviour.

The typical example is a register/unregister API, something that is often seen in older Android APIs.

Take for instance the pair of methods on the View class in Android:

public void addOnLayoutChangeListener(OnLayoutChangeListener listener) {
    ListenerInfo li = getListenerInfo();
    if (li.mOnLayoutChangeListeners == null) {
        li.mOnLayoutChangeListeners = new ArrayList<OnLayoutChangeListener>();
    }
    if (!li.mOnLayoutChangeListeners.contains(listener)) {
        li.mOnLayoutChangeListeners.add(listener);
    }
}

public void removeOnLayoutChangeListener(OnLayoutChangeListener listener) {
    ListenerInfo li = mListenerInfo;
    if (li == null || li.mOnLayoutChangeListeners == null) {
        return;
    }
    li.mOnLayoutChangeListeners.remove(listener);
}

and the SAM interface:

public interface OnLayoutChangeListener {
    void onLayoutChange(View v, int left, int top, int right, int bottom,
            int oldLeft, int oldTop, int oldRight, int oldBottom);
}

If a caller tries to use this API in the following manner:

class SomeActivity: Activity() {
    
    private val listener = { v: View, left: Int, top: Int, right: Int, bottom: Int, oldLeft: Int, oldTop: Int, oldRight: Int, oldBottom: Int ->
        // something
    }
    private val view get() = findViewById<View>(android.R.id.content)

    override fun onStart() {
        super.onStart()
        view.addOnLayoutChangeListener(listener)
    }

    override fun onStop() {
        super.onStop()
        view.removeOnLayoutChangeListener(listener)
    }
}

They will introduce a bug. The problem is obviously that, internally, the method adds/removes an object from some data structure, and since there is an implicit SAM conversion at the call site of addOnLayoutChangeListener and removeOnLayoutChangeListener, the objects are not the same, therefore the listener is never removed.

There is a similar bug when an API internally holds a weak reference to the passed object. The bug there is that the weak reference is almost immediately cleared since nothing holds on to the SAM object if it was declared in a similar manner above.

What I propose is a new annotation, @RejectSam or similar, where API declare sites can provide hints to the compiler that an automatic SAM conversion might introduce subtle bugs and should be avoided. For the example above, this would be:

public void addOnLayoutChangeListener(@RejectSam OnLayoutChangeListener listener) {
    // ....
}

public void removeOnLayoutChangeListener(@RejectSam OnLayoutChangeListener listener) {
    // ....
}

This could then cause either a warning or even a compiler error at the call site if an automatic SAM conversion happens.

2

Similar issue for

fun onLayoutChangeListener(v: View, left: Int, top: Int, right: Int, bottom: Int, oldLeft: Int, oldTop: Int, oldRight: Int, oldBottom: Int) = TODO()

view.addOnLayoutChangeListener(::onLayoutChangeListener)

@RejectSam does not fix the code:

view.addOnLayoutChangeListener(object : OnLayoutChangeListener { ... } )

Finally the follow code is a workaround for the issue:

    private val listener = OnLayoutChangeListener { v: View, left: Int, top: Int, right: Int, bottom: Int, oldLeft: Int, oldTop: Int, oldRight: Int, oldBottom: Int ->
        // something
    }
3

The problem is the implicit conversion of Lamba/Function types to SAM types. What is not a problem is the use of a lambda literal that directly becomes a SAM implementation (rather than function), both because of a lack of conversion and there not being a handle to it.

The function reference case is an interesting one. The problem there is the problem of comparison of listener “objects” created automatically. Without caching they will be different objects (even in Java) so removing the listener needs to support removal based on equality and the objects need to support equality comparison - if they don’t it is a bug/missing feature.

I agree with you that the automatic conversion is a problem. Maybe a way to look at it is that the problem is that these objects take shared ownership of the the SAM object and expose this ownership externally (removeOnChangeListener exposes the ownership). If the “shared” object on the calling side is a temporary that will not work properly unless there is no actual sharing (the calling side forgets the object).

Note that this problem is the same in Java. Method handles are based upon identity equality (two method references to the same method on the same object (not class) are not actually equal. One thing that could be done is the observation that Sam conversion from a property is almost always invalid or suboptimal. A warning with quick fix would be warranted and should catch most of these issues.