Smart casts and nullability in single-threaded contexts

Language Design
21

Ok, smart casts and nullability in single-threaded environments are the reasons why I joined this forum, so… hi all :smiley:

Since Kotlin can be used now as a language of choice for Android, there is a big reason for having something like this: keeping view elements as a fields in Fragments or Activities (etc.).

Consider following example:

class PlayerComboFragment : Fragment() {

    private val mScrollListener = object : RecyclerView.OnScrollListener() {
        override fun onScrollStateChanged(recyclerView: RecyclerView?, newState: Int) {
            // ...
        }
    }

    private var mPlayerViewModel: PlayerViewModel? = null
    private var mRecyclerView: RecyclerView? = null
    private var mSnapHelper: LinearSnapHelper? = null

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        mPlayerViewModel = PlayerViewModel(activity.settingsManager,
                activity.messageExchangeHelper, activity.ambientModeStateProvider,
                activity.musicLibraryNavigator)
    }
    
    override fun onCreateView(inflater: LayoutInflater,
                              container: ViewGroup?,
                              savedInstanceState: Bundle?): View? {
        val view = inflater.inflate(R.layout.player_combo_view, container, false)

        mRecyclerView = view.findViewById(R.id.recycler_view) as RecyclerView
        mRecyclerView!!.layoutManager = SmoothScrollingLinearLayoutManager(activity)
        mRecyclerView!!.adapter = MainRecyclerViewAdapter(mPlayerViewModel!!)
        mRecyclerView!!.scrollToPosition(1)
        mRecyclerView!!.addOnScrollListener(mScrollListener)

        mSnapHelper = LinearSnapHelper()
        mSnapHelper!!.attachToRecyclerView(mRecyclerView)

        return view
    }
    
    override fun onResume() {
        super.onResume()
        mPlayerViewModel!!.onResume()
    }

    override fun onPause() {
        mPlayerViewModel!!.onPause()
        super.onPause()
    }
    
    override fun onDestroyView() {
        mRecyclerView!!.removeOnScrollListener(mScrollListener)
        mRecyclerView!!.adapter = null
        mRecyclerView!!.layoutManager = null
        mSnapHelper!!.attachToRecyclerView(null)
        super.onDestroyView()
    }

    override fun onDestroy() {
        super.onDestroy()
        mPlayerViewModel = null
    }
}

The number of exclamation marks is just annoying. Workarounds like a local variable or using let and so on do not convince me at all. All the work is done on the UI thread, fields are private, so there’s no way that anyone could ever nullify these. Not to mention that even trying to fiddle with UI elements from another thread would cause a big mess… :wink: It is really a common practice, to find/create view and then configure it.

Going a bit further, I could also live with something like this:

override fun onDestroyView() {
    mRecyclerView!!.removeOnScrollListener(mScrollListener)
    mRecyclerView.adapter = null
    mRecyclerView.layoutManager = null
    mSnapHelper!!.attachToRecyclerView(null)
    super.onDestroyView()
}

I mean it should be pretty much enough to just do “!!” on mRecyclerView once in this scope, without a need to do it two more times below - as this is still the same, single-thread fragment, it would throw NPE anyway before reaching the further code.

Actually I’d even prefer a shorter solution - a keyword that we could place next to “var”, to make compiler happy :slight_smile:

22

This keyword is lateinit :slight_smile:

23

No, it is not - lateinit does not allow you to nullify view (or anything else) in onDestroyView (or in any other place :stuck_out_tongue:) .

24

Another possibility is to use sub objects of your activity or fragment. I am only showing it partially for models here:

class PlayerComboFragment : Fragment() {
    private var models: PlayerComboFragmentModels? = null

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        models = PlayerComboFragementModels()
    }

    override fun onResume() {
        super.onResume()
        models?.onResume()
    }

    override fun onDestroy() {
        super.onDestroy()
        models = null
    }

    private inner class PlayerComboFragmentModels {
        private val mPlayerViewModel: PlayerViewModel

        init {
            mPlayerViewModel = PlayerViewModel(activity.settingsManager,
                    activity.messageExchangeHelper, activity.ambientModeStateProvider,
                    activity.musicLibraryNavigator)
        }

        fun onResume() {
            mPlayerViewModel.onResume()
        }
    }
}
25

This is a bit hacky, but a very nice suggestion, though :slight_smile: Thanks!

(I still consider it as a workaround of the “real” problem…)

26

I don’t see what is hacky about it: You enter a certain scope/state (in this case the fragment becomes created), so you create a new object for working within that scope/state.

1 Like
27

Okay, maybe “hacky” is a bad word. Rather just different - it’s not exactly what you get used to when writing similar code in Java. While I don’t mind changing a mindset, then I am not sure if I can say that your solution is perfect - let’s for example separate an object for views:

