close_sinks rule false positive

[pq]

From @jaumard on November 20, 2018 12:37

With the following code:

TransactionReceiptBloc() {
    final loadTransactionController = StreamController<int>(sync: true);
    transactionId = loadTransactionController.sink;
  }

  void close() {
    transactionId.close();
  }

dart analyzer still warn about sink not closed, but it is. Or do I miss something ?

ENV: Dart VM version: 2.1.0-dev.9.4.flutter-f9ebf21297 (Thu Nov 8 23:00:07 2018 +0100) on "macos_x64"

Copied from original issue: dart-lang/sdk#35221

[pq]

From @lukepighetti on December 9, 2018 21:32

It looks like your close function is outside of your Bloc class. Can you confirm?

[pq]

@jaumard : could you share a more complete example so we can reproduce on our end?

Thanks!

[jaumard]

Sure I'll do a small example asap, but the simple way to reproduce can be to simply put the Stream under a List and then iterate on the list to close them, the linter will still say it's not closed. Also for https://github.com/aloisdeniel/built_bloc it will say it's not closed are stream are closed by inherited method and under a List

[bwilkerson]

I suspect the problem is that lint rules are limited to performing local analysis and that the coding style you're describing would require non-local analysis. Using this rule as an example, it can check to see whether a stream opened in a given method is closed before that method exists, but it cannot verify that if an instance of TransactionReceiptBloc is created that the close method will always be invoked.

I would propose that the solution is to not produce a lint when a stream is assigned to a field, added to a collection, or otherwise escapes the bounds of what the rule can look at to verify safety.

[pq]

I would propose that the solution is to not produce a lint when a stream is assigned to a field, added to a collection, or otherwise escapes the bounds of what the rule can look at to verify safety.

Great proposal. 👍

[rrousselGit]

I recently faced a similar but different issue with close_sinks:

Consider the following function that returns a Sink from somewhere (and correctly internally close it).

Sink getSink();

Then used as such:

final sink = getSink();

This will trigger a close_sinks warning on sink declaration when there is, in fact, no issue at all.

[felangel]

I've created a sample app which you can use to reproduce the issue at https://github.com/felangel/bloc-close-sinks-bug.

Hope that helps 👍

[pq]

Thanks a million @felangel!

[felangel]

@pq is there any update on this?

[pq]

Hey @felangel. Sorry no, I haven't had time to dig in. Looking at https://github.com/felangel/bloc/pull/572#issuecomment-543679914, I'm missing some context too. Are CounterBlocks self-closing? That is, do we simply need to special case them?

[felangel]

@pq BlocProvider automatically closes them when it is disposed (implementation).

Please let me know if you have any other questions or if there’s anything else I can do to help, thanks!

[pq]

Great. It sounds like we want to ignore blocks created by BlocProviders.

The initial hurdle is a bit mechanical since I need mock out a bunch of your classes (and now that I'm looking, that's more cumbersome than it should be...)

I'll update this issue w/ progress.

[rrousselGit]

As a more general rule, maybe this warning should simply not trigger for sinks that are returned by a function:

Sink<T> createSink() {
  return StreamController();
}

[pq]

Interesting idea @rrousselGit! I'm definitely for generalizing but need to think this through.

/fyi @bwilkerson (maybe we can chat about this later?)

[pq]

Ok, a few follow-ups. Thanks all for the thoughtful input so far!

@felangel: how do you ensure BlocProvider gets disposed? Is this a place where another rule is in order?

As for @rrousselGit's suggestion, after a conversation w/ @bwilkerson I'm cooling on it after hearing about experiences with exceptions made to a similar rule implemented in CodePro. My thinking now is to just update the predicate to be something like:

isSink(type) && !isBloc(type)

It seems like that would at least hand the Bloc case?

If there are other framework classes that are similarly managed we could consider adding a more general purpose marker interface or annotation.

@felangel, @rrousselGit: wdyt?

[felangel]

@pq thanks for the quick follow-up! BlocProvider is a StatefulWidget so it will be disposed by Flutter when it is removed from the widget tree.

Your proposal would address the false positive but it would also remove the warning in places where a bloc is manually instantiated (especially outside the context of Flutter).

Ideally, we'd be able to determine where the bloc was instantiated and disable the warning if it was done in the context of BlocProvider's builder.

BlocProvider(
  builder: (_) => MyBloc(), // sink is automatically closed
  child: MyChild(),
),

It would be nice to still have the warning in cases where the bloc is instantiated manually.

BlocProvider.value(
  value: MyBloc(), // sink is not automatically closed
);

void main() {
  final bloc = MyBloc(); // sink is not automatically closed
}

Thoughts?

[rrousselGit]

I don't like the exception specific for flutter_bloc. Especially considering the solution is not bulletproof, nor specific to flutter_bloc.

For example, BlocProvider has a secondary .value constructor:

And doing this:

BlocProvider.value(
  value: MyBloc(),
);

should trigger the warning because the bloc is never closed.

[pq]

Ah, OK. There are a number of issues here apparently. 🤔

@rrousselGit that's a great example and points out a short-coming in our current implementation. I'm pretty sure this won't fire at all currently

BlocProvider.value(
  value: MyBloc(),
);

And nor would this unassigned IOSink

  File('foo.txt').openWrite();

