CFE sometimes pops flow analysis constructs out of order

[stereotype441]

When analyzing the following code:

f(int? i, List? l) => i ?? l?.length;

CFE makes the following calls to flow analysis (excluding calls to isReachable, isAssigned, and isUnassigned, which are not relevant to this bug):

declare(VariableDeclaration(int? i), true)
declare(VariableDeclaration(List<dynamic>? l), true)
variableRead(VariableGetImpl(i), VariableDeclaration(int? i))
ifNullExpression_rightBegin(VariableGetImpl(i), InterfaceType(int?))
variableRead(VariableGetImpl(l), VariableDeclaration(List<dynamic>? l))
nullAwareAccess_rightBegin(VariableGetImpl(l), InterfaceType(List<dynamic>?))
variableRead(VariableGet(#0), VariableDeclaration(final List<dynamic>? #0))
nullAwareAccess_rightBegin(VariableGet(#0), InterfaceType(List<dynamic>?))
ifNullExpression_end()
nullAwareAccess_end()
nullAwareAccess_end()
handleExit()
finish()

This is improperly nested. The call to ifNullExpression_end should occur after the two calls to nullAwareAccess_end.

Fortunately this is a benign bug, for now, because ifNullExpression_end and nullAwareAccess_end have identical behavior. But it's technical debt, because it limits our ability to change the behavior of those two methods in the future.

[stereotype441]

@johnniwinther FYI

[johnniwinther]

I think this can be solved by moving calls to ifNullExpression_end to the end of the visit methods.

Given that nullAwareAccess_end is called implicitly when shorting is stopped by reading a subexpression as an expresssion (as opposed to a guard and an action) this might happen elsewhere.

[stereotype441]

I think this can be solved by moving calls to ifNullExpression_end to the end of the visit methods.

Given that nullAwareAccess_end is called implicitly when shorting is stopped by reading a subexpression as an expresssion (as opposed to a guard and an action) this might happen elsewhere.

@johnniwinther as we discussed in our meeting today, I made an attempt along the lines of your suggestion. It didn't work out too well :(

https://dart-review.googlesource.com/c/sdk/+/167146

[johnniwinther]

Thanks for trying. I'll take it from here.

[johnniwinther]

Fixed by https://dart-review.googlesource.com/c/sdk/+/173729