Marking method as default in interface considered as breaking change [188691010]

Obsolete

Bug

Status Update

No update yet.

Description

se...@google.com

created issue #1

May 19, 2021 08:17PM

Steps to reproduce:

Mark an existent method as default;
run updateApi;
method that was marked as default disappears from existing classes that implement that interface;

For example in ViewModelProvider.Factory mark a method create as default:

@NonNull
default <T extends ViewModel> T create(@NonNull Class<T> modelClass) {
  throw new UnsupportedOperationException();
}

then run ./gradlew :lifec:lifec-view:updateApi

It will fail with:

/Users/sergeyv/android/androidx-main/frameworks/support/lifecycle/lifecycle-viewmodel/api/2.3.0-beta01.txt:37: error: Removed method androidx.lifecycle.ViewModelProvider.NewInstanceFactory.create(Class<T>) [RemovedMethod]/Users/sergeyv/android/androidx-main/frameworks/support/lifecycle/lifecycle-viewmodel/api/restricted_2.3.0-beta01.txt:37: error: Removed method androidx.lifecycle.ViewModelProvider.NewInstanceFactory.create(Class<T>) [RemovedMethod]
Aborting: Found compatibility problems checking the public API (/Users/sergeyv/android/androidx-main/out/androidx/lifecycle/lifecycle-viewmodel/build/api/current.txt) against the API in /Users/sergeyv/android/androidx-main/frameworks/support/lifecycle/lifecycle-viewmodel/api/2.3.0-beta01.txt

Comments

au...@google.com <au...@google.com> #2May 19, 2021 11:36PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

au...@google.com <au...@google.com> #3May 19, 2021 11:53PM

@sergeyv any idea what I'm missing here?

se...@google.com <se...@google.com> #4Jun 25, 2021 01:25PM

Is it fine that your source file for SecondClass doesn't contain implementation?

 public class SecondClass extends FirstClass {
     @Override
     public <T extends String> T create(Class<T> modelClass); // implement me?
 }

au...@google.com <au...@google.com> #5Jul 8, 2021 09:54PM

I tried to go even closer to source for the test, but no luck

    @Test
    fun `Adding default modifier to a method`() {
        check(
            expectedIssues = "",
            format = FileFormat.V4,
            checkCompatibilityApi = """
                // Signature format: 4.0
                package test.pkg {
                  public abstract class ViewModel {
                    ctor public ViewModel();
                  }
                  public class ViewModelProvider {
                    ctor public ViewModelProvider();
                  }
                  public static interface ViewModelProvider.Factory {
                    method public <T extends test.pkg.ViewModel> T! create(Class<T!>!);
                  }
                  public static class ViewModelProvider.NewInstanceFactory implements test.pkg.ViewModelProvider.Factory {
                    ctor public ViewModelProvider.NewInstanceFactory();
                    method public <T extends test.pkg.ViewModel> T! create(Class<T!>!);
                  }
                }
                """,
            sourceFiles = arrayOf(
                java("""
                    package test.pkg;
                    public class ViewModelProvider {
                        public interface Factory {
                            default <T extends ViewModel> T create(Class<T> modelClass) {
                                throw new UnsupportedOperationException();
                            }
                        }
                        
                        public static class NewInstanceFactory implements Factory {
                            @Override
                            public <T extends ViewModel> T create(Class<T> modelClass) {
                                return null;
                            }
                        }
                    }
                """),
                java("""
                    package test.pkg;
                    public abstract class ViewModel {
                    }
                """)
            )
        )
    }

au...@google.com <au...@google.com> #6Jul 8, 2021 10:04PM

Status: Won't Fix (Obsolete)

It must have been a bug in old metalava, i'm unable to repro in androidx either now (I reverted 927300ff9fd87da629f28a56058b29750700c845 and ce18bf219466f2453cebb2a20bc4cef23a6a89bd, then changed ViewModelProvider.Factory to default method.

ap...@google.com <ap...@google.com> #7Sep 26, 2021 08:30PM

Project: platform/frameworks/support
Branch: androidx-main

commit 15d5a9df61e3b6f362046b29412913ef3aec10e0
Author: Sergey Vasilinets <sergeyv@google.com>
Date: Fri Jun 25 14:30:22 2021

Introduce CreationExtras

It also passes key associated with ViewModel
as an extra.

Metalava incorrectly says that this is a breaking change:

b/188691010

Relnote: "ViewModelProvider.CreationExtras were introduced.
This API will simplify passing an additional information to ViewModelProvider.Factory"
bug: 188541057
Test: ViewModelProviderTest

Change-Id: Ia73439cb2282609a9a1eaebf8ba79b9cc93feb7c

M lifecycle/lifecycle-viewmodel/api/current.txt
M fragment/fragment/build.gradle
A lifecycle/lifecycle-viewmodel/src/main/java/androidx/lifecycle/viewmodel/CreationExtras.kt
M lifecycle/lifecycle-viewmodel/build.gradle
M lifecycle/lifecycle-viewmodel-savedstate/api/current.txt
M lifecycle/lifecycle-viewmodel/api/restricted_current.txt
M fragment/fragment-testing/build.gradle
M compose/integration-tests/docs-snippets/build.gradle
M lifecycle/lifecycle-viewmodel-compose/build.gradle
M lifecycle/lifecycle-viewmodel-savedstate/api/public_plus_experimental_current.txt
M fragment/fragment-ktx/build.gradle
M activity/activity-ktx/build.gradle
A lifecycle/lifecycle-viewmodel-savedstate/api/current.ignore
M lifecycle/lifecycle-viewmodel-savedstate/build.gradle
M navigation/navigation-runtime/build.gradle
M navigation/navigation-dynamic-features-fragment/build.gradle
M lifecycle/lifecycle-viewmodel/src/test/java/androidx/lifecycle/ViewModelProviderTest.java
M lifecycle/lifecycle-viewmodel/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-viewmodel/src/main/java/androidx/lifecycle/ViewModelProvider.kt

https://android-review.googlesource.com/1748315

Issue 188691010