commit 80dc547e6cee1b19d6be2102235defad647c1126
author Paul Berry <paulberry@google.com> Mon Mar 18 19:32:00 2024 +0000
committer Commit Queue <dart-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Mar 18 19:32:00 2024 +0000
tree f30db727f7ebeab0ba03f781bb0d92d76e536bb4
parent 3cf6ac9bb0b6b778f80e28de13be54345c95f124

Replace InferenceContext and ThisAccessTracker with simpler mechanisms.

During my work on https://github.com/dart-lang/language/issues/3648, I
ran into some trybot failures with an exception that looked like this:

    RangeError (index): Invalid value: Valid value range is empty: -1
    #0      List.[] (dart:core-patch/growable_array.dart:264:36)
    #1      List.removeLast (dart:core-patch/growable_array.dart:336:20)
    #2      InferenceContext.popFunctionBodyContext (package:analyzer/src/generated/resolver.dart:124:33)
    #3      ResolverVisitor.visitBlockFunctionBody (package:analyzer/src/generated/resolver.dart:1890:38)

Some quick reading of the code revealed that `popFunctionBodyContext`
always removed a single entry from a list, and
`pushFunctionBodyContext` always added a single entry to it. The two
calls were always paired up using a straightforward try/finally
pattern that seemed like it should guarantee proper nesting, making
this exception impossible:

    try {
      inferenceContext.pushFunctionBodyContext(...);
      ...
    } finally {
      ...
      inferenceContext.popFunctionBodyContext(node);
    }

After a lot of headscratching and experimenting, I eventually figured
out what was happening: an exception was being thrown during
`pushFunctionBodyContext`, _before_ it had a chance to add an entry to
the list. But since the exception happened inside the `try` block, the
call to `popFunctionBodyContext` would happen anyway. As a result, the
pop would fail, causing its own exception, and the exception that was
the original source of the problem would be lost.

This seemed like a code smell to me: where possible, the clean-up
logic in `finally` clauses should be simple enough that it can always
succeed, without causing an exception, even if a previous exception
has put data structures in an unexpected state.

And I had gained enough familiarity with the code over the course of
my debugging to see that what we were doing in those `finally` clauses
was more complex than necessary:

- In the ResolverVisitor, we were pushing and popping a stack of
  `BodyInferenceContext` objects using the try/finally pattern
  described above. But we were only ever accessing the top entry on
  the stack, which meant that the same state could be maintained with
  a single BodyInferenceContext pointer, and some logic that can't
  possibly lead to an exception in the `finally` clause:

    var oldBodyContext = _bodyContext;
    try {
      _bodyContext = ...;
      ...
    } finally {
      _bodyContext = oldBodyContext;
    }

- In the ResolverVisitor and the ErrorVerifier, we were also pushing
  and popping a stack of booleans tracking whether the currently
  enclosing function (or initializer) has access to `this`. In the
  ResolverVisitor, this information wasn't being used at all, so it
  could be safely removed. In the ErrorVerifier, it was being used,
  but it was possible to simplify it in a similar way, so that it was
  tracked with a single boolean (`_hasAccessToThis`), rather than a
  stack.

Simplifying this logic brings several advantages:

- As noted above, since it's now impossible for an exception to occur
  in the `finally` clause, exceptions occurring in the `try` clause
  won't get lost, making debugging easier.

- The code should be more performant, since it no longer requires
  auxiliary heap-allocated stacks.

- The code is (IMHO) easier to understand, since the try/catch pattern
  for maintaining the new `_bodyContext` and `_hasAccessToThis` fields
  just involves saving a field in a local variable (and restoring it
  later), rather than calling out to separate classes.

Change-Id: I61ae80fb28a69760ea0b2856a6954b4a68cfcbe1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358200
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>