Migration tool gets distracted by assert functions

[a14n]

With the following code:

int get i {
  int result;
  assert(() {
    if (false) {
      result = 1;
    }
    return true;
  }());
  if (result == null) {
    throw Error();
  }
  return result;
}

the migration tool suggests:

int? get i {
  int? result;
  assert(() {
    if (false) {
      result = 1;
    }
    return true;
  }());
  if (result == null) {
    throw Error();
  }
  return result;
}

but the return type of the method should be int instead of int? because of the null gard at the end.

Without assert the migration tool works correctly and suggest:

int  get i {
  int? result;
  // assert(() {
  //   if (false) {
  //     result = 1;
  //   }
  //   return true;
  // }());
  if (result == null) {
    throw Error();
  }
  return result;
}

[a14n]

/cc @stereotype441

[MichaelRFairhurst]

Simpler repro:

int? get i {
  int? result;
  () {
    result = 1;
  };
  if (result == null) {
    throw Error();
  }
  return result;
}

What's happening here is that type promotion doesn't work for variables that are referenced in anonymous functions. So I think this is WAI, should be resolved by adding a hint.

int /*!*/ get i {
  int? result;
  () {
    result = 1;
  };
  if (result == null) {
    throw Error();
  }
  return result!;
}

(notice, for instance, the ! on return result! to manually de-null the type since type promotion won't do so)

A last additional note: we have considered that flow analysis is underpowered for the migration itself, because its overly conservative. The classic example is fields. #40566 is the issue for us to create our own more aggressive flow analysis that does things the default language spec one cannot. However, the main motivating case for this is fields, which are essentially a very subtle soundness problem. I don't believe we can really confidently say the same of function literals. I think those are more not handled due to an inherent essential complexity.

So potentially we could handle some limited aspect of this, such as the fact that the original example inclused a self executing anonymous function? So I'll close this but link to it from that issue.

In terms of usability, it also would be nice to better describe these things. We have an issue for that in terms of the dart analyzer itself, and if we solve that, it would be nice to try to apply that same info to the nullability trace view. #38773