private inner class Views(view: View) {
    private val mContentView: WearableRecyclerView = view byName R.id.recycler_view
    private val mSpinner: ProgressSpinner = view byName R.id.spinner
    private val mSnapHelper = LinearSnapHelper()
    private val mAdapter = BrowserRVAdapter<Clickable>(createViewHolderFactory())

    init {
        mContentView.layoutManager = CurvedChildLayoutManager(activity)
        mContentView.centerEdgeItems = true
        mContentView.adapter = mAdapter
        mSnapHelper.attachToRecyclerView(mContentView)
    }

    fun resume() {
        mContentView.isCircularScrollingGestureEnabled =
                activity.settingsManager.useCircularScrollingGesture()
    }

    fun pause() {
        mScrollStateHelper.save(mContentView)
    }

    fun destroy() {
        mSnapHelper.attachToRecyclerView(null)
        mContentView.adapter = null
        mContentView.layoutManager = null
    }

    fun refresh(restoreScrollState: Boolean) {
        if (tryRestoreCachedItems()) {
            mSpinner.visibility = View.GONE
            mContentView.visibility = View.VISIBLE
            if (restoreScrollState) mScrollStateHelper.restoreTo(mContentView)
        } else {
            mSpinner.visibility = View.VISIBLE
            mContentView.visibility = View.GONE
            fetchItems()
        }
    }
    
    fun scrollToPosition(position: Int) {
        mContentView.scrollToPosition(position)
    }
}

It’s pretty understandable that I need to place all view-accessing functions in such sub-object. But let’s just add some public function to the Fragment itself:

fun refresh() {
    onGoingToRefresh()
    mViews!!.let {
        it.refresh(false)
        it.scrollToPosition(0)
    }
}

And the thing that bothers me a little (but definitely less than lots of exclamation marks :smiley: ) is the fact that I really need to have these extra wrapper methods, like scrollToPosition inside Views class, which just forwards the argument to mContentView. Normally I wouldn’t care, but I’ve used to work on a product which was very close to hit 65k methods limit in android’s dex file and we could not use MultiDex library for a long time.

But now when I think of it, while writing this post, “inline” should do the work, shouldn’t it? :slight_smile:

28

There is a mismatch between Android and Kotlin because Android was designed for Java. Kotlin can help ease the pain of state management a bit, but you cannot write short, clean, and correct code for Android.

If, for example, Android called your activity/fragment/… factories instead of directly creating the object, things would be a lot better.

If you run into the problem with too many methods, it certainly does.

Try to limit them to internal and private, because client code won’t see changes to inline functions until the client code is recompiled.

29

FWIW, I filed an issue over this:
https://youtrack.jetbrains.com/issue/KT-20294

Feel free to voice your support :slight_smile:

Another issue over const getters is forthcoming, when I find the time…

1 Like
30

@mslenc I really do wonder why you can’t get your point across. I’m a Kotlin noob, but I fully agree with you, especially concerning the multithreaded access. Without volatile, the compiler and the JVM are free to cache the field in a local variable and then the NPE is impossible. This could Kotlin do silently when a smart cast is needed.

However, with interleaved method calls, it’s no more true as they could ensure visibility of a field change or modify it directly. OTOH repeating x!!. multiple times is pretty terrible and makes the code hard to read and prone to missing some more realistic nullability issues.

Maybe the right thing could be some syntax like

if (x != null) !!! {
    x.f();
    x.g();
    x.h();
}

where !!! after the condition would say “I know, it might blow, but telling it once is good enough; I insist on the smart cast”. Unlike a class-wide @SharedAcrossThreads annotation, it’s close to the possibly throwing code, it’s pretty short, yet nicely visible and much less disturbing than the “idiomatic” way.

It looks like the normal !! operator and does the same thing: It tells the compiler that you know what you’re doing.

31

I don’t know either… My working hypothesis is that the current compiler infrastructure is too weak to allow for decent flow analysis (smart cast for locals also breaks down as soon as you touch them from local functions or even with inline stuff), and the team prefers working on flashy new features rather than improving the basics… Then they hide behind “safety” and claim it’s good for us :slight_smile:

32

Then, you would sooner get no smart casts at all.

And it’s not true that local smart casts break down as soon as you touch them. Inline functions cannot change them (there are no out parameters) and if you reassign it a nullable value, it will be smart cast back to nullable.

33

Yeah…

val foo:String
something.let {
   foo = it // Error: Captured values initialization is forbidden due to possible reassignment
}

fun bubu() {
    var foo: String? = null
    fun baba() {
        foo = foo ?: "default"
    }
    if (foo != null)
        println(foo.length) // Error: Smart cast to `String` is impossible, because 'foo' is a local variable that is captured by a changing closure
}

Note how baba is not only not called during the if statement, but in fact never…

I didn’t say there is no analysis, just that it’s weak…