googlearchive / pedantic

How to get the most value from Dart static analysis
https://pub.dev/packages/pedantic
BSD 3-Clause "New" or "Revised" License
324 stars 56 forks source link

Allow future.unawaited aside unawaited(future) #82

Closed a14n closed 3 years ago

a14n commented 3 years ago

future.unawaited vs. unawaited(future) is a matter of taste :) . Both could silent unawaited_futures. What do you think of adding the following extension?

extension FutureExt<T> on Future<T> {
  void get unawaited {}
}
davidmorgan commented 3 years ago

I like it--and it doesn't pollute the top level namespace like the current unawaited.

But we've learned to be very careful with changes to this package :/ as so much depends on it. I think if we add such an extension it would break someone who already has the same extension?

a14n commented 3 years ago

I think if we add such an extension it would break someone who already has the same extension?

It should be ok as long as no other extension defines a unawaited member on Future.

//---------- pedantic.dart
extension FutureExt<T> on Future<T> {
  void get unawaited {}
}

//---------- a.dart
extension FutureExt<T> on Future<T> {
  void get a {}
}

//-----------  main.dart
import 'a.dart';
import 'pedantic.dart';

main() {
  var future = Future<void>.value();
  future.unawaited; // OK - extension from pedantic.dart
  future.a; // OK - extension from a.dart
  future.b; // extension from current lib
}

extension FutureExt<T> on Future<T> {
  void get b {}
  // void unawaited() {}  // error if this unawaited is also defined hehe
}

void unawaited() {} // OK 

@lrhn : do you see a potential breaking issue about this addition?

davidmorgan commented 3 years ago

That is much like what broke when we tried moving unawaited to package:meta; package:test had defined its own unawaited, and so we broke package:test and most of the world.

We don't have a good way to check for such breakage in important packages.

a14n commented 3 years ago

no, no breakage if an unawaited member is defined in main.dart (I updated the above code). The breakage only occurs if unawaited is also defined in an extension of Future (that is really more unlikely)

davidmorgan commented 3 years ago

Right. I mean someone might define the exact same extension, for exactly the same reason :)

I can search google3 code, nobody has done it there yet :) so we're probably safe on core packages.

alextekartik commented 3 years ago

I like the extension idea, but maybe not in this package (why not in core or meta). I have to add pedantic as a regular dependency just for some unawaited calls (that I sometimes remove to use ignore comment instead to avoid this dependency). I'd rather have this package without code so I can use it as a dev_dependency.

davidmorgan commented 3 years ago

Agreed, that's why we tried to move unawaited to package:meta, but we failed.

The extension might have better luck.

a14n commented 3 years ago

@davidmorgan feel free to move this issue to the appropriate issue tracker if it can land in meta.

a14n commented 3 years ago

I can also create a PR on meta!

davidmorgan commented 3 years ago

:D please go ahead with a PR/issue for package:meta, let's keep this one here too since it relates to effectively deprecating the code part of this package.

lrhn commented 3 years ago

I can also not see any issues with adding this to package:meta (and the recommending it over unawaited from package:pedantic). That said, I also saw no issue with moving the unawaited future top package meta to begin with.

There should not be any conflict unless someone already defined another extension on Future with name unawaited (unlikely) and you need to use both of them in the same library (compounding the unlikeliness) and you are not willing to invoke them explicitly.

Are we allowed to ignore FutureOr now. I can see that unawaited takes a Future? as argument. Not sure why accepting one union type and not the other, so should it take FutureOr? as well?

An alternative name could be .ignore (but that should probably also catch and ignore errors).

a14n commented 3 years ago

PR submitted at dart-lang/sdk#45284