commit 3546ba47d94cc526a01b5280476e3f1a0e148c61
author Paul Berry <paulberry@google.com> Wed Oct 16 19:01:53 2024 +0000
committer Commit Queue <dart-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Oct 16 19:01:53 2024 +0000
tree e9fbdec38e78e636c49b8406633a44ceee407cd1
parent 0da426deb391a97c07f04c2f0062dada814da76f

analyzer: Fix null shorting of rewritten expressions.

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>