Fix cache corruption in incremental resolver

pkg/analyzer/lib/src/context/cache.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.context.cache;
 
 import 'dart:async';
 import 'dart:collection';
 
+import 'package:analyzer/src/dart/element/element.dart'
+    show ElementImpl, Modifier;

[scheglov, 2016/02/17 02:34:25]

Do we want to introduce Element dependency into cache?
Can we create an interface for objects that have freeze / unfreeze?

Ideally we want stop putting objects that may change their hashCode or == over
time.
I.e. simply ban local VariableElement and local FunctionElement, and implement
corresponding functionality without putting these elements into the cache. I
think that would be better than adding a check that we would have to fix anyway.

[skybrian, 2016/02/17 03:05:57]

To avoid this import statement, an alternative might be to add an isCacheKey
property to AnalysisTarget, and set it for all cache keys. But there are many
implementations of AnalysisTarget and perhaps it's not worth doing temporarily.

I consider this a temporary fix. I don't know the best solution.

The problem is that I don't think we fully understand which subtypes of Element
can used as cache keys. (We missed DefaultParameterElementImpl and there might
be others.) This way ensures that the incremental resolver will always do the
right thing in one spot, but there could be unhandled exceptions somewhere else.

I think it would be good to make Element stop implementing AnalysisTarget so
that we can tell statically which subtypes are used as cache keys. I
experimented in [1] with moving AnalysisTarget to subtypes of Element but it's a
complicated class hierarchy.

Another possibility would be to give each Element subtype that can be used in
the cache a cacheKey property. Generally, using mutable objects as map keys is
something we should avoid. But if an element's cacheKey property changes then it
would still be a memory leak, even if the key itself is immutable.

[1] https://codereview.chromium.org/1657463002/

[Brian Wilkerson, 2016/02/17 04:03:54]

I definitely do not want to add a temporary method to public API.

Other than figuring out what the correct solution is and implementing it, that
seems to imply that we need to build in a temporary dependency on ElementImpl.

 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/java_engine.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/generated/utilities_collection.dart';
 import 'package:analyzer/src/task/model.dart';
 import 'package:analyzer/task/model.dart';
 
 /**
  * Return `true` if the given [target] is a priority one.
  */
@@ skipping 212 matching lines @@
    */
   CacheEntry remove(AnalysisTarget target) {
     int count = _partitions.length;
     for (int i = 0; i < count; i++) {
       CachePartition partition = _partitions[i];
       if (partition.isResponsibleFor(target)) {
         if (_TRACE_CHANGES) {
           AnalysisEngine.instance.logger
               .logInformation('Removed the cache entry for $target.');
         }
-        return partition.remove(target);
+        CacheEntry entry = partition.remove(target);
+        if (entry != null) {
+          entry.dispose();
+        }
+        return entry;
       }
     }
     return null;
   }
 
   /**
    * Return the number of targets that are mapped to cache entries.
    */
   int size() {
     int size = 0;
@@ skipping 47 matching lines @@
    * A bit-encoding of boolean flags associated with this entry's target.
    */
   int _flags = 0;
 
   /**
    * A table mapping result descriptors to the cached values of those results.
    */
   Map<ResultDescriptor, ResultData> _resultMap =
       new HashMap<ResultDescriptor, ResultData>();
 
-  CacheEntry(this.target);
+  CacheEntry(this.target) {
+    if (target is ElementImpl) {
+      (target as ElementImpl).setModifier(Modifier.CACHE_KEY, true);
+    }
+  }
 
   /**
    * The exception that caused one or more values to have a state of
    * [CacheState.ERROR].
    */
   CaughtException get exception => _exception;
 
   /**
    * Return `true` if the source was explicitly added to the context or `false`
    * if the source was implicitly added because it was referenced by another
@@ skipping 22 matching lines @@
     _resultMap.forEach((descriptor, data) {
       TargetedResult result = new TargetedResult(target, descriptor);
       for (TargetedResult dependedOnResult in data.dependedOnResults) {
         ResultData dependedOnData = _partition._getDataFor(dependedOnResult);
         if (dependedOnData != null) {
           dependedOnData.dependentResults.remove(result);
         }
       }
     });
     _resultMap.clear();
+    if (target is ElementImpl) {
+      (target as ElementImpl).setModifier(Modifier.CACHE_KEY, false);
+    }
   }
 
   /**
    * Fix the state of the [exception] to match the current state of the entry.
    */
   void fixExceptionState() {
     if (!hasErrorState()) {
       _exception = null;
     }
   }
@@ skipping 14 matching lines @@
     if (data == null) {
       return CacheState.INVALID;
     }
     return data.state;
   }
 
   /**
    * Return the value of the result represented by the given [descriptor], or
    * the default value for the result if this entry does not have a valid value.
    */
-  dynamic /*=V*/ getValue /*<V>*/ (ResultDescriptor /*<V>*/ descriptor) {
+  dynamic/*=V*/ getValue/*<V>*/(ResultDescriptor/*<V>*/ descriptor) {
     ResultData data = _resultMap[descriptor];
     if (data == null) {
       return descriptor.defaultValue;
     }
     if (_partition != null) {
       _partition.resultAccessed(target, descriptor);
     }
     return data.value;
   }
 
@@ skipping 77 matching lines @@
         //
         data.value = descriptor.defaultValue;
       }
     }
   }
 
   /**
    * Set the value of the result represented by the given [descriptor] to the
    * given [value].
    */
-  void setValue /*<V>*/ (ResultDescriptor /*<V>*/ descriptor,
-      dynamic /*=V*/ value, List<TargetedResult> dependedOn) {
+  void setValue/*<V>*/(ResultDescriptor/*<V>*/ descriptor, dynamic/*=V*/ value,
+      List<TargetedResult> dependedOn) {
 //    {
 //      String valueStr = '$value';
 //      if (valueStr.length > 20) {
 //        valueStr = valueStr.substring(0, 20) + '...';
 //      }
 //      valueStr = valueStr.replaceAll('\n', '\\n');
 //      print(
 //          'setValue $descriptor for $target value=$valueStr $dependedOn=$dependedOn');
 //    }
     _validateStateChange(descriptor, CacheState.VALID);
@@ skipping 77 matching lines @@
     for (TargetedResult dependedOnResult in thisData.dependedOnResults) {
       ResultData data = _partition._getDataFor(dependedOnResult);
       if (data != null && deltaResult != DeltaResult.KEEP_CONTINUE) {
         data.dependentResults.remove(thisResult);
       }
     }
     // Invalidate results that depend on this result.
     _invalidateDependentResults(id, thisData, delta, level + 1);
     // If empty and not explicitly added, remove the entry altogether.
     if (_resultMap.isEmpty && !explicitlyAdded) {
-      _partition.entryMap.remove(target);
+      CacheEntry entry = _partition.entryMap.remove(target);
+      if (entry != null) {
+        entry.dispose();
+      }
       _partition._removeIfSource(target);
     }
     // Notify controller.
     _partition.onResultInvalidated
         .add(new InvalidatedResult(this, descriptor, thisData.value));
   }
 
   /**
    * Invalidates all the results of this entry, with propagation.
    */
@@ skipping 657 matching lines @@
   void resultAccessed(TargetedResult result) {}
 
   @override
   List<TargetedResult> resultStored(TargetedResult newResult, newValue) {
     return TargetedResult.EMPTY_LIST;
   }
 
   @override
   void targetRemoved(AnalysisTarget target) {}
 }