Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Replace InferenceContext and ThisAccessTracker with simpler mechanisms.
During my work on dart-lang/language#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 <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Paul Berry <[email protected]>
- Loading branch information
1 parent
3cf6ac9
commit 80dc547