Skip to content

[various] Fixes unawaited_futures violations #4067

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.

Sign up for GitHub

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

Merged
auto-submit merged 18 commits into from
May 24, 2023
Merged

[various] Fixes unawaited_futures violations #4067

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b329c0f
Enable by default
stuartmorgan-g May 22, 2023
f2095ff
Add the new opt-outs
stuartmorgan-g May 23, 2023
907d834
Remove metrics center
stuartmorgan-g May 23, 2023
87051f6
Fix format
stuartmorgan-g May 23, 2023
a8c7651
Fix url_launcher tests
stuartmorgan-g May 23, 2023
8e6981e
Fix camera tests
stuartmorgan-g May 23, 2023
055260b
Fix script
stuartmorgan-g May 23, 2023
d6cb450
Fix violations
stuartmorgan-g May 23, 2023
3002f3d
Better comment wording
stuartmorgan-g May 23, 2023
d747104
CHANGELOGs and versions
stuartmorgan-g May 23, 2023
1263593
Fix typo, reword again to not refer to relative time
stuartmorgan-g May 23, 2023
bed74b3
Move new rule into test dir
stuartmorgan-g May 24, 2023
1490224
Fix test hang
stuartmorgan-g May 24, 2023
e92cac8
Restore passing errors through google_sign_in init
stuartmorgan-g May 24, 2023
b0b1895
Merge remote-tracking branch 'upstream/main' into unawaited-async
stuartmorgan-g May 24, 2023
8909565
Update excerpts
stuartmorgan-g May 24, 2023
86e18bf
Ignore analysis options changes for version check
stuartmorgan-g May 24, 2023
0b8a5ec
Fix include path
stuartmorgan-g May 24, 2023
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
Prev Previous commit
Next Next commit
Add the new opt-outs
  • Loading branch information
@stuartmorgan-g
stuartmorgan-g committed May 23, 2023
commit f2095ffc4e1ce9dba61cc7e2f67e5ba7cd44087a
10 changes: 10 additions & 0 deletions packages/animations/analysis_options.yaml
View file
Open in desktop
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
8 changes: 8 additions & 0 deletions packages/camera/camera_android_camerax/analysis_options.yaml
View file
Open in desktop
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

@reidbaker reidbaker May 23, 2023

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

@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.

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
10 changes: 10 additions & 0 deletions packages/go_router/analysis_options.yaml
View file
Open in desktop
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

@chunhtai chunhtai May 23, 2023

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 •
edited
Loading

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

@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.

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

Copy link
Contributor

@chunhtai chunhtai May 23, 2023

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

@goderbauer goderbauer May 23, 2023

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

@reidbaker reidbaker May 23, 2023

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

@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.

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

@chunhtai chunhtai May 23, 2023

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

@stuartmorgan-g stuartmorgan-g May 24, 2023

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

@goderbauer goderbauer May 26, 2023

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.

8 changes: 8 additions & 0 deletions packages/webview_flutter/webview_flutter/analysis_options.yaml
View file
Open in desktop
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

linter:
rules:
unawaited_futures: false
8 changes: 8 additions & 0 deletions packages/webview_flutter/webview_flutter_android/analysis_options.yaml
View file
Open in desktop
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

linter:
rules:
unawaited_futures: false
8 changes: 8 additions & 0 deletions packages/webview_flutter/webview_flutter_wkwebview/analysis_options.yaml
View file
Open in desktop
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

linter:
rules:
unawaited_futures: false
14 changes: 12 additions & 2 deletions script/configs/custom_analysis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,27 @@
# which references this file from source, but out-of-repo.
# Contact stuartmorgan or devoncarew for assistance if necessary.

# Temporary opt-outs of unawaited_futures; see
# https://github.com/flutter/flutter/issues/127323
- camera_android_camerax
- webview_flutter/webview_flutter
- webview_flutter_android
- webview_flutter_wkwebview
# Opts out of unawaited_futures, matching flutter/flutter's disabling due
# to interactions with animation.
- animations
# Deliberately uses flutter_lints, as that's what it is demonstrating.
- flutter_lints/example
# Adopts some flutter_tools rules regarding public api docs due to being an
# extension of the tool and using tools base code.
- flutter_migrate
# Has some test files that are intentionally broken to conduct dart fix tests.
# Also opts out of unawaited_futures, matching flutter/flutter.
- go_router
# Adds unawaited_futures. We should investigating adding this to the root
# options instead.
- metrics_center
# Has some constructions that are currently handled poorly by dart format.
- rfw/example
# Disables docs requirements, as it is test code.
- web_benchmarks/testing/test_app
# Has some test files that are intentionally broken to conduct dart fix tests.
- go_router