Added `ComposeItemKeyNotSaveable` rule by jonapoul · Pull Request #572 · slackhq/compose-lints

jonapoul · 2026-06-07T08:51:23Z

This adds a new rule that flags item key values in lazy layouts (LazyColumn, LazyVerticalGrid, staggered grids, Wear lazy lists, etc.) when the key type must be saveable into a Bundle, but the parameter type is only Any (and therefore has no compiler-driven type checking. For reference, the target scopes for this rule was driven primarily by this search on cs.android.com. Until now I've been maintaining very a basic custom rule on this, but I figured it'd be better in a proper project.

On Android the key type needs to be a primitive, Serializable, Parcelable or one of a few other types. If the key is something else (e.g. a data class), the app will crash at runtime. I've been tripped up a few times by this in various projects, thought it'd be useful as a CI check.

I've been a bit eager with the amount of tests - can always strip some back if you'd prefer.

Just in case it needs declaring anywhere, a lot of this was 🤖 AI-assisted 🤖

salesforce-cla · 2026-06-07T08:51:28Z

Thanks for the contribution! Before we can merge this, we need @jonapoul to sign the Salesforce Inc. Contributor License Agreement.

ZacSweers

I think there's something useful here but I think your heavy LLM use has resulted in some bespoke code that I'm curious if you can elaborate on. I get that LLMs are used heavily but please try to take a pass at de-fluffing and cleaning up the code after, some things here are just artifacts of its internal reasoning and don't really have obvious explanations here.

ZacSweers · 2026-06-08T01:44:12Z

+   * Whether this call is invoked on a [RESTRICTED_SCOPES] receiver (or a subtype). Covers both
+   * interface members and extension functions, since both report the scope as the receiver type.
+   */
+  private fun UCallExpression.isRestrictedScopeCall(): Boolean {


What's this for?

See my other comment, checking the parent type of the function with the key parameter before we analyse it properly.

Which other comment? You've said what it does but not why it's needed. Please be a bit more detailed, large AI-generated changes like this are going to demand a bit more scrutiny in review

#572 (comment)

It's to make sure that we don't flag on an arbitrary function that happens to take a key parameter without the Bundle-saveability restriction. E.g. the no errors for an unrelated scope with a key parameter test added in this pr. The relevant functions that we're checking are all under these specific scope types - see the above cs.android.com link if you're curious.

ZacSweers · 2026-06-08T01:45:04Z

+  }
+
+  /** The expression whose value a lambda returns (its last statement / explicit return). */
+  private val ULambdaExpression.returnedExpression: UExpression?


Is it not enough to resolve the type of this expression and, if it's a function/lambda type, just look at its return type that way? You're essentially doing implicit return type resolution here

That won't do the trick here because the key lambda has a declared Any return type - we need to explicitly resolve the actual type to pick up the issue.

My question still stands, the inner expression should have a resolved type whose' return is just being upcast to Any right?

Yes, so we have to get the concrete returned type from the inner expression, otherwise nothing would get flagged at all

I'm asking if you can do that by asking UAST/PSI what the resolved type of the body expression is. Currently you are iterating the whole body and doing your own custom implicit return type resolution that I'm wondering if it may be redundant

ZacSweers · 2026-06-08T01:45:57Z

+   * stored selector or reference, where the real key type is out of reach) and `Any`/`Object`
+   * (whose static type says nothing about the runtime value).
+   */
+  private fun PsiType.isSaveable(): Boolean =


I think @stagg has something similar floating around? But also - what about types with custom Savers?

I had taken a run at this type of lint with slackhq/slack-lints#414. The whole check kinda boils down to building a fully accurate Android only runtime check of all the possible types. Feasibility of accurately determining that isn't really (sanely?) possible.

Its likely better to make sure you have a StateRestorationTester based test around the compose code, especially for any custom savers.

As far as I'm aware you can't pass custom savers into lazy list item keys like this. Happy to be overturned on that, if you're aware of a way?

It seems surprising that only implicitly saveable types are ever supported if the docs just say types must be saveable?

ZacSweers · 2026-06-08T01:48:16Z

