Background: null-shorting is a feature of Dart in which an expression
might evaluate to `null` because a `null` was found deep in a
subexpression, skipping evaluation of the rest of the expression. For
example, in the expression `a?.b(c).d`, if `a` evaluates to `null`,
execution will skip the evaluation of `c`, the method call to `b`, and
the get of `d`, will be skipped, and the whole expression `a?.b(c).d`
will evaluate to `null`. The analyzer needs to account for this by
making the static type of `a?.b(c).d` nullable, even if the type of
the `d` getter is not nullable.
The analyzer's implementation of null shorting works like this:
- When visiting a null-aware expression (such as `a?.b(c)` in the
above example), the analyzer uses
`NullShortableExpression.nullShortingTermination` to find the "null
shorting termination expression". This is the expression that will
evaluate to `null` if the null-shorting code path is taken
(`a?.b(c).d` in the above example).
- The null shorting termination expression is pushed onto the stack
`ResolverVisitor._unfinishedNullShorts`.
- After visiting an expression that might be a null shorting
termination expression, the analyzer passes the node that was just
visited to `ResolverVisitor.nullShortingTermination`, which checks
whether it matches any nodes at the top of the
`ResolverVisitor._unfinishedNullShorts` stack. If any nodes match,
they are popped off the stack, and the static type of the null
shorting termination expression is made nullable.
A subtlety with this approach is that if the resolution process has
rewritten the null shorting termination expression, then there are two
expressions in play: the old one (from the original AST, before
rewriting) and the new one (after rewriting). The node in
`ResolverVisitor._unfinishedNullShorts` is the old one, because the
code that pushes nodes onto the stack happens before rewrites. But the
node that needs to have its static type changed is the new one,
because that's the one that will wind up in the final resolved AST. In
fact, trying to change the static type of the old node would lead to a
crash, because the old node doesn't have a static type assigned.
Previous to this CL, the analyzer dealt with this situation by having
`ResolverVisitor.visitMethodInvocation` pass `discardType: true` to
`ResolverVisitor.nullShortingTermination`. This disabled the logic for
changing the type of the null shorting termination expression, which
avoided a crash, but it meant that the type was never updated
properly, leading to https://github.com/dart-lang/sdk/issues/56896.
The proper solution is to pass both the old and new nodes to
`ResolverVisitor.nullShortingTermination`. The old node is matched up
against `ResolverVisitor._unfinishedNullShorts`, and the new node is
used for marking the static type as nullable.
As part of this fix, I've changed the return types of methods in
`MethodInvocationResolver` from `FunctionExpressionInvocation?` to
`FunctionExpressionInvocationImpl?`. This is a harmless change, since
all these methods are private to the analyzer, and it avoids some type
casts in `ResolverVisitor.nullShortingTermination`.
I also added an assertion to `ResolverVisitor.nullShortingTermination`
to verify that the correct rewritten node is passed in (this assertion
relies on the `ResolverVisitor._replacements` expando, which is only
populated when assertions are enabled). Adding this assertion exposed
two other call sites that had to be updated (in
`ResolverVisitor.visitIndexExpression` and
`ResolverVisitor.visitPropertyAccess`). I've added additional analyzer
tests to cover these cases.
Fixes https://github.com/dart-lang/sdk/issues/56896.
Bug: https://github.com/dart-lang/sdk/issues/56896
Change-Id: I8119a05b4d9f386b1129e5419b823a61677358c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/390560
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Paul Berry
3546ba47d9