(which seems like a bug to me.)

Maybe we should take a step back and try and specify exactly the desired analysis. I'm wondering if maybe close_sinks isn't the right jumping off point for a rule to help steer Bloc usage.

Tabling fixing close_sinks for the moment, can you describe what we'd want to flag? What if we had a lint (something like avoid_unmanaged_blocs) that identified all instantiations of Blocs outside of a BlocProvider's builder?

BlocProvider(
  builder: (_) => MyBloc(), // OK
  child: MyChild(),
),

final counterBloc = BlocProvider.of<CounterBloc>(context); // OK

BlocProvider.value(
  value: MyBloc(), // LINT
);

final counterBloc = MyBloc(); // LINT

[felangel]

@pq I think the snippets you've provided cover the bloc-specific cases and desired behavior 👍

[rrousselGit]

If something as specific as avoid_unmanaged_blocs is alright, then this discussion should probably include the Flutter team

Sink is only an implementation detail in this story. In the end, it's not about avoid_unmanaged_blocs but widget_build_pure where we shouldn't create Futures, Bloc, ChangeNotifier, or anything else inside build

Which then expends the discussion into how provider (and its dependents like flutter_bloc) or flutter_hook use callbacks to keep the purity of build

[felangel]

@rrousselGit I don't think this is a Flutter specific topic and it isn't just scoped to a widget's build. In my opinion, keeping a widget's build pure is a separate topic. In this case, maybe close_blocs would be a more fitting name if we're tabling a more generic solution.

[rrousselGit]

The topic is not Flutter specific, but the proposed solution is.

Take this snippet:

BlocProvider(
  builder: (_) => MyBloc(), // OK
  child: MyChild(),
),

final counterBloc = BlocProvider.of<CounterBloc>(context); // OK

BlocProvider.value(
  value: MyBloc(), // LINT
);

final counterBloc = MyBloc(); // LINT

It falls in the same situation as:

ChangeNotifierProvider(
  builder: (_) => MyChangeNotifier(), // OK
  child: MyChild(),
),

final counterBloc = Provider.of<CounterBloc>(context); // OK

ChangeNotifierProvider.value(
  value: MyChangeNotifier(), // LINT
);

final counterBloc = MyChangeNotifier(); // LINT

Same thing with FutureProvider+Future, StreamProvider+Stream, or custom approaches

[felangel]

@rrousselGit can you elaborate on FutureProvider + Future?

I agree that there is a lot in common between ChangeNotifier, Stream, and Bloc but having more granular rules might be the way to go (dispose_change_notifiers, close_sinks, cancel_subscriptions).

In any case, I also prefer not to have a bloc-specific solution because in its current state a bloc is a sink and as @pq mentioned there are other cases where close_sinks is also not working as expected so I would still consider this a bug with the existing close_sink implementation as opposed to a feature request for a new rule.

[felangel]

@pq are there any updates on this? Thanks!

[pq]

Hi @felangel. Sorry no, this stalled out without a consensus on direction. Let's re-open the conversation now. @rrousselGit: any additional thoughts?

[pq]

@rrousselGit: any additional thoughts on this one?

[felangel]

bump

[pq]

Sorry @felangel. We've been focussed elsewhere. @rrousselGit: do you have any additional thoughts on this?

[rrousselGit]

Additional, no But I'm still convinced that the situation shouldn't be flutter_bloc specific.

IMO the fix should be "if the sink comes from a function, or is returned by the function that creates it, we don't need to warn"

On Tue, 26 May 2020, 16:20 Phil Quitslund, notifications@github.com wrote:

Sorry @felangel https://github.com/felangel. We've been focussed elsewhere. @rrousselGit https://github.com/rrousselGit: do you have any additional thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/linter/issues/1381#issuecomment-634092496, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEZ3I3JHMMAVZNDWBJE7HQDRTPM3VANCNFSM4GRL6XIA .

[pq]

Thanks! See also parallel conversation in #2113.

[afucher]

Hi, any news on that? 🎅

[pq]

Sorry @afucher but no... The big rock right now is NNBD support and migration (#2401). This one is tricky too and just hasn't gotten prioritized.

[reynolkb]

@pq late to the party here, but i'm running into this same error. it sounds like i should just ignore the error for now since there's no way around it? it's only a yellow error and my code is working fine

[cedvdb]

This one is triggering for me:

class RecordService {
  StreamController<Duration>? _progress$; // here

  Stream<Duration> record() {
    _progress$ = StreamController();

    Timer.periodic(const Duration(seconds: 1), (timer) {
      if (_progress$ != null) {
        _progress$!.add(Duration(seconds: timer.tick));
      }
      else {
        timer.cancel();
      }
    });

    return _progress$!.stream.asBroadcastStream();
  }

  Future<String> stop() async {
    await _progress$!.close();
    _progress$ = null;
    return 'path';
  }
}

[Brads3290]

For what it's worth, this seems to happen if the StreamController is a member of a generic class too. I've produced it with this example:

No warning:

class Abc {

  // No warning here
  final _streamController = StreamController<String>();

  void dispose() {
    this._streamController.close();
  }
}

Warning:

class Abc<T> {

  // Warning: Close instances of `dart.core.Sink`
  final _streamController = StreamController<String>();

  void dispose() {
    this._streamController.close();
  }
}