Re-enable and add new HTML support

pkg/analyzer/lib/src/context/context.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.context;
 
 import 'dart:async';
 import 'dart:collection';
 
 import 'package:analyzer/instrumentation/instrumentation.dart';
@@ skipping 18 matching lines @@
 import 'package:analyzer/src/generated/scanner.dart';
 import 'package:analyzer/src/generated/sdk.dart' show DartSdk;
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/generated/utilities_collection.dart';
 import 'package:analyzer/src/task/dart.dart';
 import 'package:analyzer/src/task/dart_work_manager.dart';
 import 'package:analyzer/src/task/driver.dart';
 import 'package:analyzer/src/task/manager.dart';
 import 'package:analyzer/task/dart.dart';
 import 'package:analyzer/task/general.dart';
+import 'package:analyzer/task/html.dart';
 import 'package:analyzer/task/model.dart';
+import 'package:html/dom.dart' show Document;
 
 /**
  * Type of callback functions used by PendingFuture. Functions of this type
  * should perform a computation based on the data in [entry] and return it. If
  * the computation can't be performed yet because more analysis is needed,
  * `null` should be returned.
  *
  * The function may also throw an exception, in which case the corresponding
  * future will be completed with failure.
  *
@@ skipping 503 matching lines @@
           buffer.write(tokens[i].lexeme);
         }
         return buffer.toString();
       }
       nameNode = nameNode.parent;
     }
     return null;
   }
 
   @override
-  List<AnalysisError> computeErrors(Source source) =>
-      _computeResult(source, DART_ERRORS);
+  List<AnalysisError> computeErrors(Source source) {
+    String name = source.shortName;
+    if (AnalysisEngine.isDartFileName(name)) {
+      return _computeResult(source, DART_ERRORS);
+    } else if (AnalysisEngine.isHtmlFileName(name)) {
+      return _computeResult(source, HTML_ERRORS);
+    }
+    return AnalysisError.NO_ERRORS;
+  }
 
   @override
   List<Source> computeExportedLibraries(Source source) =>
       _computeResult(source, EXPORTED_LIBRARIES);
 
   @override
-  // TODO(brianwilkerson) Implement this.
-  HtmlElement computeHtmlElement(Source source) => null;
+  HtmlElement computeHtmlElement(Source source) {
+    // TODO(brianwilkerson) Implement this. For the time being, we compute the
+    // data that will implicitly be computed by creating an element.
+    _computeResult(source, REFERENCED_LIBRARIES);
+    return null;
+  }
 
   @override
   List<Source> computeImportedLibraries(Source source) =>
       _computeResult(source, EXPLICITLY_IMPORTED_LIBRARIES);
 
   @override
   SourceKind computeKindOf(Source source) {
     String name = source.shortName;
     if (AnalysisEngine.isDartFileName(name)) {
       return _computeResult(source, SOURCE_KIND);
@@ skipping 168 matching lines @@
     } catch (exception) {
       // If the location cannot be decoded for some reason then the underlying
       // cause should have been logged already and we can fall though to return
       // null.
     }
     return null;
   }
 
   @override
   AnalysisErrorInfo getErrors(Source source) {
-    return dartWorkManager.getErrors(source);
+    String name = source.shortName;
+    if (AnalysisEngine.isDartFileName(name)) {
+      return dartWorkManager.getErrors(source);
+    } else if (AnalysisEngine.isHtmlFileName(name)) {
+      List<AnalysisError> errors = analysisCache.getValue(source, HTML_ERRORS);
+      // TODO(brianwilkerson) We don't currently have line info for HTML files.
+      return new AnalysisErrorInfoImpl(errors, null);
+    }
+    return new AnalysisErrorInfoImpl(null, null);

[Paul Berry, 2015/06/15 21:34:38]

The old task model does this instead:

return new AnalysisErrorInfoImpl(AnalysisError.NO_ERRORS, null);

Would that be better?  It might help avoid NPE's in clients.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

   }
 
   @override
   HtmlElement getHtmlElement(Source source) {
     // TODO(brianwilkerson) Implement this.
 //    SourceEntry sourceEntry = getReadableSourceEntryOrNull(source);
 //    if (sourceEntry is HtmlEntry) {
 //      return sourceEntry.getValue(HtmlEntry.ELEMENT);
 //    }
     return null;
   }
 
   @override
   List<Source> getHtmlFilesReferencing(Source source) {

[Paul Berry, 2015/06/15 21:34:38]

What's the contract on this function?  Is it supposed to compute an approximate
result based on the analysis that has been done so far, or is it supposed to
parse and analyze the HTML files as needed before returning so that it can
produce a precise result?  I can't tell from the documentation above this method
in the AnalysisContext class.

It looks the the current implementation computes an approximate result based on
the analysis that has been done so far.

[Brian Wilkerson, 2015/06/16 16:28:08]

Yes.

-    // TODO(brianwilkerson) Implement this.
-    SourceKind sourceKind = getKindOf(source);
-    if (sourceKind == null) {
+    if (!AnalysisEngine.isDartFileName(source.shortName)) {
       return Source.EMPTY_LIST;
     }
     List<Source> htmlSources = <Source>[];
-//    while (true) {
-//      if (sourceKind == SourceKind.PART) {
-//        List<Source> librarySources = getLibrariesContaining(source);
-//        for (Source source in _cache.sources) {
-//          CacheEntry entry = _cache.get(source);
-//          if (entry.getValue(SOURCE_KIND) == SourceKind.HTML) {
-//            List<Source> referencedLibraries =
-//                (entry as HtmlEntry).getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//            if (_containsAny(referencedLibraries, librarySources)) {
-//              htmlSources.add(source);
-//            }
-//          }
-//        }
-//      } else {
-//        for (Source source in _cache.sources) {
-//          CacheEntry entry = _cache.get(source);
-//          if (entry.getValue(SOURCE_KIND) == SourceKind.HTML) {
-//            List<Source> referencedLibraries =
-//                (entry as HtmlEntry).getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//            if (_contains(referencedLibraries, source)) {
-//              htmlSources.add(source);
-//            }
-//          }
-//        }
-//      }
-//      break;
-//    }
+    List<Source> librarySources = getLibrariesContaining(source);
+    for (Source source in _cache.sources) {
+      if (AnalysisEngine.isHtmlFileName(source.shortName)) {
+        List<Source> referencedLibraries =
+            analysisCache.getValue(source, REFERENCED_LIBRARIES);
+        if (_containsAny(referencedLibraries, librarySources)) {
+          htmlSources.add(source);
+        }
+      }
+    }
     if (htmlSources.isEmpty) {
       return Source.EMPTY_LIST;

[Paul Berry, 2015/06/15 21:34:38]

Nit: it seems like the benefit this confers is tiny, and I'm not convinced it's
worth the effort.  If we drop these three lines and just return htmlSources, the
client won't see any difference in behavior (unless they are doing something
bizarre, like trying to detect an empty list by comparing the result to
Source.EMPTY_LIST instead of simply calling ".isEmpty").

[Brian Wilkerson, 2015/06/16 16:28:08]

The places where we've seen benefit are where the result is being stored. I
don't know if that's happening with the result of this method, so it might not
be necessary here, but it made a noticeable difference in some other places to
store lots of pointers to a single empty list rather than lots of pointers to
lots of empty lists. I think I'll leave it for now.

     }
     return htmlSources;
   }
 
   @override
   SourceKind getKindOf(Source source) {
     String name = source.shortName;
     if (AnalysisEngine.isDartFileName(name)) {
       return _cache.getValue(source, SOURCE_KIND);
     } else if (AnalysisEngine.isHtmlFileName(name)) {
@@ skipping 25 matching lines @@
         }
       }
     }
     if (dependentLibraries.isEmpty) {
       return Source.EMPTY_LIST;
     }
     return dependentLibraries;
   }
 
   @override
   List<Source> getLibrariesReferencedFromHtml(Source htmlSource) {

[Paul Berry, 2015/06/15 21:34:38]

Similar concern about the contract on this function--it seems to return an
approximate result based on analysis so far, and I see no indication in the
comments to lead the user to expect this behavior.

[Brian Wilkerson, 2015/06/16 16:28:08]

In general, the "get" methods are lazy, returning results if we have them and
not doing any work if we don't. There's usually a corresponding "compute" method
that does whatever work is necessary, but there isn't one for this method. But
given that the old implementation is also lazy, I think this one should be too.

Personally, I think we need to redesign the AnalysisContext API at some point.

-    // TODO(brianwilkerson) Implement this.
-//    cache.CacheEntry entry = getReadableSourceEntryOrNull(htmlSource);
-//    if (entry is HtmlEntry) {
-//      HtmlEntry htmlEntry = entry;
-//      return htmlEntry.getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//    }
+    CacheEntry entry = _cache.get(htmlSource);
+    if (entry != null) {
+      return entry.getValue(REFERENCED_LIBRARIES);
+    }
     return Source.EMPTY_LIST;
   }
 
   @override
   LibraryElement getLibraryElement(Source source) =>
       _cache.getValue(source, LIBRARY_ELEMENT);
 
   @override
   LineInfo getLineInfo(Source source) => _cache.getValue(source, LINE_INFO);
 
@@ skipping 150 matching lines @@
     try {
       getContents(source);
     } catch (exception, stackTrace) {
       throw new AnalysisException('Could not get contents of $source',
           new CaughtException(exception, stackTrace));
     }
     return _computeResult(source, PARSED_UNIT);
   }
 
   @override
-  ht.HtmlUnit parseHtmlUnit(Source source) {
+  Document parseDocument(Source source) {

[Paul Berry, 2015/06/15 21:34:38]

How about calling this "parseHtmlDocument" instead?  I'm concerned that to the
author of a client of AnalysisContext (particularly a client that doesn't need
the analyzer's HTML abilities), it won't be obvious what kind of document we're
talking about.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

     if (!AnalysisEngine.isHtmlFileName(source.shortName)) {
       return null;
     }
-    // TODO(brianwilkerson) Implement HTML analysis.
-    return null; //_computeResult(source, null);
+    return _computeResult(source, DOCUMENT);
   }
 
   @override
+  @deprecated // use parseDocument(source)
+  ht.HtmlUnit parseHtmlUnit(Source source) {
+    return null;

[Paul Berry, 2015/06/15 21:34:38]

Instead of returning null, I'd suggest doing "throw new UnimplementedError();"
since that will make it a little easier for a client author to track down what
is going wrong.

I'd also recommend adding a TODO comment to remind us that once we've switched
over to the new task model, we should remove the parseHtmlUnit() method from the
AnalysisContext class, so that client authors will be alerted to the fact that
their code is going to break before runtime.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done
Done

+  }
+
+  @override
   AnalysisResult performAnalysisTask() {
     return PerformanceStatistics.performAnaysis.makeCurrentWhile(() {
       _evaluatePendingFutures();
       bool done = !driver.performAnalysisTask();
       if (done) {
         done = !_validateCacheConsistency();
       }
       List<ChangeNotice> notices = _getChangeNotices(done);
       if (notices != null) {
         int noticeCount = notices.length;
         for (int i = 0; i < noticeCount; i++) {
           ChangeNotice notice = notices[i];
           _notifyErrors(notice.source, notice.errors, notice.lineInfo);
         }
       }
       return new AnalysisResult(notices, -1, '', -1);
     });
   }
 
-  void _evaluatePendingFutures() {
-    for (AnalysisTarget target in _pendingFutureTargets.keys) {
-      CacheEntry cacheEntry = _cache.get(target);
-      List<PendingFuture> pendingFutures = _pendingFutureTargets[target];
-      for (int i = 0; i < pendingFutures.length;) {
-        if (pendingFutures[i].evaluate(cacheEntry)) {
-          pendingFutures.removeAt(i);
-        } else {
-          i++;
-        }
-      }
-    }
-  }
-
   @override
   void recordLibraryElements(Map<Source, LibraryElement> elementMap) {
     elementMap.forEach((Source librarySource, LibraryElement library) {
       //
       // Cache the element in the library's info.
       //
       CacheEntry entry = getCacheEntry(librarySource);
       setValue(ResultDescriptor result, value) {
         entry.setValue(result, value, TargetedResult.EMPTY_LIST);
       }
@@ skipping 235 matching lines @@
   bool _contains(List<Source> sources, Source targetSource) {
     for (Source source in sources) {
       if (source == targetSource) {
         return true;
       }
     }
     return false;
   }
 
   /**
+   * Return `true` if the given list of [sources] contains any of the given
+   * [targetSources].
+   */
+  bool _containsAny(List<Source> sources, List<Source> targetSources) {
+    for (Source targetSource in targetSources) {
+      if (_contains(sources, targetSource)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /**
    * Set the contents of the given [source] to the given [contents] and mark the
    * source as having changed. The additional [offset], [oldLength] and
    * [newLength] information is used by the context to determine what reanalysis
    * is necessary. The method [setChangedContents] triggers a source changed
    * event where as this method does not.
    */
   bool _contentRangeChanged(Source source, String contents, int offset,
       int oldLength, int newLength) {
     bool changed = false;
     String originalContents = _contentCache.setContents(source, contents);
@@ skipping 55 matching lines @@
     List<CacheEntry> entries = <CacheEntry>[];
     MapIterator<AnalysisTarget, CacheEntry> iterator = _cache.iterator();
     while (iterator.moveNext()) {
       if (iterator.key.source == source) {
         entries.add(iterator.value);
       }
     }
     return entries;
   }
 
+  void _evaluatePendingFutures() {
+    for (AnalysisTarget target in _pendingFutureTargets.keys) {
+      CacheEntry cacheEntry = _cache.get(target);
+      List<PendingFuture> pendingFutures = _pendingFutureTargets[target];
+      for (int i = 0; i < pendingFutures.length;) {
+        if (pendingFutures[i].evaluate(cacheEntry)) {
+          pendingFutures.removeAt(i);
+        } else {
+          i++;
+        }
+      }
+    }
+  }
+
   /**
    * Return a list containing all of the change notices that are waiting to be
    * returned. If there are no notices, then return either `null` or an empty
    * list, depending on the value of [nullIfEmpty].
    */
   List<ChangeNotice> _getChangeNotices(bool nullIfEmpty) {
     if (_pendingNotices.isEmpty) {
       if (nullIfEmpty) {
         return null;
       }
       return ChangeNoticeImpl.EMPTY_LIST;
     }
     List<ChangeNotice> notices = new List.from(_pendingNotices.values);
     _pendingNotices.clear();
     return notices;
   }
 
   /**
    * Return a list containing all of the sources known to this context that have
    * the given [kind].
    */
   List<Source> _getSources(SourceKind kind) {
     List<Source> sources = <Source>[];
-    for (Source source in _cache.sources) {
-      CacheEntry entry = _cache.get(source);
-      if (entry.getValue(SOURCE_KIND) == kind) {
-        sources.add(source);
+    if (kind == SourceKind.LIBRARY || kind == SourceKind.PART) {
+      for (Source source in _cache.sources) {
+        CacheEntry entry = _cache.get(source);
+        if (entry.getValue(SOURCE_KIND) == kind) {
+          sources.add(source);
+        }
       }
+    } else if (kind == SourceKind.HTML) {
+      for (Source source in _cache.sources) {
+        if (AnalysisEngine.isHtmlFileName(source.shortName)) {
+          sources.add(source);
+        }
+      }
+    }
+    if (sources == null) {
+      return Source.EMPTY_LIST;

[Paul Berry, 2015/06/15 21:34:38]

Unreachable code.  Sources will never be null.

See also my comment on line 812 above--even if we change the condition to "if
(sources.isEmpty)" I'm not convinced that it confers enough benefit to be worth
it.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

     }
     return sources;
   }
 
   /**
    * Look at the given [source] to see whether a task needs to be performed
    * related to it. If so, add the source to the set of sources that need to be
    * processed. This method is intended to be used for testing purposes only.
    */
   void _getSourcesNeedingProcessing(Source source, CacheEntry entry,
@@ skipping 532 matching lines @@
         new PendingFuture<T>(_context, target, computeValue);
     if (!pendingFuture.evaluate(entry)) {
       _context._pendingFutureTargets
           .putIfAbsent(target, () => <PendingFuture>[])
           .add(pendingFuture);
       scheduleComputation();
     }
     return pendingFuture.future;
   }
 }