add .unawaited extension on Future #45284

a14n · 2021-03-11T13:09:45Z

This will allow to silent unawaited_futures lint.

Future<void> saveUserPreferences() async {
  await _writePreferences();

  // While 'log' returns a Future, the consumer of 'saveUserPreferences'
  // is unlikely to want to wait for that future to complete; they only
  // care about the preferences being written).
  log('Preferences saved!').unawaited;
}

Related to googlearchive/pedantic#82

a14n · 2021-03-11T13:23:35Z

https://dart-review.googlesource.com/c/sdk/+/190720

a14n · 2021-03-11T16:33:59Z

Not sure who is the good reviewer for it ? @bwilkerson perhaps ?

bwilkerson

This is definitely an interesting idea. We previously experimented with moving the definition of the function unawaited from pedantic to this package, but it turned out to be too big a breaking change. This nicely avoids that issue. @davidmorgan I'd be interested to hear your thoughts about this.

However, if we do decide we want to add this I'd want to put it in a new library so that users can decide whether they want this functionality or not.

davidmorgan · 2021-03-15T08:28:54Z

It seems like a possible path to doing what we wanted to do before: dropping the code from pedantic. I guess we'd do a 2.0 release of pedantic without the method; this would force people to move to the extension.

I think if it goes in it should be in the main library, making it the one recommended way of ignoring future results in async methods.

Thanks.

davidmorgan · 2021-03-15T17:23:17Z

I suggest we aim for more discussion / signoff on this one as it is a big change :) it will be in scope in a lot of places.

bwilkerson · 2021-03-15T17:37:52Z

I suggest we aim for more discussion / signoff on this one as it is a big change :) ...

Yes, it probably does deserve more discussion. Perhaps we could create an issue for that purpose.

... it will be in scope in a lot of places.

That's one of the reasons I see for moving it to a separate library. Then it's only in scope if users explicitly add it to the scope.

a14n · 2021-03-15T20:07:09Z

@lrhn is it valid Dart to have void getter? Or should we prefer Null return type?

extension FutureExtension<T> on Future<T>? {
  void get unawaited {}
}

bwilkerson · 2021-03-15T20:22:13Z

It's definitely valid in that the code has valid Dart semantics and can be run without error. I think the real question is whether it's considered to be a good API design style that we want to promote. @munificent might also have thoughts in this area.

nt4f04uNd · 2021-03-15T21:38:51Z

there's a little problem with this, fwiw
i'm not sure whether could this be a concern to not make this change

void main() {
  dynamic future = Future.value();
  unawaited(future);
  future.unawaited; // throws without cast to Future
}

void unawaited(Future<void>? future) {}
extension FutureExtension<T> on Future<T>? {
  void get unawaited {}
}

lrhn · 2021-03-16T09:49:02Z

The reason to use .unawaited is to silence the "unawaited future" lint. That lint won't trigger for a variable typed as dynamic, so it doesn't matter that future.unawaited doesn't work because you'll never need it.

lrhn · 2021-03-16T09:51:42Z

pkg/meta/lib/future_extensions.dart

+library meta_future_extensions;
+
+/// Extensions that apply to futures.
+extension FutureExtension<T> on Future<T>? {


This name could be more specific. You're likely never going to need to write it, but if you are, it's nicer if it is somewhat related to the content.

Maybe MetaFutureExtension or FutureLintExtension. Or Just FutureLintTools, extensions don't have to be named Extension (we do tend to name extensions as Extension and mixins as Mixin because it's both their declaration and their role, but it's not a rule that you must, if you can describe their role better with other words).

I really like FutureLintTools! We could also rename the lib to lint_tools.dart. @bwilkerson WDYT?

I don't have any problem making the name more specific, but I find "Tools" to be even more general than "Extensions", so I prefer FutureLintExtensions.

And about the library name? Should I change it to lint_tools.dart?

I think the general style is to use the snake-case equivalent of the declaration's name when there's a single declaration in the file, so maybe future_lint_extensions.dart? It's kind of long, and I'm not convinced that "lint" is adding anything, either in the file name or the extension name, but I don't have a strong preference.

Regarding sibling file name like dart2js.dart it could be worth to name it linter.dart. It will open the door to adding annotations that can be leveraged by the linter package.

About the name, as there can be several extension on the same type, we could name the extension SilentUnawaitedFuturesExtension!

I don't have any objection to linter.dart. It indicates that the contents are related to the linter, which is true.

... adding annotations that can be leveraged by the linter package.

I'm not sure what you have in mind, but so far we've chosen to make enforcement of annotations be enabled by default and hence hints, rather than lints.

I'm not sure what you have in mind

There are several issue in the linter issue tracker mentioning annotations to know if a lint should apply or not. This linter.dart lib could be the recipient where those annotations land.

lrhn · 2021-03-16T09:52:36Z

pkg/meta/lib/future_extensions.dart

+/// Annotations that developers can use to extend [Future] capabilities.
+library meta_future_extensions;
+
+/// Extensions that apply to futures.


That's just repeating the declaration.

Maybe:

/// Operations on futures which can help with enabling or avoiding lints.

bwilkerson · 2021-03-16T17:30:40Z

pkg/meta/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 1.3.1
+
+* Introduce `.unawaited` extension on `Future` used to indicates to tools that


"indicates" --> "indicate"

bwilkerson · 2021-03-16T17:35:14Z

pkg/meta/lib/future_extensions.dart

+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Annotations that developers can use to extend [Future] capabilities.


This is wrong in a couple of ways. First, it isn't defining annotations, it's defining extension methods. Second, the things it's declaring can't be used to extend the capabilities of Future, they can be used to control the behavior of a lint rule.

lrhn · 2021-03-17T09:35:39Z

pkg/meta/lib/linter.dart

@@ -0,0 +1,29 @@
+// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file


Are users intended to import it directly. If so, it should have a short name, like meta/lints.dart.
Is the meta package otherwise separated between lints and "other warnings"?

Could this file just be in src/ and be exported by meta.dart?

Are users intended to import it directly.

Yes.

Is the meta package otherwise separated between lints and "other warnings"?

No, we haven't made such a distinction. As far as I can see there is only one annotation that is related to a lint (optionalTypeArgs) and it's of the form that @a14n mentioned elsewhere of an annotation to suppress a lint (a "users can safely violate this lint with this API" semantic).

Could this file just be in src/ and be exported by meta.dart?

Yes, but there are a couple of reasons why I'm leaning toward making it a separate top-level library:

This is the first declaration in this package that isn't intended to be used as an annotation, and that feels like a good reason to split it out.

There are a lot of libraries that import meta.dart and I don't want to make breaking changes unnecessarily. Users can import it with a prefix to help prevent collisions when we add a top-level name, but prefixes don't help with extension methods.

I'm very unconcerned about extension members colliding. It happens very rarely.
I'd be much more concerned about having to add an extra import, something people will consistently forget to do, then be annoyed and go back and add it (bonus points if we can quick-fix the import, obviously).

I think it would be a better user experience if import "package:meta/meta.dart"; would import everything you need to interact with the analyzer or linter. Otherwise someone will just introduce package:metameta/meta.dart which exports both (I know I would).

I think it would be a better user experience if import "package:meta/meta.dart"; would import everything you need to interact with the analyzer or linter.

Possibly, but that isn't the way the package is currently organized. Importing meta.dart doesn't get you the dart2js-specific annotations, nor the Target annotation used only by annotation authors. In those cases we split out the pieces that are not universally useful.

I see this the same way. This extension is only useful to users that have enabled the unawaited_futures lint and who don't want to use // ignore comments to disable it in specific locations.

If we introduce the unawaited_futures lint as default in all new Dart projects (and we will), then it does become almost universally useful to have the helper extension.

Otherwise consider having a convenience package:meta/all.dart which imports all the (common) annotations and functions/extensions. Maybe everything, or at least everything which isn't platform dependent.
I'd use it (well, if I used meta at all).

davidmorgan · 2021-03-17T09:55:01Z

Please do not land this before wider review and signoff; we will have to update a significant fraction of google-internal *.dart files based on what is decided here. We need sufficient consensus to ensure that this doesn't get deprecated (for google-internal use) as soon as it lands.

We definitely don't want the end result to be that there are two widely-used ways of doing this, one in pedantic, one in meta.

bwilkerson · 2021-03-17T14:55:00Z

Please do not land this before wider review and signoff ...

Understood. Would you mind adding me to those discussions so that I can track the progress of the discussions and understand the reasons for the decision(s) that are made?

We definitely don't want the end result to be that there are two widely-used ways of doing this ...

Agreed.

a14n · 2021-04-06T20:19:43Z

Any update about internal discussions?

davidmorgan · 2021-04-07T12:53:45Z

Sorry for the lack of update; internal discussion stalled without reaching a conclusion.

The sticking point is that for long statements, the extension isn't very visible:

    unawaited(listener.onStateChanged(state).catchError(
        (e, stackTrace) =>
            _logger.warning('Failed...', e, stackTrace)));

vs

    listener.onStateChanged(state).catchError(
        (e, stackTrace) =>
            _logger.warning('Failed...', e, stackTrace)).unawaited;

and this observation leads specifically to a desire for the unawaited modifier to go in the same place as the await keyword, which means either a method as we have today, a keyword which would be a language change, or an annotation which would also be a language change.

lrhn · 2021-04-07T13:16:41Z

I personally prefer reading the .unawaited. I don't see it as very important part of the statement, it's only there to make the previous part of the statement not cause a warning. If the code does not have a warning, all is good, and you can and should ignore the .unawaited when reading the code.

So, you can look at the statement expression and think "does it have an await? If no, then it's not something to wait for." Asynchrony is just not important for this statement. Not unless it contains other nested awaits, but it's the presence of await which is important, not the presence of unawaited, that's just a way to say that you didn't forget an await.
Putting the unawaited(...) in front makes it appear as the most important part of the statement - it's the first thing you see: "this things is definitely not awaited, whatever it is", before you even see what the operation is.

I can see other people think otherwise. Possibly because the fear .unawaited being used incorrectly and not being spotted during code-review.

Anyone can write the extension themselves if they want to. It's not magical in any way, it's just a non-async function ignoring a future. I'd consider adding one myself

a14n · 2021-04-07T13:33:21Z

If all this unawaited work takes place in a new linter.dart libaray we can add both unawaited(future) function and future.unawaited extension without breakage in the existing code. This will let user use the one they prefer.

To be even funnier we could replace the unawaited(future) with an unawaited object having a single operator % member and write : unawaited % future... ok, ok, forget that :D

davidmorgan · 2021-04-07T13:34:52Z

Thanks Lasse.

I don't have a super strong opinion about it, since I rarely write code that needs to unawait things. But it's enough of a question mark that I don't feel confident pushing for the switch. We could take a poll.

Re: including both--I think we'd definitely like to end up with just the one recommended way...

lrhn · 2021-04-13T10:54:30Z

If we make the await_futures lint be on by default in all new Dart projects, it might make sense to add the .unawaited getter in the SDK (we have an extension on futures already, it can go in there).

Being in the SDK would mean that you can use it without needing package:meta, but it would not conflict with another declaration (because non-SDK extensions take precedence over SDK extensions in case of conflict).
I'm not particularly happy about adding lint-related things in dart:core (it's not core functionality), but it may give the best user experience.

