Skip to content

Conversation

@tarrinneal
Copy link
Contributor

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM.

Did you audit the other generators to make sure none of them have this same bug in the other direction (for calling a FlutterApi with an int, and no custom types anywhere)?

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 28, 2024
@auto-submit auto-submit bot merged commit 27c9853 into flutter:main Sep 28, 2024
@tarrinneal
Copy link
Contributor Author

LGTM.

Did you audit the other generators to make sure none of them have this same bug in the other direction (for calling a FlutterApi with an int, and no custom types anywhere)?

Yeah, the change to handling ints is only a dart change so the logic doesn't exist in the other generators

@EchoEllet
Copy link
Contributor

EchoEllet commented Sep 30, 2024

It seem that this part causing Flutter analysis failure:

class _PigeonCodec extends StandardMessageCodec {
  const _PigeonCodec();
  @override
  void writeValue(WriteBuffer buffer, Object? value) {
    if (value is int) {
      buffer.putUint8(4);
      buffer.putInt64(value); // Unnecessary duplication of receiver. Try using a cascade to avoid the duplication
    } else {
      super.writeValue(buffer, value);
    }
  }

Should be updated to:

buffer..putUint8(4)
      ..putInt64(value);

This version was released as a minor version, pub always uses the latest available minor version and it causes analysis failure, maybe this should be changed to satisfy Dart analysis or ignore all lint rules in the top of the file, or maybe provide the option to?

Since at the moment, we can't directly depend on dart run pigeon --input ./pigeons/messages.dart, instead we have to use our own command which runs dart fix for that generated file to fix them all.

@stuartmorgan-g
Copy link
Collaborator

Should be updated to

This PR has already landed, and cannot be updated. The place to report issues with code that has already landed is the issue tracker.

it causes analysis failure [...] maybe this should be changed to satisfy Dart analysis

That depends on your project's analysis settings; Dart has many optional analysis options, some of which are contradictory. We do not (and cannot, due to the contradictory settings) guarantee that all Pigeon-generated code will pass every possible set of analysis options.

or ignore all lint rules in the top of the file, or maybe provide the option to?

You can put arbitrary text in the copyrightHeader file, including ignore options.

@EchoEllet
Copy link
Contributor

That depends on your project's analysis settings; Dart has many optional analysis options, some of which are contradictory. We do not (and cannot, due to the contradictory settings) guarantee that all Pigeon-generated code will pass every possible set of analysis options.

You're right, it seems that I have missed that the cascade_invocations is not part of the recommended nor the core lints provided by lints and flutter_lints.

So an issue specific to a project and not part of this repo.

Thank you for clarifying.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 30, 2024
flutter/packages@0321757...27c9853

2024-09-28 [email protected] [pigeon] fix int bug (flutter/packages#7725)
2024-09-28 [email protected] [pigeon] update deprecated command in README of the example (flutter/packages#7709)
2024-09-27 [email protected] [google_maps_flutter] Fix incorrect comment: Change "marker" to "polyline" (flutter/packages#7664)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
thejitenpatel pushed a commit to thejitenpatel/flutter that referenced this pull request Oct 1, 2024
flutter/packages@0321757...27c9853

2024-09-28 [email protected] [pigeon] fix int bug (flutter/packages#7725)
2024-09-28 [email protected] [pigeon] update deprecated command in README of the example (flutter/packages#7709)
2024-09-27 [email protected] [google_maps_flutter] Fix incorrect comment: Change "marker" to "polyline" (flutter/packages#7664)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [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://issues.skia.org/issues/new?component=1389291&template=1850622

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

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: pigeon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[pigeon] Regression in number handing on Android

3 participants