Skip to content

Latest Dart roll blocked by deprecated code in the engine #137054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
flar opened this issue Oct 23, 2023 · 12 comments
Closed

Latest Dart roll blocked by deprecated code in the engine #137054

flar opened this issue Oct 23, 2023 · 12 comments
Assignees
Labels
dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@flar
Copy link
Contributor

flar commented Oct 23, 2023

See: flutter/engine#47175

Failure at:

Analyzing ui...

   info - platform_dispatcher.dart:76:31 - 'UnmodifiableByteDataView' is deprecated and shouldn't be used. Use ByteData.asUnmodifiableView() instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
@flar flar added the engine flutter/engine repository. See also e: labels. label Oct 23, 2023
@dam-ease dam-ease added the team-engine Owned by Engine team label Oct 23, 2023
@zanderso zanderso added dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression and removed engine flutter/engine repository. See also e: labels. labels Oct 23, 2023
@zanderso
Copy link
Member

@lrhn
Copy link
Contributor

lrhn commented Oct 23, 2023

Flutter makes it impossible to deprecate something and introduce the replacement in the same CL.

Need to first introduce the replacement, then migrate Flutter, and then deprecate.
(Or, prepare a CL to land in Flutter at the same time, which does the migration. Needs more cooperation.)

@derekxu16
Copy link
Contributor

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 23, 2023
This reverts commit b657773.

Reason for revert: blocking Dart SDK -> Engine roll (flutter/flutter#137054)

Original change's description:
> [typed_data] Deprecate UnmodifiableUint8ListView and friends
>
> This is the first of several steps to remove the unmodifiable views for typed data classes. The end goal is that dart2js has only one class implementing `Uint8List` so that `Uint8List` accesses can always be compiled down to JavaScript code that directly uses indexed property accesses (`a[i]`).
>
> This first step deprecates the unmodifiable view classes to help prevent their use in new code, and adds `asUnmodifiableView()` methods as a replacement for the small number of places that use the classes.
>
> The next steps (see #53785) are to remove uses of the unmodifiable view classes from the SDK. Once this is complete the classes themselves can be removed.
>
> TEST=ci
>
> Issue: #53218
> Issue: #53785
>
> Change-Id: I04d4feb0d9f1619e6eee65236e559f5e6adf2661
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321922
> Reviewed-by: Nicholas Shahan <[email protected]>
> Reviewed-by: Lasse Nielsen <[email protected]>
> Commit-Queue: Stephen Adams <[email protected]>
> Reviewed-by: Alexander Markov <[email protected]>
> Reviewed-by: Martin Kustermann <[email protected]>
> Reviewed-by: Ömer Ağacan <[email protected]>

Issue: #53218
Issue: #53785
Change-Id: I0bb042269f9ff8e5cd69619cf97b60c79ea98cbf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331680
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Derek Xu <[email protected]>
@flar
Copy link
Contributor Author

flar commented Oct 23, 2023

This looks like a 1-line change, can't we just fix the 1 line and let the Dart roll through?

Or do we have evidence the change will impact more than 1 line in the framework?

Never mind, I see the reports of other failures. They are already being patched, though.

@rakudrama
Copy link

I am going to revert the revert, but without the deprecation annotations.

Then someone can change the Flutter engine to use the new API.
Who is doing this?

@chinmaygarde
Copy link
Member

xref go/137054-chat

@rakudrama
Copy link

This is the reapplication of the reverted CL with the @Deprecated annotations commented-out: https://dart-review.googlesource.com/c/sdk/+/331741

Once that lands it will be possible to change

ByteData? _wrapUnmodifiableByteData(ByteData? byteData) =>
    byteData == null ? null : UnmodifiableByteDataView(byteData);

to

ByteData? _wrapUnmodifiableByteData(ByteData? byteData) =>
    byteData?.asUnmodifiableView();

@flar
Copy link
Contributor Author

flar commented Oct 23, 2023

Here is the list of Unmodifiable hits currently in the our repos:

List of `Unmodifiable` references in the engine repo
% git grep Unmodifiable
lib/ui/fixtures/ui_test.dart:    if (result is UnmodifiableByteDataView &&
lib/ui/platform_dispatcher.dart:ByteData? _wrapUnmodifiableByteData(ByteData? byteData) =>
lib/ui/platform_dispatcher.dart:    byteData == null ? null : UnmodifiableByteDataView(byteData);
lib/ui/window/platform_message_response_dart.cc:          byte_buffer = Dart_NewUnmodifiableExternalTypedDataWithFinalizer(
lib/ui/window/platform_message_response_dart.cc:                                        "_wrapUnmodifiableByteData"),
shell/platform/fuchsia/dart-pkg/zircon/test/zircon_tests.dart:      Uint8List fileData = UnmodifiableUint8ListView(mapResult.data);
shell/platform/fuchsia/dart-pkg/zircon/test/zircon_tests.dart:      Uint8List vmoData = UnmodifiableUint8ListView(mapResult.data);
List of `Unmodifiable` references in the framework repo
% git grep Unmodifiable
packages/flutter_tools/lib/src/android/application_package.dart:      UnmodifiableMapView<String, Map<String, String>>(_data);
packages/flutter_tools/lib/src/base/context.dart:            UnmodifiableListView<Type>(_reentrantChecks!.sublist(index)));

@chinmaygarde chinmaygarde added the triaged-engine Triaged by Engine team label Oct 23, 2023
@rakudrama
Copy link

Here is the list of Unmodifiable hits currently in the our repos:

Thanks for the lists.

List of Unmodifiable references in the engine repo

% git grep Unmodifiable
lib/ui/fixtures/ui_test.dart:    if (result is UnmodifiableByteDataView &&

This needs to be changed. Perhaps to a try-catch containing an attempted write of a byte that was accessed from the result.

lib/ui/platform_dispatcher.dart:ByteData? _wrapUnmodifiableByteData(ByteData? byteData) =>
lib/ui/platform_dispatcher.dart:    byteData == null ? null : UnmodifiableByteDataView(byteData);

The migration for this is described two messages above.

lib/ui/window/platform_message_response_dart.cc:          byte_buffer = Dart_NewUnmodifiableExternalTypedDataWithFinalizer(
lib/ui/window/platform_message_response_dart.cc:                                        "_wrapUnmodifiableByteData"),

These should work unchanged.

shell/platform/fuchsia/dart-pkg/zircon/test/zircon_tests.dart:      Uint8List fileData = UnmodifiableUint8ListView(mapResult.data);
shell/platform/fuchsia/dart-pkg/zircon/test/zircon_tests.dart:      Uint8List vmoData = UnmodifiableUint8ListView(mapResult.data);

Change these to = mapResult.data.asUnmodifiableView();

List of Unmodifiable references in the framework repo

% git grep Unmodifiable
packages/flutter_tools/lib/src/android/application_package.dart:      UnmodifiableMapView<String, Map<String, String>>(_data);
packages/flutter_tools/lib/src/base/context.dart:            UnmodifiableListView<Type>(_reentrantChecks!.sublist(index)));

These wrapper views are unrelated to the deprecated typed_data classes. No change needed.

@rakudrama
Copy link

CL available: flutter/engine#47276

@flar
Copy link
Contributor Author

flar commented Oct 24, 2023

Fixed in flutter/engine#47276

@flar flar closed this as completed Oct 24, 2023
Copy link

github-actions bot commented Nov 7, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests

7 participants