new lint to ensure proper use of `BuildContexts` (use_build_context_synchronously?)

[pq]

New lint: Do not store BuildContext into variables, or use across async calls.

This lint will flag source where

• a BuildContext is ever assigned to anything other than a local variable, or

• a BuildContext is ever passed as an argument to any async function or whose corresponding parameter doesn't have the type BuildContext (where an async method is one that returns either Future or Stream).

Proposed name: do_not_store_build_contexts ?

/cc @jefflim-google @bwilkerson

[jefflim-google]

• a BuildContext is ever assigned to anything other than a local variable, or

This sounds correct.

• a BuildContext is ever passed as an argument to any async function

This is too restrictive. A context can be used in a async function before await is used.

Future<bool> doSomething(BuildContext context) async {
  // At this stage, using `context` is fine.
  final navigator = Navigator.of(context);
  final result = await shouldDoSomething();

  // After the await, using `context` is NOT ok.
  navigator.pushNamed('blah');  // This should be permitted.
  Navigator.of(context).pushNamed('blah');  // This should trigger the lint.
}

[jefflim-google]

Re lint naming: I'm not familiar with with how lints are generally named. Should they be named according to what is permitted, or what is forbidden? use_build_context_synchronously ? vs do_not_store_build_context

[bwilkerson]

A context can be used in a async function before await is used.

There is currently a technical limitation that would make that difficult: the linter looks at a single library at a time. That means that when it is analyzing the library containing the invocation of the async function it does not have visibility of whether the parameter is referenced after the first await.

How many issues would we fail to catch if it ignored passing a context to an async function?

Should they be named according to what is permitted, or what is forbidden?

We have not been very consistent about that. I suppose it depends on what you're trying to emphasize. In this case I like your suggestion of use_build_context_synchronously because it leaves room to expand the lint to find other ways in which that could be violated, whereas do_not_store_build_context already fails to imply that they shouldn't be passed as arguments to async functions.

[jefflim-google]

I was thinking that the linter would allow the function call without issue:

  myMethod(context);

But would analyze the function that was called instead:

Future<void> myMethod(BuildContext context) async {

This can work by observing a single library at a time still?

[jefflim-google]

Expanding on that,

Future<void> myMethod(BuildContext context) async {
  myMethod1(context); // fine
  myMethod2(context); // still fine
  await myMethod3(context); // ok

  myMethod4(context); // not ok, since it's after an await.
}

Is there a pointer to documentation/information explaining what information the analyzer does have so that I can understand it better?

[bwilkerson]

This can work by observing a single library at a time still?

Yes, that would work.

Is there a pointer to documentation/information explaining what information the analyzer does have so that I can understand it better?

Nothing great. There's a short tutorial explaining how to get the information through the analyzer's APIs, but not really describing the information you can get. Beyond that there's only the dartdoc on the classes.

In short form, the linter has available the results of analyzing a single library (a ResolvedLibraryResult). Those results include a resolved AST for each compilation unit. The AST structure represents the full syntactic structure of the code. Many of the nodes in the AST have 'elements' associated with them. Elements represent the semantics of the code (the things that are declared by the code). So for example, every identifier in the AST has associated with it the element that it refers (resolves) to.

For example, the proposed lint rule can search the AST for the first use of await and then search the remaining nodes (the nodes that would be evaluated after the await) in the AST for any references to the parameter context.

[jacob314]

We could even add quick fixes to use
https://api.flutter.dev/flutter/widgets/DisposableBuildContext-class.html
for many cases where BuildContext is used unsafely.

[dnfield]

The basic problem is that passing a build context to anything that might hold on to it beyond when it's still mounted. this can happen with or without async work, but awaiting in an async function is certainly more likely to make it happen.

For example, it's also problematic to use it inside of a closure (see flutter/flutter#79605) when that closure may get invoked in another build cycle.

@jefflim-google - even await myMethod3(context); // ok is probably not ok, since you have no idea if that function is going to itself await something else that slips to a new build cycle and ends up using the context when it's no longer valid, thus retaining memory or doing other bad things.

I think a good lint would be to encourage you to use disposable build context if you pass it to anything async, including using it inside of a closure.