proposal: extend use_build_context_synchronously to consider Riverpod's WidgetRef

[rrousselGit]

Hello! I'm not sure whether this is reasonable, but it's worth asking.

Long story short, Riverpod would like to define a lint strictly identical to use_build_context_synchronously but for its WidgetRef object.

The lint would be identical in every point, such that BuildContext/WidgetRef can constantly be interchanged. As a matter of fact, WidgetRef is a BuildContext. It uses a different interface to cause less confusion and allows folks using package:provider to smoothly migrate to package:riverpod.

As such, would it be reasonable to update use_build_context_synchronously to handle Riverpod's WidgetRef too?

Such that we have:

void fn() async {
  WidgetRef ref;

  await something;
  ref.anything();
  ^^^^^^^^^^ use_build_context_synchronously

  if (mounted) {
    ref.anything(); // ok
  }

I understand that this repository probably wants to be clean of third-party package logic.

If necessary, maybe a solution could be to add a custom package:meta annotation that both WidgetRef and BuildContext would use (@shouldUseSynchronously or @isBuildContext?)

[bwilkerson]

The current rule says

When a BuildContext is used from a StatefulWidget, the mounted property must be checked after an asynchronous gap.

Does the same apply for WidgetRef, or would we need some way for the annotation to specify a property that can be checked?

[rrousselGit]

Yes, this applies to WidgetRef too.

[rrousselGit]

It's probably worth noting that WidgetRef is a class that interacts with BuildContext (hence why the rules should be respected)

Such that we have:

abstract class WidgetRef {
  void doSomething() {
    Object.of(context).doSomething();
  }
}

So maybe what makes sense would be to annotate the functions of WidgetRef, not WidgetRef itself. Maybe something like @useBuildContext?

abstract class WidgetRef {
  @useBuildContext
  void doSomething();
}

[bwilkerson]

Definitely worth considering.

Would this be useful for other users? I don't know Flutter well enough to know whether users might write methods similar to doSomething or whether that's unlikely to occur very often.

[rrousselGit]

It's probably not very common. Most cases would end-up passing the context variable as parameter to their function

The case described here is different because it already has access to the context, so passing it is redundant

[srawlins]

CC @goderbauer any strong opinion?

[rrousselGit]

For what it's worth, I'm working on riverpod-specific lint-rules using analyzer_plugin

So I could implement this on my own if necessary. Although the annotation approach here would make my like a lot easier

[navaronbracke]

@rrousselGit Another use case would be the mounted property on StateNotifier.

@bwilkerson Perhaps we can generalize the rule to a bool get mounted property regardless of the class?
I.e. if the class has such a property (I assume that we hard-reference to the name mounted here) and we don't add a return after an await, we flag the block of code?

class Foo {
 bool get mounted => true;
 
 void qux() {}
 
 void bar() async {
   await Future.delayed(const Duration(seconds: 1));
   
   if(!mounted) { // LINT without this check
     return;
   }
   
   qux();
 }
}

[bwilkerson]

I'd like to get some input from the Flutter team before making a decision, but I'd much rather see us define an annotation that can be used outside of Flutter rather than implement some special case logic.

[navaronbracke]

I agree, having the use_build_context_synchronously lint is good in a Flutter context, but the idea behind it (not using some state that might have become invalid after an async gap) should be the focus of the lint imo.