Fix many, many type warnings. (#3539)

- Adds a `check-types` gulp task for checking only types in main binary, which is our initial goal.
- Adds type inference for our assert functions
- First round of type fixes.

build-system/runner/src/org/ampproject/AmpCodingConvention.java

@@ -16,9 +16,15 @@
 
 package org.ampproject;
 
+import com.google.common.collect.ImmutableList;
 import com.google.javascript.jscomp.CodingConvention;
+import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec;
 import com.google.javascript.jscomp.CodingConventions;
 import com.google.javascript.jscomp.ClosureCodingConvention;
+import com.google.javascript.jscomp.newtypes.JSType;
+
+import java.util.ArrayList;
+import java.util.Collection;
 
 
 /**
@@ -35,6 +41,15 @@ public AmpCodingConvention(CodingConvention convention) {
     super(convention);
   }
 
+  @Override public Collection<AssertionFunctionSpec> getAssertionFunctions() {
+    return ImmutableList.of(
+        new AssertionFunctionSpec("user.assert", JSType.TRUTHY),
+        new AssertionFunctionSpec("dev.assert", JSType.TRUTHY),
+        new AssertionFunctionSpec("module$src$log.user.assert", JSType.TRUTHY),
+        new AssertionFunctionSpec("module$src$log.dev.assert", JSType.TRUTHY)
+    );
+  }
+
   /**
    * {@inheritDoc}
    * Because AMP objects can travel between compilation units, we consider

build-system/tasks/compile.js

@@ -76,6 +76,7 @@ exports.cleanupBuildDir = cleanupBuildDir;
 function compile(entryModuleFilename, outputDir,
     outputFilename, options) {
   return new Promise(function(resolve, reject) {
+    const checkTypes = options.checkTypes || argv.typecheck_only;
     var intermediateFilename = 'build/cc/' +
         entryModuleFilename.replace(/\//g, '_').replace(/^\./, '');
     console./*OK*/log('Starting closure compiler for', entryModuleFilename);
@@ -199,7 +200,8 @@ function compile(entryModuleFilename, outputDir,
       }
     };
 
