Make context explicit in flow analysis API

[stereotype441]

Flow analysis currently works by maintaining a stack of contexts that parallels the set of code constructs surrounding the expression currently being analyzed; entries are pushed and popped on this stack by FlowAnalysis methods whose name ends in begin, and end, respectively. This allows it to maintain state information that the client doesn't need to know about, but it carries a big disadvantage: if the client ever fails to match up begin and end calls properly, this typically results in a hard crash, but the crash doesn't occur until some time after the mismatched begin/end pair; this makes it very hard to debug.

@johnniwinther, @scheglov, and I have talked about this in the past, and the solution we've generally favored is to change the API of flow analysis so that each of the begin methods returns the context object, and each of the end methods takes it as a first argument; this frees flow analysis from having to keep a stack (though it could still do so as a safety check). This means that (a) the client can't forget to call begin, because it won't have a context object to thread through to the end call, and (b) if the client forgets to call end, it will result in incorrect analysis but probably not a crash.

Probably we would want to do this in stages:

• Change the implementation to use the new API, but have a wrapper that manages the stack and exposes the old API (so clients don't change yet)
• One by one, transition clients over to the new API
• After all clients are transitioned, remove the wrapper

I'm currently dealing with a bug related to improper nesting of begin and end calls, so I might need to work on this ASAP

[stereotype441]

Here's an alternative incremental approach that I suspect will help me make faster progress on my debugging:

• Change begin methods to return context objects and end methods to accept them as optional arguments. If a context is supplied to an end method, it is used (and an assertion verifies that it matches the top of the stack); otherwise the context is obtained from the stack.
• One by one, transition clients over to tracking contexts and supplying them to end methods.
• After all clients are transitioned, make the context arguments required and remove the stack.

[scheglov]

Any the described approaches looks good to me.

[stereotype441]

Now that I've finished diagnosing #43093 I'm no longer sure this is such a good idea, at least not in the form proposed above. The problem in #43093 was that when processing an expression like x?.y!, the analyzer was calling FlowAnalysis.nullAwareAccess_rightBegin (as it should) but failing to match it with a call to FlowAnalysis.nullAwareAccess_end. That left an extra item on the flow analysis stack, which could lead to a crash if its type didn't match what was expected by the next "pop". If we had refactored the API as suggested above, that would have gotten rid of the crash, but it would not have gotten rid of the underlying bug because the analyzer would have still failed to call FlowAnalysis.nullAwareAccess_end. The user visible effect would be subtly wrong types from flow analysis, which would have made the issue far harder to notice in the first place, and harder to track down once it was noticed (because it would not have been immediately obvious that flow analysis was the problem).

Here's an alternative proposal that would actually have made the failure easier to debug without making it harder to notice:

• Change begin methods to return context objects and end methods to accept them as optional arguments. If a context is supplied to an end method, it is used (and an explicit check verifies that it matches the top of the stack); otherwise the context is obtained from the stack.
• One by one, transition clients over to tracking contexts and supplying them to end methods.
• After all clients are transitioned, make the context arguments required. Do not remove the stack, since the explicit checks are necessary to make the bug easy to notice.

Based on our current crash reporting data, crashes like #43093 aren't very frequent (at least not yet), so for now I'm going to de-prioritize this issue. I still hope to get to it before null safety ships, but there are many more important things to work on at the moment. If more crashes like #43093 crop up, I'll consider re-prioritizing this issue.