Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@srujzs
Copy link
Contributor

@srujzs srujzs commented May 10, 2023

@staticInterop members will start disallowing tear-offs, so this member should turn into a closure.

The static check wasn't added in time, so this is modifying the source code again.

ee3ce32 was the original change, but the static error didn't make it into the Dart SDK, so this is fixing another tear-off.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • [] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@srujzs srujzs requested a review from eyebrowsoffire May 10, 2023 04:18
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label May 10, 2023
@flutter-dashboard
Copy link

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.

Copy link
Contributor

@Hixie Hixie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test-exempt: code refactor with no semantic change

LGTM

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a small comment that avoids the lint override. Will static interop methods ever be valid as tearoffs? It bothers me a bit that the lint and this limitation are in conflict.

fontFaces.forEach(domDocument.fonts!.add);
// Since we can't use tear-offs for interop members, this code is faster
// and easier to read with a for loop instead of forEach.
// ignore: prefer_foreach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid this lint override, you could instead do:

fontFaces.forEach((DomFontFace fontFace) => domDocument.fonts!.add(fontFace));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blech. Well then just ignore my comment for now. If we will eventually get tearoffs for static interop (you could even just have a kernel transform emit a closure like this for static interop tearoffs) maybe we should put a TODO here so that we can remove the lint override?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I read your more detailed comment, thanks for the context. I was imagining dynamic tearoff creation but I didn't think about tearoff equality... hmmm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general these lint rules are awfully opinionated about things that don't seem to matter all that much 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, even if we were to ignore tearoff equality, the semantics is inconsistent between a regular call and a tear-off call, so in cases where optionality matters, users will still have to battle these lints.

I agree that there shouldn't be a lint for a regular for-loop, but that's my personal opinion. :)

@srujzs
Copy link
Contributor Author

srujzs commented May 10, 2023

Will static interop methods ever be valid as tearoffs? It bothers me a bit that the lint and this limitation are in conflict.

The complication around supporting tearoffs is that

  1. The semantics are at odds with a regular call. Imagine a static interop method has positional arguments. We can lower regular invocations so that only the passed in arguments are in the final JS call. We cannot do that with a tear-off as we don't know how it's invoked, so we have to include every possible argument. So if you had external void foo(int a, [int? b]), foo(0) will give you foo(0) in JS, but (foo)(0) gives you foo(0, null) in JS.
  1. We generate a node in the AST that gives you the tear-off. This adds code size for every interop member in DDC that can instead be not emitted. We could work around this by generating a tear-off dynamically, but this fails tear-off equality semantics.

I agree that it's a bit weird that the lint is noisy here. We can't really do your suggestion as it comes across another lint (https://dart.dev/tools/linter-rules#avoid_function_literals_in_foreach_calls). I'll have to think more about what code here should look like, but I don't know if this particular case can avoid all lints.

@gaaclarke
Copy link
Member

gaaclarke commented May 10, 2023

It's my understanding that Linux linux_license is the engine_v2 recipe which has been reverted. I'm going to attempt to rebase to see if we get the Linux license bot to execute (which I think doesn't have the problem we are seeing with the rom files).

edit: the revert pr is #41903

@staticInterop members will start disallowing tear-offs, so this
member should turn into a closure.

The static check wasn't added in time, so this is modifying the
source code again.
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
@auto-submit auto-submit bot merged commit 28715a6 into flutter:main May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request May 11, 2023
…6554)

Manual roll requested by [email protected]

flutter/engine@a0925f1...f38f46f

2023-05-11 [email protected] Add pkg:protobuf as a dependency for
testing/dart (flutter/engine#41938)
2023-05-11 [email protected] Roll Skia from 0c979da775be to
ccf73af6ca91 (1 revision) (flutter/engine#41937)
2023-05-11 [email protected] Roll Skia from a12a999cbd09 to
0c979da775be (1 revision) (flutter/engine#41935)
2023-05-11 [email protected] Roll Skia from 9cbc2c578e10 to
a12a999cbd09 (2 revisions) (flutter/engine#41933)
2023-05-11 [email protected] Roll Skia from 1a29bd9a0147 to
9cbc2c578e10 (10 revisions) (flutter/engine#41932)
2023-05-11 [email protected] Roll Fuchsia Mac SDK from
a3rrULFaXlwS8mfjW... to 2tQjI0g3aDmjHAtMw... (flutter/engine#41931)
2023-05-11 [email protected] Return fonts in a deterministic
order. (flutter/engine#41907)
2023-05-11 [email protected] Turn @staticInterop
tear-off into closure (flutter/engine#41885)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from a3rrULFaXlwS to 2tQjI0g3aDmj

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…tter#126554)

Manual roll requested by [email protected]

flutter/engine@a0925f1...f38f46f

2023-05-11 [email protected] Add pkg:protobuf as a dependency for
testing/dart (flutter/engine#41938)
2023-05-11 [email protected] Roll Skia from 0c979da775be to
ccf73af6ca91 (1 revision) (flutter/engine#41937)
2023-05-11 [email protected] Roll Skia from a12a999cbd09 to
0c979da775be (1 revision) (flutter/engine#41935)
2023-05-11 [email protected] Roll Skia from 9cbc2c578e10 to
a12a999cbd09 (2 revisions) (flutter/engine#41933)
2023-05-11 [email protected] Roll Skia from 1a29bd9a0147 to
9cbc2c578e10 (10 revisions) (flutter/engine#41932)
2023-05-11 [email protected] Roll Fuchsia Mac SDK from
a3rrULFaXlwS8mfjW... to 2tQjI0g3aDmjHAtMw... (flutter/engine#41931)
2023-05-11 [email protected] Return fonts in a deterministic
order. (flutter/engine#41907)
2023-05-11 [email protected] Turn @staticInterop
tear-off into closure (flutter/engine#41885)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from a3rrULFaXlwS to 2tQjI0g3aDmj

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants