[plugin_platform_interface] Switch `PlatformInterface._instanceToken` to an expando. #6411

stereotype441 · 2022-09-12T16:10:14Z

This change replaces PlatformInterface._instanceToken (an instance field that points from a PlatformInterface to its corresponding token) with an expando that maps from PlatformInterface to Object. There is no change to the public API, and no change to the behavior of users' production code.

This change ensures that if a customer tries to implement PlatformInterface using implements rather than extends, the code in PlatformInterface._verify won't try to access the private _instanceToken field in PlatformInterface. This is important because an upcoming change to the dart language is going to cause such accesses to throw exceptions rather than deferring to noSuchMethod (see dart-lang/language#2020 for details).

Fixes flutter/flutter#109339.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

This change replaces `PlatformInterface._instanceToken` (an instance field that points from a `PlatformInterface` to its corresponding token) with an expando that maps from `PlatformInterface` to `Object`. There is no change to the public API, and no change to the behavior of users' production code. This change ensures that if a customer tries to implement `PlatformInterface` using `implements` rather than `extends`, the code in `PlatformInterface._verify` won't try to access the private `_instanceToken` field in `PlatformInterface`. This is important because an upcoming change to the dart language is going to cause such accesses to throw exceptions rather than deferring to `noSuchMethod` (see dart-lang/language#2020 for details). Fixes flutter/flutter#109339.

flutter-dashboard · 2022-09-12T16:10:17Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

stuartmorgan-g · 2022-09-12T16:26:09Z

packages/plugin_platform_interface/lib/plugin_platform_interface.dart


-  final Object? _instanceToken;
+  /// Expando mapping instances of PlatformInterface to their associated tokens.
+  /// The reason we don't simply use a private field of type `Object?` is


Nit: could you adjust this comment (here and below) to remove the use of "we"? I can't actually tell who "we" is in this context (see go/avoidwe).

stuartmorgan-g

Thanks! Just some minor comments on the explanation comment.

stuartmorgan-g · 2022-09-12T16:27:52Z

packages/plugin_platform_interface/lib/plugin_platform_interface.dart

+  /// Expando mapping instances of PlatformInterface to their associated tokens.
+  /// The reason we don't simply use a private field of type `Object?` is
+  /// because as part of implementing
+  /// https://github.com/dart-lang/language/issues/2020, it will soon become a


This comment will outlive the work, so referring to "soon" will soon be confusing.

packages/plugin_platform_interface/lib/plugin_platform_interface.dart

stereotype441 · 2022-09-12T16:45:20Z

@stuartmorgan, given that this change only affects the behavior of some corner cases in tests, I'm unsure whether I need:

a CHANGELOG entry
a version bump
tests

What do you think?

stuartmorgan-g · 2022-09-12T17:00:51Z

@stuartmorgan, given that this change only affects the behavior of some corner cases in tests, I'm unsure whether I need:

a CHANGELOG entry

a version bump

It's changing production code (and will actually change behavior for clients), so needs both.

tests

Since we already have tests that would break later without this change, I think it's covered; I would ping Hixie for an exemption on this.

stereotype441 · 2022-09-12T17:19:35Z

@stuartmorgan, given that this change only affects the behavior of some corner cases in tests, I'm unsure whether I need:

a CHANGELOG entry

a version bump

It's changing production code (and will actually change behavior for clients), so needs both.

tests

Since we already have tests that would break later without this change, I think it's covered; I would ping Hixie for an exemption on this.

Thanks! I've added a CHANGELOG entry and bumped the version; I'll ping Hixie now.

Hixie · 2022-09-12T18:14:48Z

Do we know of anyone who uses implements? Can't we just have them switch to extends instead? Expando logic is hard to understand and keeping the code simple seems very preferable unless the cost of migrating people is disproportionally large.

stuartmorgan-g · 2022-09-12T20:03:02Z

Do we know of anyone who uses implements? Can't we just have them switch to extends instead?

The point of this logic is to enforce that people don't make implementations of platform interfaces of federated plugins that use implements, because if they do it causes serious problems. As happened with webview, for instance, which predated this logic and was never updated to use it, so the mistake wasn't caught until it was too late (which is the cause of all of those PRs that are blocked in flutter/plugins that you keep finding in stale issue triage).

Expando logic is hard to understand and keeping the code simple seems very preferable unless the cost of migrating people is disproportionally large.

This allows us to throw a controlled error that matches our docs and has clear actionable instructions, instead of throwing something different in different cases for subtle language implementation reasons. I think having code that almost nobody ever needs to interact with be more complex is a reasonable tradeoff here in order to have a stable, actionable error for the many, many potential clients of this logic.

Hixie · 2022-09-14T20:00:55Z

Ah, that was not clear from the original commit message or changelog entry. I definitely support the goal here. I just wish there was a cleaner (read, less Expando-magical way) to do it...

packages/plugin_platform_interface/lib/plugin_platform_interface.dart

Hixie · 2022-09-14T21:03:42Z

I think this is worth testing, FWIW. Verifying that we give good clear error messages for things like this is valuable.

stereotype441 · 2022-09-15T13:44:50Z

I think this is worth testing, FWIW. Verifying that we give good clear error messages for things like this is valuable.

Test added!

stuartmorgan-g

LGTM; one optional test change.

packages/plugin_platform_interface/test/plugin_platform_interface_test.dart

…anceToken` to an expando. (flutter/plugins#6411)

… to an expando. (flutter#6411) This change replaces `PlatformInterface._instanceToken` (an instance field that points from a `PlatformInterface` to its corresponding token) with an expando that maps from `PlatformInterface` to `Object`. There is no change to the public API, and no change to the behavior of users' production code. This change ensures that if a customer tries to implement `PlatformInterface` using `implements` rather than `extends`, the code in `PlatformInterface._verify` won't try to access the private `_instanceToken` field in `PlatformInterface`. This is important because an upcoming change to the dart language is going to cause such accesses to throw exceptions rather than deferring to `noSuchMethod` (see dart-lang/language#2020 for details). Fixes flutter/flutter#109339.

stereotype441 requested a review from stuartmorgan-g as a code owner September 12, 2022 16:10

flutter-dashboard bot added the needs tests label Sep 12, 2022

github-actions bot added the p: plugin_platform_interface label Sep 12, 2022

stuartmorgan-g reviewed Sep 12, 2022

View reviewed changes

stuartmorgan-g suggested changes Sep 12, 2022

View reviewed changes

Improved comment

8bdf367

Update CHANGELOG and bump version

afa48e9

Hixie reviewed Sep 14, 2022

View reviewed changes

packages/plugin_platform_interface/lib/plugin_platform_interface.dart Show resolved Hide resolved

Add a test

e27a138

stuartmorgan-g approved these changes Sep 15, 2022

View reviewed changes

packages/plugin_platform_interface/test/plugin_platform_interface_test.dart Show resolved Hide resolved

stuartmorgan-g removed the needs tests label Sep 15, 2022

Fix formatting

8c6d3f2

stereotype441 merged commit feed66b into flutter:main Sep 15, 2022

stereotype441 deleted the b-2020-61 branch September 15, 2022 16:53

engine-flutter-autoroll mentioned this pull request Sep 16, 2022

Roll Plugins from 64a2931d7f8e to a6bf3b6332d3 (4 revisions) flutter/flutter#111746

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2022

feed66b87 [plugin_platform_interface] Switch `PlatformInterface._inst…

e184c35

…anceToken` to an expando. (flutter/plugins#6411)

[plugin_platform_interface] Switch PlatformInterface._instanceToken to an expando. #6411

[plugin_platform_interface] Switch PlatformInterface._instanceToken to an expando. #6411

Uh oh!

Conversation

stereotype441 commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Sep 12, 2022

Uh oh!

stuartmorgan-g Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

stereotype441 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

stereotype441 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stereotype441 commented Sep 12, 2022

Uh oh!

stuartmorgan-g commented Sep 12, 2022

Uh oh!

stereotype441 commented Sep 12, 2022

Uh oh!

Hixie commented Sep 12, 2022

Uh oh!

stuartmorgan-g commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Sep 14, 2022

Uh oh!

Uh oh!

Hixie commented Sep 14, 2022

Uh oh!

stereotype441 commented Sep 15, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[plugin_platform_interface] Switch `PlatformInterface._instanceToken` to an expando. #6411

[plugin_platform_interface] Switch `PlatformInterface._instanceToken` to an expando. #6411

stereotype441 commented Sep 12, 2022 •

edited

Loading

stuartmorgan-g commented Sep 12, 2022 •

edited

Loading