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.

[polina-c]

An alternative approach could be to introduce a lighter method than build, and to make it default and recommended:

Widget safeBuild(LightBuildContext context)

LightBuildContext should contain just basic information (like style, screen size and so on) without links to huge trees, i.e. it should be safely passable everywhere.

If safeBuild is not enough, build should be used, but with annotation. I.e. lint will alert the developer about consequences and safety rules of working with BuildContext.

There should be validation that only one of the methods is overriden.

Migration from build to safeBuild will be just method renaming in most cases.

[dnfield]

That approach isn't feasible, since the most common uses for the build context are to get information about position in the tree - it's not a simple matter of copying over primitive values for a lightweight object.

[polina-c]

Is it correct that, for stateful widgets, while we do not want usage of parameter context in closures, it is ok to use this.context?

If yes, can the lint just replace usage of the parameter in closures with this.context?

[dnfield]

This.context is the same as the parameter.

[jacob314]

They aren't the same in the context of being called from a closure within the State object's build method. this.context is the same as the parameter only if the State object is still attached to the Element. Due to that, calling this.context in closure risks leaking the State object this while calling context leaks the Element. I'm not sure whether leaking the State object is better or worse than leaking the Element.

[dnfield]

As far as the framework usage of these things goes, the following code would work:

Widget build(BuildContext context) {
  assert(identical(this.context, context));
}

The only time that would not evaluate to true is if the element is unmounted, but in that case build would never get called.

[polina-c]

Yes, the variables in most cases resolve to the same object. But, closure will be handled differently and, while the build method will not be called for unmounted elements, the closure (event handler?) may be.

[jacob314]

Here is a concrete example:

Widget build(BuildContext context) {
  return Button(onClick() async {
    _loading = true;
    await loadData();
    assert(identical(this.context, context)); // this assert would fire if the Element has been unmounted.
    Navigator.of(context).pushNamed('foo'); 
  } );
}

[dnfield]

Sure, accessing this.context after the state has been unmounted will return null, but if you leaked this.context before unmounting, it will remain leaked even after mounting.

I'm wondering if we're talking about different things though...

[jacob314]

Here is a concrete example where context is not leaked and only the State object leaks if loadData never completes. Replacing this.context with context would mitigate the leak.

Widget build(BuildContext context) {
  return Button(onClick: () async {
    _loading = true;
    await loadData();
    if (mounted) {  
      Navigator.of(this.context).pushNamed('foo'); 
     } else {
       // User has switched to a different page in the app...
     }
  });
}

[dnfield]

But if you replaced it wouldn't the context still get retained in the closure and retain the state?

[jacob314]

The closure no longer references context only this.context so context is not retained by the closure. The observatory debugger is the easiest way to verify this right now as you can see the what variables a closure is retaining as you debug.

[dnfield]

Ahh I think I misread your comment. Leaking the state is better than leaking the context because the context will unhook itself from the state.

However, leaking the state is still bad news, because the state is may be holding onto other expensive objects (streams, images, data fetched from the network, etc). Right?

[pq]

Most of this conversation coalesced into the implementation of use_build_context_synchronously.

If folks want to mine any unimplemented ideas from here, that'd be welcome. In the meantime, closing this one out.

[mehroze-zaidi]

@jefflim-google, @dnfield Hi! Can you please tell me what the long-living area means in the following paragraph: (This paragraph is from Flutter official docs )

Why BuildContext requires extra attention? An example of a large, short-living object that might squeeze into a long-living area and thus cause leaks, is the context parameter passed to Flutter’s build method.

The following code is leak-prone, as useHandler might store the handler in a long-living area:

// BAD: DO NOT DO THIS
// This code is leak-prone:
@override
Widget build(BuildContext context) {
  final handler = () => apply(Theme.of(context));  
  useHandler(handler);
…

// correct way

@override
Widget build(BuildContext context) {
  final theme = Theme.of(context);
  final handler = () => apply(theme);  
  useHandler(handler);
…

Basically, I'm trying to understand how the correct way is correct to avoid memory leaks and how the bad way is not correct.

[dnfield]

Think of it this way:

• The BuildContext is actually your Element (e.g. your State).
• Elements hold data and expect to get let go when they're no longer needed in the tree. For example, on an image widget, the element/state is holding on to the image data. Elements also may have references to other elements and render objects. Retaining a reference to a single element can mean that an entire subtree is reachable as far as the GC is concerned.
• Passing the build context to some handler may cause the element to be reachable long after the part of the tree it was in is gone. The handler, for example, may live and keep a reference to the element for the rest of the life of the program.
• This means the handler method is now keeping a reference to the image indefinitely, and it will never get GC'd. It has "leaked."
• The Theme, on the other hand, is a lighter weight object that is basically a small (comparatively) data class. If it gets retained for a long time it's ok.