Rework how we handle laziness. (#543)

* Rework how we handle laziness. TODO: test strict querying. * Update CHANGELOG * Add a test of querying strictly. * Remove another place where we were filtering out lazy imports. * Use type variable both places. * Factor out common check from Document * Fix test based on updated fixture code. * Update line in changelog. * Rename strictImports to noLazyImports
Polymer · Apr 4, 2017 · ca71623 · ca71623
1 parent f752312
commit ca71623
Show file tree

Hide file tree

Showing 17 changed files with 87 additions and 60 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 * `generateAnalysis()` now includes PolymerBehavior information in `metadata.polymer.behaviors` collection.
 * Types and descriptions are now extracted from method @param and @returns jsdoc annotations.
+* By default queries for features and warnings now traverse lazy imports. Added a query option to limit results only to those reachable by strict imports.
 
 ## [2.0.0-alpha.34] - 2017-03-20
 

diff --git a/src/core/analysis-context.ts b/src/core/analysis-context.ts
@@ -289,14 +289,8 @@ export class AnalysisContext {
             const parsedDoc = await this._parse(resolvedUrl, contents);
             const scannedDocument = await this._scanDocument(parsedDoc);
 
-            // Find all non-lazy imports
-            // TODO(justinfagnani): I think we should scan lazily imported
-            // documents since we know about them, we should load them. Their
-            // features should possibly be separated out at export time via
-            // better definition of scopes
             const imports = scannedDocument.getNestedFeatures().filter(
-                (e) => e instanceof ScannedImport &&
-                    e.type !== 'lazy-html-import') as ScannedImport[];
+                (e) => e instanceof ScannedImport) as ScannedImport[];
 
             // Update dependency graph
             const importUrls = imports.map((i) => this.resolveUrl(i.url));
@@ -317,14 +311,8 @@ export class AnalysisContext {
     return this._cache.dependenciesScannedPromises.getOrCompute(
         resolvedUrl, async() => {
           const scannedDocument = await this._scanLocal(resolvedUrl, contents);
-          // Find all non-lazy imports
-          // TODO(justinfagnani): I think we should scan lazily imported
-          // documents since we know about them, we should load them. Their
-          // features should possibly be separated out at export time via better
-          // definition of scopes
           const imports = scannedDocument.getNestedFeatures().filter(
-              (e) => e instanceof ScannedImport &&
-                  e.type !== 'lazy-html-import') as ScannedImport[];
+              (e) => e instanceof ScannedImport) as ScannedImport[];
 
           // Scan imports
           for (const scannedImport of imports) {

diff --git a/src/html/html-import-scanner.ts b/src/html/html-import-scanner.ts
@@ -39,8 +39,7 @@ const isLazyImportNode = p.AND(
     p.hasAttr('href'),
     p.NOT(p.hasSpaceSeparatedAttrValue('rel', 'import')),
     notCssLink,
-    p.parentMatches(
-        p.AND(p.hasTagName('dom-module'), p.NOT(p.hasTagName('template')))));
+    p.NOT(p.parentMatches(p.hasTagName('template'))));
 
 /**
  * Scans for <link rel="import"> and <link rel="lazy-import">
@@ -55,12 +54,13 @@ export class HtmlImportScanner implements HtmlScanner {
       Promise<ScannedImport[]> {
     const imports: ScannedImport[] = [];
 
+    const type = 'html-import';
     await visit((node) => {
-      let type: string;
+      let lazy: boolean;
       if (isHtmlImportNode(node)) {
-        type = 'html-import';
+        lazy = false;
       } else if (isLazyImportNode(node)) {
-        type = 'lazy-html-import';
+        lazy = true;
       } else {
         return;
       }
@@ -71,14 +71,15 @@ export class HtmlImportScanner implements HtmlScanner {
           importUrl,
           document.sourceRangeForNode(node)!,
           document.sourceRangeForAttributeValue(node, 'href')!,
-          node));
+          node,
+          lazy));
     });
     if (this._lazyEdges) {
       const edges = this._lazyEdges.get(document.url);
       if (edges) {
         for (const edge of edges) {
-          imports.push(new ScannedImport(
-              'lazy-html-import', edge, undefined, undefined, null));
+          imports.push(
+              new ScannedImport(type, edge, undefined, undefined, null, true));
         }
       }
     }

diff --git a/src/html/html-script-scanner.ts b/src/html/html-script-scanner.ts
@@ -48,7 +48,7 @@ export class HtmlScriptScanner implements HtmlScanner {
               importUrl,
               document.sourceRangeForNode(node)!,
               document.sourceRangeForAttributeValue(node, 'src')!,
-              node));
+              node, false));
         } else {
           const locationOffset = getLocationOffsetOfStartOfTextContent(node);
           const attachedCommentText = getAttachedCommentText(node) || '';

diff --git a/src/html/html-script-tag.ts b/src/html/html-script-tag.ts
@@ -50,7 +50,7 @@ export class ScannedScriptTagImport extends ScannedImport {
           this.sourceRange,
           this.urlSourceRange,
           this.astNode,
-          this.warnings);
+          this.warnings, false);
     } else {
       // not found or syntax error
       const error = this.error ? (this.error.message || this.error) : '';

diff --git a/src/html/html-style-scanner.ts b/src/html/html-style-scanner.ts
@@ -51,7 +51,8 @@ export class HtmlStyleScanner implements HtmlScanner {
               importUrl,
               document.sourceRangeForNode(node)!,
               document.sourceRangeForAttributeValue(node, 'href')!,
-              node));
+              node,
+              true));
         } else {
           const contents = dom5.getTextContent(node);
           const locationOffset = getLocationOffsetOfStartOfTextContent(node);

diff --git a/src/javascript/javascript-import-scanner.ts b/src/javascript/javascript-import-scanner.ts
@@ -39,7 +39,7 @@ export class JavaScriptImportScanner implements JavaScriptScanner {
             importUrl,
             document.sourceRangeForNode(node)!,
             document.sourceRangeForNode(node.source)!,
-            node));
+            node, false));
       }
     });
     return imports;

diff --git a/src/model/document.ts b/src/model/document.ts
@@ -238,14 +238,14 @@ export class Document implements Feature, Queryable {
   getByKind(kind: string, options?: QueryOptions): Set<Feature>;
   getByKind(kind: string, options?: QueryOptions): Set<Feature> {
     options = options || {};
-    if (this._featuresByKind && options.imported) {
+    if (this._featuresByKind && this._isCachable(options)) {
       // We have a fast index! Use that.
       const features = this._featuresByKind.get(kind) || new Set();
       if (!options.externalPackages) {
         return this._filterOutExternal(features);
       }
       return features;
-    } else if (this._doneResolving && options.imported) {
+    } else if (this._doneResolving && this._isCachable(options)) {
       // We're done discovering features in this document and its children so
       // we can safely build up the indexes.
       this._buildIndexes();
@@ -262,15 +262,15 @@ export class Document implements Feature, Queryable {
   getById(kind: string, identifier: string, options?: QueryOptions):
       Set<Feature> {
     options = options || {};
-    if (this._featuresByKindAndId && options.imported) {
+    if (this._featuresByKindAndId && this._isCachable(options)) {
       // We have a fast index! Use that.
       const idMap = this._featuresByKindAndId.get(kind);
       const features = (idMap && idMap.get(identifier)) || new Set();
       if (!options.externalPackages) {
         return this._filterOutExternal(features);
       }
       return features;
-    } else if (this._doneResolving && options.imported) {
+    } else if (this._doneResolving && this._isCachable(options)) {
       // We're done discovering features in this document and its children so
       // we can safely build up the indexes.
       this._buildIndexes();
@@ -308,6 +308,11 @@ export class Document implements Feature, Queryable {
     return result;
   }
 
+  private _isCachable(options?: QueryOptions): boolean {
+    options = options || {};
+    return !!options.imported && !options.noLazyImports;
+  }
+
   private _getByKind(kind: string, options: QueryOptions): Set<Feature> {
     const allFeatures = new Set<Feature>();
     this._getFeatures(allFeatures, new Set(), options);
@@ -337,7 +342,9 @@ export class Document implements Feature, Queryable {
         const imprt = feature as Import;
         const isPackageInternal =
             imprt.document && !Package.isExternal(imprt.document.url);
-        if (options.externalPackages || isPackageInternal) {
+        const externalityOk = options.externalPackages || isPackageInternal;
+        const lazinessOk = !options.noLazyImports || !imprt.lazy;
+        if (externalityOk && lazinessOk) {
           imprt.document._getFeatures(result, visited, options);
         }
       }

diff --git a/src/model/import.ts b/src/model/import.ts
@@ -47,14 +47,21 @@ export class ScannedImport implements Resolvable {
 
   warnings: Warning[] = [];
 
+  /**
+   * If true, the imported document may not be loaded until well after the
+   * containing document has been evaluated, and indeed may never load.
+   */
+  lazy: boolean;
+
   constructor(
       type: string, url: string, sourceRange: SourceRange|undefined,
-      urlSourceRange: SourceRange|undefined, ast: any|null) {
+      urlSourceRange: SourceRange|undefined, ast: any|null, lazy: boolean) {
     this.type = type;
     this.url = url;
     this.sourceRange = sourceRange;
     this.urlSourceRange = urlSourceRange;
     this.astNode = ast;
+    this.lazy = lazy;
   }
 
   resolve(document: Document): Import|undefined {
@@ -79,7 +86,8 @@ export class ScannedImport implements Resolvable {
         this.sourceRange,
         this.urlSourceRange,
         this.astNode,
-        this.warnings);
+        this.warnings,
+        this.lazy);
   }
 }
 
@@ -93,11 +101,12 @@ export class Import implements Feature {
   urlSourceRange: SourceRange|undefined;
   astNode: any|null;
   warnings: Warning[];
+  lazy: boolean;
 
   constructor(
       url: string, type: string, document: Document,
       sourceRange: SourceRange|undefined, urlSourceRange: SourceRange|undefined,
-      ast: any, warnings: Warning[]) {
+      ast: any, warnings: Warning[], lazy: boolean) {
     this.url = url;
     this.type = type;
     this.document = document;
@@ -106,6 +115,10 @@ export class Import implements Feature {
     this.urlSourceRange = urlSourceRange;
     this.astNode = ast;
     this.warnings = warnings;
+    this.lazy = lazy;
+    if (lazy) {
+      this.kinds.add('lazy-import');
+    }
   }
 
   toString() {

diff --git a/src/model/package.ts b/src/model/package.ts
@@ -130,9 +130,11 @@ export class Package implements Queryable {
 
   private _getDocumentQueryOptions(options?: QueryOptions):
       DocumentQueryOptions {
+    options = options || {};
     return {
       imported: true,
-      externalPackages: !!(options && options.externalPackages)
+      externalPackages: options.externalPackages,
+      noLazyImports: options.noLazyImports
     };
   }
 }

diff --git a/src/model/queryable.ts b/src/model/queryable.ts
@@ -25,6 +25,12 @@ export interface BaseQueryOptions {
    * that are referenced from within the package.
    */
   externalPackages?: boolean;
+
+  /**
+   * Do not include any features that are only reachable via paths that include
+   * lazy import edges.
+   */
+  noLazyImports?: boolean;
 }
 
 export type QueryOptions = BaseQueryOptions & object;

diff --git a/src/polymer/css-import-scanner.ts b/src/polymer/css-import-scanner.ts
@@ -44,7 +44,7 @@ export class CssImportScanner implements HtmlScanner {
             importUrl,
             document.sourceRangeForNode(node)!,
             document.sourceRangeForAttributeValue(node, 'href')!,
-            node));
+            node, false));
       }
     });
     return imports;