@@ -28,6 +28,7 @@ class ComposeLintsIssueRegistry : IssueRegistry() {
      *CompositionLocalUsageDetector.ISSUES,
      ContentEmitterReturningValuesDetector.ISSUE,
      ItemKeyHashCodeDetector.ISSUE,


Can this share infra with this ItemKeyHashCodeDetector detector considering they are looking in the same place?

Yeah that's fair, I can take a look at that 👍

ZacSweers · 2026-06-08T01:49:07Z

+     * documenting that restriction; extensions (paging, Wear `expandable*`) are covered
+     * transitively via the shared receiver. Pager is intentionally absent - it has no restriction.
+     */
+    private val RESTRICTED_SCOPES =


Why are scopes relevant for key lambdas?

It's to make sure that we don't flag on an arbitrary function that happens to take a key parameter without the Bundle-saveability restriction. E.g. the no errors for an unrelated scope with a key parameter test added in this pr. The relevant functions that we're checking are all under these specific scope types - see the above cs.android.com link if you're curious.

Sidebar just in case, as it has some indications of it - please don't use AI for comments in code review. Code review is human-to-human

None of the PR comments are AI

ZacSweers · 2026-06-08T01:49:21Z

+  companion object {
+    /**
+     * Receiver types whose `key` carries the "must be saveable via `Bundle`" restriction; a call is
+     * checked only when invoked on one of these (or a subtype). Sourced from every AndroidX scope


Link the source, LLMs cannot be trusted to cite correctly

I linked it in the PR description, did you want it in the class docs too?

https://cs.android.com/search?q=%22should%20be%20saveable%20via%20Bundle%22&sq=

Yes, include it in the docs. Just linking a search query in cs.android.com is also not a real citation

jonapoul

I'll clip back some of the slop comments/code too 👍

jonapoul · 2026-06-08T16:36:39Z

+  companion object {
+    /**
+     * Receiver types whose `key` carries the "must be saveable via `Bundle`" restriction; a call is
+     * checked only when invoked on one of these (or a subtype). Sourced from every AndroidX scope


I linked it in the PR description, did you want it in the class docs too?

https://cs.android.com/search?q=%22should%20be%20saveable%20via%20Bundle%22&sq=

jonapoul · 2026-06-08T16:39:43Z

+     * documenting that restriction; extensions (paging, Wear `expandable*`) are covered
+     * transitively via the shared receiver. Pager is intentionally absent - it has no restriction.
+     */
+    private val RESTRICTED_SCOPES =


It's to make sure that we don't flag on an arbitrary function that happens to take a key parameter without the Bundle-saveability restriction. E.g. the no errors for an unrelated scope with a key parameter test added in this pr. The relevant functions that we're checking are all under these specific scope types - see the above cs.android.com link if you're curious.

jonapoul · 2026-06-08T16:58:28Z

+   * Whether this call is invoked on a [RESTRICTED_SCOPES] receiver (or a subtype). Covers both
+   * interface members and extension functions, since both report the scope as the receiver type.
+   */
+  private fun UCallExpression.isRestrictedScopeCall(): Boolean {


See my other comment, checking the parent type of the function with the key parameter before we analyse it properly.

jonapoul · 2026-06-08T17:04:42Z

+  }
+
+  /** The expression whose value a lambda returns (its last statement / explicit return). */
+  private val ULambdaExpression.returnedExpression: UExpression?


That won't do the trick here because the key lambda has a declared Any return type - we need to explicitly resolve the actual type to pick up the issue.

jonapoul · 2026-06-08T17:08:01Z

+   * stored selector or reference, where the real key type is out of reach) and `Any`/`Object`
+   * (whose static type says nothing about the runtime value).
+   */
+  private fun PsiType.isSaveable(): Boolean =


As far as I'm aware you can't pass custom savers into lazy list item keys like this. Happy to be overturned on that, if you're aware of a way?

jonapoul · 2026-06-08T17:16:27Z

@@ -28,6 +28,7 @@ class ComposeLintsIssueRegistry : IssueRegistry() {
      *CompositionLocalUsageDetector.ISSUES,
      ContentEmitterReturningValuesDetector.ISSUE,
      ItemKeyHashCodeDetector.ISSUE,


Yeah that's fair, I can take a look at that 👍

Added ItemKeyNotSaveableDetector

67177e7

salesforce-cla Bot added the cla:missing label Jun 7, 2026

jonapoul marked this pull request as ready for review June 7, 2026 08:53

ZacSweers reviewed Jun 8, 2026

View reviewed changes

jonapoul commented Jun 8, 2026

View reviewed changes

Conversation

jonapoul commented Jun 7, 2026

Uh oh!

salesforce-cla Bot commented Jun 7, 2026

Uh oh!

ZacSweers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stagg Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonapoul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stagg Jun 8, 2026 •

edited

Loading