natebosch · 2021-04-13T21:18:01Z

@davidmorgan - are you driving the discussion about unawaited(Future) vs Future.unawaited?

davidmorgan · 2021-04-14T07:00:22Z

@natebosch I just pinged the thread. I'm not actively pushing things forward as none of the solutions looks great to me at this point.

gaaclarke · 2021-05-18T16:14:02Z

It was mentioned a couple of times here and it seems to solve everyone's issue but it wasn't given much consideration. If you don't want the solution to collide and you want it visible; why not make unawaited a keyword that can be placed in front of futures? If you can't make a decision about the tradeoffs go with the nuclear option thats only tradeoff is a bit more engineering effort.

a14n · 2021-05-19T20:35:20Z

Closing in favour of https://dart-review.googlesource.com/c/sdk/+/200428

adrianvintu · 2021-06-01T12:53:53Z

Same as @gaaclarke , I would love to have unawaited as a keyword. It would make the code much more readable, imho.

add .unawaited extension on Future

727865f

google-cla bot added the cla: yes label Mar 11, 2021

a14n mentioned this pull request Mar 11, 2021

Allow future.unawaited aside unawaited(future) googlearchive/pedantic#82

Closed

bwilkerson reviewed Mar 11, 2021

View reviewed changes

address review comments

04fb3de

move FutureExtension in its own library

8247f98

lrhn reviewed Mar 16, 2021

View reviewed changes

bwilkerson reviewed Mar 16, 2021

View reviewed changes

address review comments

b55bbfe

lrhn reviewed Mar 17, 2021

View reviewed changes

bwilkerson mentioned this pull request Mar 17, 2021

Move unawaited to package:meta? dart-lang/linter#2522

Closed

MrBirb mentioned this pull request May 18, 2021

Add explicit discard for futures you don't want to await #46047

Closed

a14n closed this May 19, 2021

a14n deleted the meta-unawaited-future branch May 19, 2021 20:35

		@@ -1,3 +1,8 @@
		## 1.3.1

		* Introduce `.unawaited` extension on `Future` used to indicates to tools that

		@@ -0,0 +1,29 @@
		// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file

add .unawaited extension on Future #45284

add .unawaited extension on Future #45284

Conversation

a14n commented Mar 11, 2021

a14n commented Mar 11, 2021

a14n commented Mar 11, 2021

bwilkerson left a comment

Choose a reason for hiding this comment

davidmorgan commented Mar 15, 2021

davidmorgan commented Mar 15, 2021

bwilkerson commented Mar 15, 2021

a14n commented Mar 15, 2021

bwilkerson commented Mar 15, 2021

nt4f04uNd commented Mar 15, 2021 • edited Loading

lrhn commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmorgan commented Mar 17, 2021

bwilkerson commented Mar 17, 2021

a14n commented Apr 6, 2021

davidmorgan commented Apr 7, 2021

lrhn commented Apr 7, 2021 • edited Loading

a14n commented Apr 7, 2021

davidmorgan commented Apr 7, 2021

lrhn commented Apr 13, 2021

natebosch commented Apr 13, 2021

davidmorgan commented Apr 14, 2021

gaaclarke commented May 18, 2021

a14n commented May 19, 2021

adrianvintu commented Jun 1, 2021

nt4f04uNd commented Mar 15, 2021 •

edited

Loading

lrhn Mar 17, 2021 •

edited

Loading

lrhn commented Apr 7, 2021 •

edited

Loading