Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ linter:
- tighten_type_of_initializing_formals
# - type_annotate_public_apis # subset of always_specify_types
- type_init_formals
# - unawaited_futures # too many false positives, especially with the way AnimationController works
- unawaited_futures # DIFFERENT FROM FLUTTER/FLUTTER: It's disable there for "too many false positives"; that's not an issue here, and this would have caught bugs in the past.
- unnecessary_await_in_return
- unnecessary_brace_in_string_interps
- unnecessary_const
Expand Down
10 changes: 10 additions & 0 deletions packages/animations/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# This custom rule set only exists to allow opting out of the repository
# default of enabling unawaited_futures. Please do NOT add more changes
# here without consulting with #hackers-ecosystem on Discord.

include: ../../analysis_options.yaml

linter:
rules:
# Matches flutter/flutter, which disables this rule due to false positives.
unawaited_futures: false
4 changes: 4 additions & 0 deletions packages/camera/camera/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.10.5+2

* Fixes unawaited_futures violations.

## 0.10.5+1

* Removes obsolete null checks on non-nullable values.
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera/lib/src/camera_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ class CameraController extends ValueNotifier<CameraValue> {
}

if (value.isStreamingImages) {
stopImageStream();
await stopImageStream();
}

try {
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description: A Flutter plugin for controlling the camera. Supports previewing
Dart.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.10.5+1
version: 0.10.5+2

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down
4 changes: 2 additions & 2 deletions packages/camera/camera/test/camera_image_stream_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ void main() {

await cameraController.initialize();

cameraController.startVideoRecording(
await cameraController.startVideoRecording(
onAvailable: (CameraImage image) => null);

expect(
Expand All @@ -192,7 +192,7 @@ void main() {

await cameraController.initialize();

cameraController.startVideoRecording();
await cameraController.startVideoRecording();

expect(mockPlatform.streamCallLog.contains('startVideoCapturing'), isTrue);
});
Expand Down
4 changes: 4 additions & 0 deletions packages/camera/camera_android/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## NEXT

* Fixes unawaited_futures violations.

## 0.10.8+2

* Removes obsolete null checks on non-nullable values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,12 @@ class CameraController extends ValueNotifier<CameraValue> {
enableAudio: enableAudio,
);

CameraPlatform.instance
unawaited(CameraPlatform.instance
.onCameraInitialized(_cameraId)
.first
.then((CameraInitializedEvent event) {
initializeCompleter.complete(event);
});
}));

await CameraPlatform.instance.initializeCamera(
_cameraId,
Expand Down Expand Up @@ -444,8 +444,8 @@ class CameraController extends ValueNotifier<CameraValue> {
if (_isDisposed) {
return;
}
_deviceOrientationSubscription?.cancel();
_isDisposed = true;
await _deviceOrientationSubscription?.cancel();
super.dispose();
if (_initCalled != null) {
await _initCalled;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ void main() {
isMethodCall('startImageStream', arguments: null),
]);

subscription.cancel();
await subscription.cancel();
});

test('Should stop streaming', () async {
Expand All @@ -1136,7 +1136,7 @@ void main() {
final StreamSubscription<CameraImageData> subscription = camera
.onStreamedFrameAvailable(cameraId)
.listen((CameraImageData imageData) {});
subscription.cancel();
await subscription.cancel();

// Assert
expect(channel.log, <Matcher>[
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# TODO(stuartmorgan): Remove this file and fix all the unawaited_futures
# violations. See https://github.com/flutter/flutter/issues/127323

include: ../../../analysis_options.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this pr. I wish we used absolute paths instead of relative paths when the absolute path has more meaning.

I know that effective dart says to prefer relative paths but in this case it matters more that we are using the root analysis_options.yaml than the one 3 folders up.
https://dart.dev/effective-dart/usage#prefer-relative-import-paths

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that would be better in this case, but I don't think it's actually possible; there's no package at the root.

I guess we could make a local package with the shared options, and then depend on that in every single pubspec.yaml so we could use a package: URL, but that would be pretty ugly. And the local package reference in the pubspec would have to be... a relative path.


linter:
rules:
unawaited_futures: false
4 changes: 4 additions & 0 deletions packages/camera/camera_avfoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## NEXT

* Fixes unawaited_futures violations.

## 0.9.13+2

* Removes obsolete null checks on non-nullable values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,12 @@ class CameraController extends ValueNotifier<CameraValue> {
enableAudio: enableAudio,
);

CameraPlatform.instance
unawaited(CameraPlatform.instance
.onCameraInitialized(_cameraId)
.first
.then((CameraInitializedEvent event) {
initializeCompleter.complete(event);
});
}));

await CameraPlatform.instance.initializeCamera(
_cameraId,
Expand Down Expand Up @@ -443,8 +443,8 @@ class CameraController extends ValueNotifier<CameraValue> {
if (_isDisposed) {
return;
}
_deviceOrientationSubscription?.cancel();
_isDisposed = true;
await _deviceOrientationSubscription?.cancel();
super.dispose();
if (_initCalled != null) {
await _initCalled;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ void main() {
isMethodCall('startImageStream', arguments: null),
]);

subscription.cancel();
await subscription.cancel();
});

test('Should stop streaming', () async {
Expand All @@ -1137,7 +1137,7 @@ void main() {
final StreamSubscription<CameraImageData> subscription = camera
.onStreamedFrameAvailable(cameraId)
.listen((CameraImageData imageData) {});
subscription.cancel();
await subscription.cancel();

// Assert
expect(channel.log, <Matcher>[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ void main() {
isMethodCall('startImageStream', arguments: null),
]);

subscription.cancel();
await subscription.cancel();
});

test('Should stop streaming', () async {
Expand All @@ -1102,7 +1102,7 @@ void main() {
final StreamSubscription<CameraImageData> subscription = camera
.onStreamedFrameAvailable(cameraId)
.listen((CameraImageData imageData) {});
subscription.cancel();
await subscription.cancel();

// Assert
expect(channel.log, <Matcher>[
Expand Down
3 changes: 2 additions & 1 deletion packages/camera/camera_windows/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 0.2.1+7

* Fixes unawaited_futures violations.
* Updates minimum supported SDK version to Flutter 3.3/Dart 2.18.

## 0.2.1+6
Expand Down
8 changes: 4 additions & 4 deletions packages/camera/camera_windows/example/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ class _MyAppState extends State<MyApp> {
enableAudio: _recordAudio,
);

_errorStreamSubscription?.cancel();
unawaited(_errorStreamSubscription?.cancel());
_errorStreamSubscription = CameraPlatform.instance
.onCameraError(cameraId)
.listen(_onCameraError);

_cameraClosingStreamSubscription?.cancel();
unawaited(_cameraClosingStreamSubscription?.cancel());
_cameraClosingStreamSubscription = CameraPlatform.instance
.onCameraClosing(cameraId)
.listen(_onCameraClosing);
Expand Down Expand Up @@ -193,7 +193,7 @@ class _MyAppState extends State<MyApp> {

Future<void> _recordTimed(int seconds) async {
if (_initialized && _cameraId > 0 && !_recordingTimed) {
CameraPlatform.instance
unawaited(CameraPlatform.instance
.onVideoRecordedEvent(_cameraId)
.first
.then((VideoRecordedEvent event) async {
Expand All @@ -204,7 +204,7 @@ class _MyAppState extends State<MyApp> {

_showInSnackBar('Video captured to: ${event.file.path}');
}
});
}));

await CameraPlatform.instance.startVideoRecording(
_cameraId,
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_windows/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: camera_windows
description: A Flutter plugin for getting information about and controlling the camera on Windows.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_windows
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.2.1+6
version: 0.2.1+7

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down
5 changes: 3 additions & 2 deletions packages/cross_file/tool/run_tests.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//
// usage: dart run tool/run_tests.dart
// (needs a `chrome` executable in $PATH, or a tweak to dart_test.yaml)
import 'dart:async';
import 'dart:io';
import 'package:path/path.dart' as p;

Expand All @@ -30,8 +31,8 @@ Future<void> main(List<String> args) async {

Future<Process> _streamOutput(Future<Process> processFuture) async {
final Process process = await processFuture;
stdout.addStream(process.stdout);
stderr.addStream(process.stderr);
unawaited(stdout.addStream(process.stdout));
unawaited(stderr.addStream(process.stderr));
return process;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/flutter_image/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 4.1.6

* Fixes unawaited_futures violations.
* Updates minimum supported SDK version to Flutter 3.3/Dart 2.18.
* Aligns Dart and Flutter SDK constraints.

Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_image/lib/network.dart
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class NetworkImageWithRetry extends ImageProvider<NetworkImageWithRetry> {
scale: key.scale,
);
} catch (error) {
request?.close();
await request?.close();
lastFailure = error is FetchFailure
? error
: FetchFailure._(
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_image/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: >
Image utilities for Flutter: improved network providers, effects, etc.
repository: https://github.com/flutter/packages/tree/main/packages/flutter_image
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+flutter_image%22
version: 4.1.5
version: 4.1.6

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down
1 change: 1 addition & 0 deletions packages/flutter_markdown/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT

* Fixes unawaited_futures violations.
* Updates minimum Flutter version to 3.3.
* Aligns Dart and Flutter SDK constraints.
* Replace `describeEnum` with the `name` getter.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
import 'package:flutter_markdown/flutter_markdown.dart';
Expand Down Expand Up @@ -125,11 +127,11 @@ class _BasicMarkdownDemoState extends State<BasicMarkdownDemo> {
String? href,
String title,
) async {
showDialog<Widget>(
unawaited(showDialog<Widget>(
context: context,
builder: (BuildContext context) =>
_createDialog(context, text, href, title),
);
));
}

Widget _createDialog(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class DemoNotesView extends StatelessWidget {
String? href,
String title,
) async {
showDialog<Widget>(
await showDialog<Widget>(
context: context,
builder: (BuildContext context) =>
_createDialog(context, text, href, title),
Expand Down
10 changes: 10 additions & 0 deletions packages/go_router/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# This custom rule set only exists to allow very targeted changes
# relative to the default repo settings, for specific use cases.
# Please do NOT add more changes here without consulting with
# #hackers-ecosystem on Discord.

include: ../../analysis_options.yaml

analyzer:
exclude:
# This directory deliberately has errors, to test `fix`.
- "test_fixes/**"
linter:
rules:
# Matches flutter/flutter, which disables this rule due to false positives.
unawaited_futures: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too familiar with this lint. what's the false positives in this package?

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g May 23, 2023

Choose a reason for hiding this comment

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

It looks like it's all in test/; there are about 40 violations, mostly push calls in this construction:

        goRouter.push(somePath);
        await tester.pumpAndSettle();

If you'd prefer to annotate all of the tests instead of opting out, that's definitely an option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(or do // ignore_for_file: unawaited_futures in all of the test files)

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, the pr is good as is, i can audit this later

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the Navigator API (push/pop, etc) and the AnimationController API are the two main reasons why we don't have that lint enabled in the framework. It is actually completely safe to not await these calls because they will never throw. If that's also true for the goRouter API opting the entire package out of the lint seems a reasonable approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mild preference for ignore for file in the test than opting out the directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, the Navigator API (push/pop, etc) and the AnimationController API are the two main reasons why we don't have that lint enabled in the framework. It is actually completely safe to not await these calls because they will never throw.

Have we ever requested an annotation that would opt specific APIs out of this (a better named @itsNormalAndFineToCallThisMethodWithoutAwait) that would avoid us disabling a whole (useful) lint just for a handful of APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mild preference for ignore for file in the test than opting out the directory.

or we can have a analyzer_option.yaml in test folder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good thought, moved to the test directory.

Copy link
Member

Choose a reason for hiding this comment

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

Have we ever requested an annotation that would opt specific APIs out of this (a better named @itsNormalAndFineToCallThisMethodWithoutAwait) that would avoid us disabling a whole (useful) lint just for a handful of APIs?

Yeah, there is a request for this on file with the linter project.

3 changes: 2 additions & 1 deletion packages/go_router_builder/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 2.0.2

* Fixes unawaited_futures violations.
* Updates minimum supported SDK version to Flutter 3.3/Dart 2.18.

## 2.0.1
Expand Down
6 changes: 4 additions & 2 deletions packages/go_router_builder/example/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

// ignore_for_file: public_member_api_docs

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
import 'package:provider/provider.dart';
Expand Down Expand Up @@ -185,7 +187,7 @@ class HomeScreen extends StatelessWidget {
value: '2',
child: const Text('Push w/ return value'),
onTap: () async {
FamilyCountRoute(familyData.length)
unawaited(FamilyCountRoute(familyData.length)
.push<int>(context)
.then((int? value) {
if (value != null) {
Expand All @@ -195,7 +197,7 @@ class HomeScreen extends StatelessWidget {
),
);
}
});
}));
},
),
PopupMenuItem<String>(
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router_builder/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: go_router_builder
description: >-
A builder that supports generated strongly-typed route helpers for
package:go_router
version: 2.0.1
version: 2.0.2
repository: https://github.com/flutter/packages/tree/main/packages/go_router_builder
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router_builder%22

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ void main() {
final dom.DomHtmlElement target = dom.document.createElement('div');

test('Injects script into desired target', () async {
loadWebSdk(target: target);
await loadWebSdk(target: target);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change was incorrect; the test never simulates running the callback, causing the test to hang; I'll make it unawaited with a comment.


// Target now should have a child that is a script element
final Object children = js_util.getProperty<Object>(target, 'children');
Expand Down
Loading