Skip to content
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

Bump lower Dart SDK constraints to 3.0 & add class modifiers #122546

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Mar 13, 2023

Part of #118837.

This will allow us to use the latest Dart 3.0 features.

This PR also adds class modifiers where necessary:

  • to all classes identified as mixin-able as part of the analysis in http://flutter.dev/go/mixin-modifier
  • subclasses of LinkedListEntry (a newly marked base class) to make the analyzer warnings below happy
  • slight adjustments to how TapRegionRegistry is used to fix analyzer warnings
  error • The type '_OverlayEntryLocation' must be 'base', 'final' or 'sealed' because the supertype 'LinkedListEntry' is 'base' • packages/flutter/lib/src/widgets/overlay.dart:1641:7 • subtype_of_base_or_final_is_not_base_final_or_sealed
  error • The type '_RenderDeferredLayoutBox' must be 'base', 'final' or 'sealed' because the supertype 'LinkedListEntry' is 'base' • packages/flutter/lib/src/widgets/overlay.dart:1957:7 • subtype_of_base_or_final_is_not_base_final_or_sealed
  error • The type '_ListenerEntry' must be 'base', 'final' or 'sealed' because the supertype 'LinkedListEntry' is 'base' • packages/flutter/lib/src/widgets/scroll_notification_observer.dart:38:7 • subtype_of_base_or_final_is_not_base_final_or_sealed

Blocked on waiting for flutter/engine#40178 to roll into the framework.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 13, 2023
@goderbauer goderbauer mentioned this pull request Mar 13, 2023
23 tasks
@goderbauer
Copy link
Member Author

FYI @jakemac53 @chingjun

@goderbauer goderbauer changed the title Bump lower Dart SDK constraints to 3.0 & add mixin modifiers Bump lower Dart SDK constraints to 3.0 & add class modifiers Mar 13, 2023
@flutter-dashboard flutter-dashboard bot added the f: scrolling Viewports, list views, slivers, etc. label Mar 13, 2023
@goderbauer
Copy link
Member Author

@LongCatIsLooong Do the changes to overlay.dart look good to you?

@gspencergoog Do the changes in tap_region.dart look good to you to keep TapRegionRegistry as an interface instead of making it mixin-able?

@flutter-dashboard flutter-dashboard bot added the f: routes Navigator, Router, and related APIs. label Mar 13, 2023
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

YES! The overlay.dart changes LGTM. Pattern matching and record are still behind the experimental flag right?

@goderbauer
Copy link
Member Author

Pattern matching and record are still behind the experimental flag right?

Once this PR lands we're good to use them in the framework. This PR is still blocked on waiting for Dart v3.0.0-325.0.dev to roll into the engine & framework, though. That Dart version flips the flag on these features.

@jakemac53
Copy link
Contributor

cc @johnniwinther it sounds like we should hold off on landing this until exhaustiveness checking is finished? What is the status and is there an issue we can track?

@goderbauer
Copy link
Member Author

I am already getting a bunch of exhaustive_cases warnings on this PR (see failing checks).

@johnniwinther
Copy link

@goderbauer Note that the errors you see currently are from a temporary exhaustiveness algorithm based on flow-analysis. The real algorithm, which is in the process of being enabled, will yield different results.

@johnniwinther
Copy link

The CL for enabling the full exhaustiveness checking is https://dart-review.googlesource.com/c/sdk/+/288703

@gspencergoog
Copy link
Contributor

TapRegion changes LGTM.

@goderbauer
Copy link
Member Author

Full exhaustiveness checking is rolling into the framework with #123059.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer
Copy link
Member Author

goderbauer commented Mar 20, 2023

To my surprise, with the bump to 3.0 the analyzer complained about a handful of additional places where const should be used, I fixed those up in the last commit.

Edit: Looks like at runtime these new const get flagged as invalid: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8786064751393689137/+/u/run_test.dart_for_web_canvaskit_tests_shard_and_subshard_3/test_stdout

Copy link
Contributor

Choose a reason for hiding this comment

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

ScrollBehavior changed re: dart-lang/sdk#51802 LGTM. Thank you!

child: ExpansionTile(
// TODO(goderbauer): Reevaluate the following ignores when https://github.com/dart-lang/sdk/issues/51800 is fixed.
await tester.pumpWidget(MaterialApp( // ignore: prefer_const_constructors
home: Material( // ignore: prefer_const_constructors
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these going to show up in any customer code that uses prefer_const_constructors? This lint is turned on as part of flutter_lints, so it might affect a lot of people if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they will see this once they bump the sdk constraints to 3.0 in their pubspecs. I am pushing hard on dart-lang/sdk#51800 to get this fixed before Dart 3 is released to stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that seems important.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2023
@auto-submit auto-submit bot merged commit 25e38a2 into flutter:master Mar 21, 2023
@goderbauer goderbauer deleted the bump30 branch March 21, 2023 20:22
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 f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants