Feature request: Update MergeAdapter.Config.Builder to have same defaults as MergeAdapter.Config.DEFAULT [157169835]

Fixed

Feature Request

Status Update

No update yet.

Description

st...@gmail.com

created issue #1

May 21, 2020 01:43AM

androidx.recyclerview:recyclerview:1.2.0-alpha03

First of all, thank you for the new MergeAdapter! This is my first time using and benefiting from it.

When I create a merge adapter configuration via MergeAdapter.Config.Builder().build(), I receive a configuration with Config::isolateViewTypes set to false. The value of this same field in MergeAdapter.Config::DEFAULT is true.

This violated the principle of least surprise for me. I was evolving code that initially used "no" (= the default) configuration:

mAdapter = MergeAdapter(adapter1, adapter2)

I wanted to update this code to change the stable ID mode. I tried to achieve that by building and passing in a custom configuration:

mAdapter = MergeAdapter(
    MergeAdapter.Config.Builder()
        .setStableIdMode(ISOLATED_STABLE_IDS)
        .build(),
    adapter1,
    adapter2
)

but this temporarily broke my code because view type isolation changed compared to the initial code. I instead needed to explicitly write

mAdapter = MergeAdapter(
    MergeAdapter.Config.Builder()
        .setIsolateViewTypes(true)
        .setStableIdMode(ISOLATED_STABLE_IDS)
        .build(),
    adapter1,
    adapter2
)

to retain default view type isolation behavior.

Please evaluate whether it would be less surprising to initialize

private boolean mIsolateViewTypes = true;

in MergeAdapter.Config::Builder instead!

Comments

st...@gmail.com <st...@gmail.com> #2May 21, 2020 01:47AM

Could you look into it @christofferqa?

The issue is with this class:

class MySpi : Spi {
    @Volatile
    private var myApi: Api? = null

    // Incorrectly rewritten to return null;
    override fun createApi(): Api {
        return myApi ?: synchronized(this) {
            myApi ?: MyApi().also { myApi = it }
        }
    }
}

The error seems to be that the ArgumentPropagator believes that the fieldState of myApi is ExactDynamicType(@NotNull MyApi) while it should be ExactDynamicType(@Nullable MyApi)

It then removes the first branch (assuming myApi is not null):

return myApi ?: synchronized(this) {
            myApi ?: MyApi().also { myApi = it }
        }

is rewritten into:

return myApi

And then it eventually concludes that myApi is always null, replacing the code by

return null

The ArgumentPropagatorCodeScanner analyzes the assignment myApi = it and correctly set join the state to ExactDynamicType(@NotNull MyApi)

However the ArgumentPropagator fails to understand that myApi can be read before being written in the method, relying on the default null value, so it fails to join null to the dynamic type.

sh...@google.com <sh...@google.com> #3Feb 22, 2021 11:25PM

Reassigned to yb...@google.com.

Yes I can take a look. Would it be possible for you to add a regression test of this issue?

yb...@google.com <yb...@google.com> #4Feb 23, 2021 12:59AM

Project: r8
Branch: main
Author: Christoffer Adamsen <christofferqa@google.com>
Link: https://r8-review.googlesource.com/101407

Reproduce field propagation issue with ServiceLoader

Expand for full commit details

Reproduce field propagation issue with ServiceLoader 
 
Bug: b/389737060 
Change-Id: Id850448f68b999194bc0380dfca625f4f68bc31b

Files:

A src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/DefaultFieldValueJoinerWithServiceLoaderTest.java

Hash: 6ba9af34b15f2a0d3fe356f6e7a5a4071128aed9
Date: Wed Feb 12 09:31:01 2025

yb...@google.com <yb...@google.com> #5Mar 16, 2021 11:49PM

Project: r8
Branch: main
Author: Christoffer Adamsen <christofferqa@google.com>
Link: https://r8-review.googlesource.com/101408

Account for ServiceLoaders in field value propagation

Expand for full commit details

Account for ServiceLoaders in field value propagation 
 
Fixes: b/389737060 
Change-Id: Ide438a8e1b0541f3ce2385087f06f8b9909ab3c5

Files:

M src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
M src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/DefaultFieldValueJoinerWithServiceLoaderTest.java

Hash: 692fff07b70f6ee5ac520b179aed5e125a0d4848
Date: Wed Feb 12 09:34:04 2025

yb...@google.com <yb...@google.com> #6Mar 17, 2021 03:46PM

Marked as fixed.

Project: r8
Branch: 8.9
Author: Christoffer Adamsen <christofferqa@google.com>
Link: https://r8-review.googlesource.com/101512

Account for ServiceLoaders in field value propagation

Expand for full commit details

Account for ServiceLoaders in field value propagation 
 
Fixes: b/389737060 
Change-Id: Ide438a8e1b0541f3ce2385087f06f8b9909ab3c5

Files:

M src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
M src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/DefaultFieldValueJoinerWithServiceLoaderTest.java

Hash: 76cad195cf43c16aa9e7b0203e51662036587e71
Date: Thu Feb 13 10:33:29 2025

st...@gmail.com <st...@gmail.com> #7Mar 17, 2021 04:05PM

Project: r8
Branch: 8.9
Author: Christoffer Adamsen <christofferqa@google.com>
Link: https://r8-review.googlesource.com/101522

Version 8.9.28

Expand for full commit details

Version 8.9.28 
 
Bug: b/389737060 
Change-Id: I42c8d3194dc053e3a16eef8ea1c4b3aced6752d6

Files:

M src/main/java/com/android/tools/r8/Version.java

Hash: 668c1778e857c1bc8e3f9308a65ddc01a6c14898
Date: Thu Feb 13 10:33:34 2025

Issue 157169835

Description

Issue summary

Comments

st...@gmail.com <st...@gmail.com> #2May 21, 2020 01:47AM

sh...@google.com <sh...@google.com> #3Feb 22, 2021 11:25PM

yb...@google.com <yb...@google.com> #4Feb 23, 2021 12:59AM

yb...@google.com <yb...@google.com> #5Mar 16, 2021 11:49PM

yb...@google.com <yb...@google.com> #6Mar 17, 2021 03:46PM

st...@gmail.com <st...@gmail.com> #7Mar 17, 2021 04:05PM

Add comment

Issue metadata