Fix memory leak in incremental resolver

pkg/analyzer/lib/src/dart/element/element.dart

 // Copyright (c) 2014, 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.dart.element.element;
 
 import 'dart:collection';
 import 'dart:math' show min;
 
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 1688 matching lines @@
    * or `null` if this element does not have a documentation comment.
    */
   int _docRangeOffset;
 
   /**
    * The length of the documentation comment range for this element.
    */
   int _docRangeLength;
 
   /**
+   * If true, the element is being used as a map key.
+   * Mutations that change operator== (and hashCode)
+   * will throw an exception.
+   */
+  bool frozen = false;

[Brian Wilkerson, 2016/02/01 14:58:16]

This would be better implemented as a Modifier because then it wouldn't increase
the memory usage.

[skybrian, 2016/02/02 02:12:19]

Hmm. That apparently makes it part of the public API?

[Brian Wilkerson, 2016/02/02 16:32:06]

I'm not sure why. The class Modifier isn't part of the public API. Anyway, we
need to resolve the bigger issues before worrying about this one.

+
+  /**
    * Initialize a newly created element to have the given [name] at the given
    * [_nameOffset].
    */
   ElementImpl(String name, this._nameOffset) {
     this._name = StringUtilities.intern(name);
   }
 
   /**
    * Initialize a newly created element to have the given [name].
    */
@@ skipping 29 matching lines @@
     _docComment = doc?.replaceAll('\r\n', '\n');
   }
 
   @override
   Element get enclosingElement => _enclosingElement;
 
   /**
    * Set the enclosing element of this element to the given [element].
    */
   void set enclosingElement(Element element) {
+    _checkNotFrozen();
     _enclosingElement = element as ElementImpl;
     _cachedLocation = null;
     _cachedHashCode = null;
   }
 
   @override
   int get hashCode {
     // TODO: We might want to re-visit this optimization in the future.
     // We cache the hash code value as this is a very frequently called method.
     if (_cachedHashCode == null) {
@@ skipping 61 matching lines @@
       }
       _cachedLocation = new ElementLocationImpl.con1(this);
     }
     return _cachedLocation;
   }
 
   @override
   String get name => _name;
 
   void set name(String name) {
+    _checkNotFrozen();

[scheglov, 2016/02/01 16:01:25]

The location of an Element consists of identifiers (names) of all enclosing
elements.
So, in order to guarantee that the location does not change we should freeze not
only this, but also all the enclosing elements.

[Brian Wilkerson, 2016/02/01 16:11:58]

Good point!

If we went with the method on CachePartition we would need to remove and restore
all children of the element being modified.

If we stick with the approach of freezing elements, then we'll probably have to
go with a reference counting scheme so that if multiple children of an element
are both placed in the cache we won't unfreeze the parent until all of the
children are removed. (Which makes me even more concerned about freezing
elements.)

[Brian Wilkerson, 2016/02/02 16:32:06]

It seems to me that the consequences are that we're postponing the problem until
something forces the cached data to be flushed. It appears that the only time
we're flushing the cached hash code is when something changes that would have
demonstrated the problem anyway, so it might not be an issue.
I'm not totally convinced that it's easy :-). Also, I'm starting to get confused
by all of the possibilities, so I'm not sure what changes we're actually looking
at making at this point. Could you update the CL to reflect your current
understanding of what we should do to the code? Then we can move forward from
there.

     this._name = name;
     _cachedLocation = null;
     _cachedHashCode = null;
   }
 
   @override
   int get nameLength => displayName != null ? displayName.length : 0;
 
   @override
   int get nameOffset => _nameOffset;
 
   /**
    * Sets the offset of the name of this element in the file that contains the
    * declaration of this element.
    */
   void set nameOffset(int offset) {
+    _checkNotFrozen();
     _nameOffset = offset;
     _cachedHashCode = null;
     _cachedLocation = null;
   }
 
   @override
   Source get source {
     if (_enclosingElement == null) {
       return null;
     }
@@ skipping 143 matching lines @@
   String toString() {
     StringBuffer buffer = new StringBuffer();
     appendTo(buffer);
     return buffer.toString();
   }
 
   @override
   void visitChildren(ElementVisitor visitor) {
     // There are no children to visit
   }
+
+  void _checkNotFrozen() {
+    if (frozen) {
+      //print("$this at $location is frozen and can't be mutated");

[Brian Wilkerson, 2016/02/01 14:58:16]

Remove debugging code.

[skybrian, 2016/02/02 01:16:54]

I've seen other commented out debugging code checked in.
Should I remove it when I see it?

[skybrian, 2016/02/02 02:12:19]

Done.

[Brian Wilkerson, 2016/02/02 16:32:06]

It's a judgement call. Commented out code tends to get stale quickly, so if it
gets left in it should be really useful code (and even then we should consider
whether it can be re-written so that it can be left uncommented. In this case,
we're already throwing an exception with the same message, so the print
statement would be easy to re-create.

+      throw new StateError("$this at $location is frozen and can't be mutated");
+    }
+  }
 }
 
 /**
  * A concrete implementation of an [ElementLocation].
  */
 class ElementLocationImpl implements ElementLocation {
   /**
    * The character used to separate components in the encoded form.
    */
   static int _SEPARATOR_CHAR = 0x3B;
@@ skipping 2072 matching lines @@
   ParameterElementImpl(String name, int nameOffset) : super(name, nameOffset);
 
   /**
    * Initialize a newly created parameter element to have the given [name].
    */
   ParameterElementImpl.forNode(Identifier name) : super.forNode(name);
 
   /**
    * Creates a synthetic parameter with [name], [type] and [kind].
    */
-  factory ParameterElementImpl.synthetic(String name, DartType type,
-      ParameterKind kind) {
+  factory ParameterElementImpl.synthetic(
+      String name, DartType type, ParameterKind kind) {
     ParameterElementImpl element = new ParameterElementImpl(name, -1);
     element.type = type;
     element.synthetic = true;
     element.parameterKind = kind;
     return element;
   }
 
   @override
   String get defaultValueCode => _defaultValueCode;
 
@@ skipping 646 matching lines @@
 
   @override
   void visitElement(Element element) {
     int offset = element.nameOffset;
     if (offset != -1) {
       map[offset] = element;
     }
     super.visitElement(element);
   }
 }