-    if (argv.typecheck_only) {
+    // For now do type check separately
+    if (argv.typecheck_only || checkTypes) {
       // Don't modify compilation_level to a lower level since
       // it won't do strict type checking if its whitespace only.
       compilerOptions.compilerFlags.define = 'TYPECHECK_ONLY=true';

gulpfile.js

@@ -109,15 +109,18 @@ function polyfillsForTests() {
  * @param {boolean} watch
  * @param {boolean} shouldMinify
  * @param {boolean=} opt_preventRemoveAndMakeDir
+ * @param {boolean=} opt_checkTypes
  */
-function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir) {
+function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir,
+    opt_checkTypes) {
   compileCss();
   // For compilation with babel we start with the amp-babel entry point,
   // but then rename to the amp.js which we've been using all along.
   compileJs('./src/', 'amp-babel.js', './dist', {
     toName: 'amp.js',
     minifiedName: 'v0.js',
     includePolyfills: true,
+    checkTypes: opt_checkTypes,
     watch: watch,
     preventRemoveAndMakeDir: opt_preventRemoveAndMakeDir,
     minify: shouldMinify,
@@ -134,6 +137,7 @@ function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir) {
   });
   compileJs('./3p/', 'integration.js', './dist.3p/' + internalRuntimeVersion, {
     minifiedName: 'f.js',
+    checkTypes: opt_checkTypes,
     watch: watch,
     minify: shouldMinify,
     preventRemoveAndMakeDir: opt_preventRemoveAndMakeDir,
@@ -278,6 +282,16 @@ function dist() {
   buildLoginDone({minify: true, watch: false, preventRemoveAndMakeDir: true});
 }
 
+/**
+ * Dedicated type check path.
+ */
+function checkTypes() {
+  process.env.NODE_ENV = 'production';
+  cleanupBuildDir();
+  // We only check types in the main binary for now.
+  compile(false, true, true, /* check types */ true);
+}
+
 /**
  * Build the examples
  *
@@ -723,6 +737,7 @@ function mkdirSync(path) {
  * Gulp tasks
  */
 gulp.task('build', 'Builds the AMP library', build);
+gulp.task('check-types', 'Check JS types', checkTypes);
 gulp.task('css', 'Recompile css to build directory', compileCss);
 gulp.task('default', 'Same as "watch"', ['watch', 'serve']);
 gulp.task('dist', 'Build production binaries', dist);

src/service/viewer-impl.js

@@ -92,6 +92,7 @@ export const TRUSTED_VIEWER_HOSTS = [
  * but instead delegates everything to the actual viewer. This class and the
  * actual Viewer are connected via "AMP.viewer" using three methods:
  * {@link getParam}, {@link receiveMessage} and {@link setMessageDeliverer}.
+ * @package Visible for type.
  */
 export class Viewer {
 

src/service/viewport-impl.js

@@ -47,6 +47,7 @@ let ViewportChangedEventDef;
  * This object represents the viewport. It tracks scroll position, resize
  * and other events and notifies interesting parties when viewport has changed
  * and how.
+ * @package Visible for type.
  */
 export class Viewport {
 

src/service/vsync-impl.js

@@ -46,6 +46,7 @@ let VsyncTaskSpecDef;
  * NOTE: If the document is invisible due to prerendering (this includes
  * application level prerendering where the doc is rendered in a hidden
  * iframe or webview), then no frame will be scheduled.
+ * @package Visible for type.
  */
 export class Vsync {
 

src/service/xhr-impl.js

@@ -32,7 +32,7 @@ import {isArray, isObject, isFormData} from '../types';
  * See https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch
  *
  * @typedef {{
- *   body: (!Object|!Array|undefined),
+ *   body: (!Object|!Array|undefined|string),
  *   credentials: (string|undefined),
  *   headers: (!Object|undefined),
  *   method: (string|undefined),
@@ -44,7 +44,7 @@ let FetchInitDef;
 /** @private @const {!Array<string>} */
 const allowedMethods_ = ['GET', 'POST'];
 
-/** @private @const {!Array<function():boolean>} */
+/** @private @const {!Array<function(*):boolean>} */
 const allowedJsonBodyTypes_ = [isArray, isObject];
 
 /** @private @const {string} */
@@ -56,8 +56,9 @@ const ALLOW_SOURCE_ORIGIN_HEADER = 'AMP-Access-Control-Allow-Source-Origin';
 
 /**
  * A service that polyfills Fetch API for use within AMP.
+ * @package Visible for type.
  */
-class Xhr {
+export class Xhr {
 
   /**
    * @param {!Window} win
@@ -148,7 +149,7 @@ class Xhr {
    *
    * @param {string} input
    * @param {?FetchInitDef=} opt_init
-   * @return {!Promise<!JSONValue>}
+   * @return {!Promise<!JSONObject>}
    */
   fetchJson(input, opt_init) {
     const init = opt_init || {};
@@ -259,6 +260,7 @@ function setupJson_(init) {
  * @private Visible for testing
  */
 export function fetchPolyfill(input, opt_init) {
+  /** @type {!FetchInitDef} */
   const init = opt_init || {};
   return new Promise(function(resolve, reject) {
     const xhr = createXhrRequest(init.method || 'GET', input);
@@ -313,7 +315,7 @@ export function fetchPolyfill(input, opt_init) {
 /**
  * @param {string} method
  * @param {string} url
- * @return {!XMLHttpRequest}
+ * @return {!XMLHttpRequest|!XDomainRequest}
  * @private
  */
 function createXhrRequest(method, url) {
@@ -401,7 +403,7 @@ export class FetchResponse {
 
   /**
    * Drains the response and returns the JSON object.
-   * @return {!Promise<!JSONValue>}
+   * @return {!Promise<!JSONType>}
    */
   json() {
     return this.drainText_().then(JSON.parse);

src/srcset.js

@@ -223,7 +223,7 @@ export class Srcset {
   }
 
   /**
-   * @param {number} width
+   * @param {number} _width
    * @param {number} dpr
    * @return {number}
    * @private

src/string.js

@@ -61,7 +61,7 @@ export function startsWith(string, prefix) {
  * @param {string} template The template string to expand.
  * @param {!function(string):*} getter Function used to retrieve a value for a
  *   placeholder. Returns values will be coerced into strings.
- * @param {number=} optMaxIterations Number of times to expand the template.
+ * @param {number=} opt_maxIterations Number of times to expand the template.
  *   Defaults to 1, but should be set to a larger value your placeholder tokens
  *   can be expanded to other placeholder tokens. Take caution with large values
  *   as recursively expanding a string can be exponentially expensive.

src/styles.js

@@ -18,6 +18,7 @@ import {setStyles, setStyle} from './style';
 import {platformFor} from './platform';
 import {waitForBody} from './dom';
 import {waitForExtensions} from './render-delaying-extensions';
+import {dev} from './log';
 
 
 /**
@@ -52,7 +53,7 @@ export function installStyles(doc, cssText, cb, opt_isRuntimeCss, opt_ext) {
     style.setAttribute('amp-extension', opt_ext || '');
     afterElement = doc.querySelector('style[amp-runtime]');
   }
-  insertAfterOrAtStart(doc.head, style, afterElement);
+  insertAfterOrAtStart(dev.assert(doc.head), style, afterElement);
   // Styles aren't always available synchronously. E.g. if there is a
   // pending style download, it will have to finish before the new
   // style is visible.
@@ -91,7 +92,7 @@ export function installStyles(doc, cssText, cb, opt_isRuntimeCss, opt_ext) {
  */
 export function makeBodyVisible(doc, opt_waitForExtensions) {
   const set = () => {
-    setStyles(doc.body, {
+    setStyles(dev.assert(doc.body), {
       opacity: 1,
       visibility: 'visible',
       animation: 'none',

src/timer.js

@@ -87,7 +87,7 @@ export class Timer {
 
   /**
    * Cancels the previously scheduled callback.
-   * @param {number|string} timeoutId
+   * @param {number|string|null} timeoutId
    */
   cancel(timeoutId) {
     if (typeof timeoutId == 'string') {

src/url.js

@@ -28,6 +28,21 @@ const cache = Object.create(null);
 /** @private @const Matches amp_js_* paramters in query string. */
 const AMP_JS_PARAMS_REGEX = /[?&]amp_js[^&]*/;
 
+/**
+ * @typedef {({
+ *   href: string,
+ *   protocol: string,
+ *   host: string,
+ *   hostname: string,
+ *   port: string,
+ *   pathname: string,
+ *   search: string,
+ *   hash: string,
+ *   origin: string
+ * }|!Location)}
+ */
+export let Location;
+
 /**
  * Returns a Location-like object for the given URL. If it is relative,
  * the URL gets resolved.

src/viewer.js

@@ -19,8 +19,9 @@ import {getService} from './service';
 
 /**
  * @param {!Window} window
- * @return {!Viewer}
+ * @return {!./service/viewer-impl.Viewer}
  */
 export function viewerFor(window) {
-  return getService(window, 'viewer');
+  return /** @type {!./service/viewer-impl.Viewer} */ (
+      getService(window, 'viewer'));
 };

src/viewport.js

@@ -18,8 +18,9 @@ import {getService} from './service';
 
 /**
  * @param {!Window} window
- * @return {!Viewport}
+ * @return {!./service/viewport-impl.Viewport}
  */
 export function viewportFor(window) {
-  return getService(window, 'viewport');
+  return /** @type {!./service/viewport-impl.Viewport} */ (
+      getService(window, 'viewport'));
 };

src/vsync.js

@@ -19,8 +19,9 @@ import {getService} from './service';
 
 /**
  * @param {!Window} window
- * @return {!Vsync}
+ * @return {!./service/vsync-impl.Vsync}
  */
 export function vsyncFor(window) {
-  return getService(window, 'vsync');
+  return /** @type {!./service/vsync-impl.Vsync} */ (
+      getService(window, 'vsync'));
 };

src/xhr.js

@@ -19,8 +19,8 @@ import {getService} from './service';
 
 /**
  * @param {!Window} window
- * @return {!Xhr}
+ * @return {!./service/xhr-impl.Xhr}
  */
 export function xhrFor(window) {
-  return getService(window, 'xhr');
+  return /** @type {!./service/xhr-impl.Xhr} */ (getService(window, 'xhr'));
 };