diff --git a/src/test/analyzer_test.ts b/src/test/analyzer_test.ts
@@ -91,6 +91,8 @@ suite('Analyzer', () => {
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 <link rel="import" href="subfolder/in-folder.html">
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+<link rel="lazy-import" href="lazy.html">
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 `);
     });
@@ -302,25 +304,30 @@ suite('Analyzer', () => {
       const root = await analyzer.analyze('static/dependencies/root.html');
 
       // If we ask for documents we get every document in evaluation order.
+      const strictlyReachableDocuments = [
+        ['static/dependencies/root.html', 'html', false],
+        ['static/dependencies/inline-only.html', 'html', false],
+        ['static/dependencies/inline-only.html', 'js', true],
+        ['static/dependencies/inline-only.html', 'css', true],
+        ['static/dependencies/leaf.html', 'html', false],
+        ['static/dependencies/inline-and-imports.html', 'html', false],
+        ['static/dependencies/inline-and-imports.html', 'js', true],
+        ['static/dependencies/subfolder/in-folder.html', 'html', false],
+        ['static/dependencies/subfolder/subfolder-sibling.html', 'html', false],
+        ['static/dependencies/inline-and-imports.html', 'css', true],
+      ];
+      assert.deepEqual(
+          Array
+              .from(root.getByKind(
+                  'document', {imported: true, noLazyImports: true}))
+              .map((d) => [d.url, d.parsedDocument.type, d.isInline]),
+          strictlyReachableDocuments);
       assert.deepEqual(
           Array.from(root.getByKind('document', {imported: true}))
               .map((d) => [d.url, d.parsedDocument.type, d.isInline]),
-          [
-            ['static/dependencies/root.html', 'html', false],
-            ['static/dependencies/inline-only.html', 'html', false],
-            ['static/dependencies/inline-only.html', 'js', true],
-            ['static/dependencies/inline-only.html', 'css', true],
-            ['static/dependencies/leaf.html', 'html', false],
-            ['static/dependencies/inline-and-imports.html', 'html', false],
-            ['static/dependencies/inline-and-imports.html', 'js', true],
-            ['static/dependencies/subfolder/in-folder.html', 'html', false],
-            [
-              'static/dependencies/subfolder/subfolder-sibling.html',
-              'html',
-              false
-            ],
-            ['static/dependencies/inline-and-imports.html', 'css', true],
-          ]);
+          strictlyReachableDocuments.concat(
+              [['static/dependencies/lazy.html', 'html', false]]));
+
 
       // If we ask for imports we get the import statements in evaluation order.
       // Unlike documents, we can have duplicates here because imports exist
@@ -335,6 +342,7 @@ suite('Analyzer', () => {
             'static/dependencies/subfolder/in-folder.html',
             'static/dependencies/subfolder/subfolder-sibling.html',
             'static/dependencies/subfolder/in-folder.html',
+            'static/dependencies/lazy.html',
           ]);
 
       const inlineOnly = root.getOnlyAtId(

diff --git a/src/test/html/html-import-scanner_test.ts b/src/test/html/html-import-scanner_test.ts
@@ -72,8 +72,9 @@ suite('HtmlImportScanner', () => {
 
       const features = await scanner.scan(document, visit);
       assert.equal(features.length, 2);
-      assert.equal(features[1].type, 'lazy-html-import');
+      assert.equal(features[1].type, 'html-import');
       assert.equal(features[1].url, 'lazy-polymer.html');
+      assert.equal(features[1].lazy, true);
     });
   });
 
@@ -97,12 +98,10 @@ suite('HtmlImportScanner', () => {
       const visit = async(visitor: HtmlVisitor) => document.visit([visitor]);
 
       const features = await scanner.scan(document, visit);
-      assert.deepEqual(features.map((f) => f.type), [
-        'html-import',
-        'lazy-html-import',
-        'lazy-html-import',
-        'lazy-html-import'
-      ]);
+      assert.deepEqual(
+          features.map((f) => f.type),
+          ['html-import', 'html-import', 'html-import', 'html-import']);
+      assert.deepEqual(features.map((i) => i.lazy), [false, true, true, true]);
       assert.deepEqual(
           features.map((f) => f.url),
           ['polymer.html', 'lazy1.html', 'lazy2.html', 'lazy3.html']);

diff --git a/src/test/static/dependencies/lazy.html b/src/test/static/dependencies/lazy.html
diff --git a/src/test/static/dependencies/root.html b/src/test/static/dependencies/root.html
@@ -2,3 +2,4 @@
 <link rel="import" href="leaf.html">
 <link rel="import" href="inline-and-imports.html">
 <link rel="import" href="subfolder/in-folder.html">
+<link rel="lazy-import" href="lazy.html">
diff --git a/src/typescript/typescript-import-scanner.ts b/src/typescript/typescript-import-scanner.ts
@@ -40,7 +40,7 @@ export class TypeScriptImportScanner implements
             // TODO(justinfagnani): make SourceRanges work
             null as any as SourceRange,
             null as any as SourceRange,
-            node));
+            node, false));
       }
     }
     const visitor = new ImportVisitor();