🏗 Rewrite and enable type-checking on src/core target (#33921)

* add compile options to not add anything unspecified * Rewrite check-types task to support checking individual targets * Update conformance allowlist for string helpers now in core * Define extern types for existing core files * Update core files to pass type-checking * %s/object.externs.js/object.extern.js/ * Remove duplicate externs defined in core * Opt-out failing targets from type-checking; let flag override * Clean up extern list code * Lint fixes * destructure * typo * comments
ampproject · Apr 21, 2021 · df20a6f · df20a6f
1 parent e14c2b6
commit df20a6f
Show file tree

Hide file tree

Showing 11 changed files with 281 additions and 108 deletions.
diff --git a/build-system/compile/compile.js b/build-system/compile/compile.js
@@ -194,17 +194,19 @@ function getSrcs(entryModuleFilenames, outputDir, outputFilename, options) {
  * @return {!Object}
  */
 function generateCompilerOptions(outputDir, outputFilename, options) {
-  const baseExterns = [
-    'build-system/externs/amp.extern.js',
-    'build-system/externs/dompurify.extern.js',
-    'build-system/externs/layout-jank.extern.js',
-    'build-system/externs/performance-observer.extern.js',
-    'third_party/web-animations-externs/web_animations.js',
-    'third_party/moment/moment.extern.js',
-    'third_party/react-externs/externs.js',
-    'build-system/externs/preact.extern.js',
-    'build-system/externs/weakref.extern.js',
-  ];
+  // Determine externs
+  let externs = options.externs || [];
+  if (!options.noAddDeps) {
+    externs = [
+      'third_party/web-animations-externs/web_animations.js',
+      'third_party/react-externs/externs.js',
+      'third_party/moment/moment.extern.js',
+      ...globby.sync('src/core{,/**}/*.extern.js'),
+      ...globby.sync('build-system/externs/*.extern.js'),
+      ...externs,
+    ];
+  }
+
   const hideWarningsFor = [
     'third_party/amp-toolbox-cache-url/',
     'third_party/caja/',
@@ -230,11 +232,6 @@ function generateCompilerOptions(outputDir, outputFilename, options) {
     ? options.wrapper.replace('<%= contents %>', '%output%')
     : `(function(){%output%})();`;
   wrapper = `${wrapper}\n\n//# sourceMappingURL=${outputFilename}.map`;
-  let externs = baseExterns;
-  if (options.externs) {
-    externs = externs.concat(options.externs);
-  }
-  externs.push('build-system/externs/amp.multipass.extern.js');
 
   /**
    * TODO(#28387) write a type for this.
@@ -264,9 +261,9 @@ function generateCompilerOptions(outputDir, outputFilename, options) {
     module_resolution: 'NODE',
     package_json_entry_names: 'module,main',
     process_common_js_modules: true,
-    // This strips all files from the input set that aren't explicitly
+    // PRUNE strips all files from the input set that aren't explicitly
     // required.
-    dependency_mode: 'PRUNE',
+    dependency_mode: options.noAddDeps ? 'SORT_ONLY' : 'PRUNE',
     output_wrapper: wrapper,
     source_map_include_content: !!argv.full_sourcemaps,
     // These arrays are filled in below.
@@ -277,6 +274,7 @@ function generateCompilerOptions(outputDir, outputFilename, options) {
     hide_warnings_for: hideWarningsFor,
     // TODO(amphtml): Change 'QUIET' to 'DEFAULT'.
     warning_level: argv.warning_level ?? options.warningLevel ?? 'QUIET',
+    extra_annotation_name: ['visibleForTesting', 'restricted'],
   };
   if (argv.pseudo_names) {
     // Some optimizations get turned off when pseudo_names is on.
@@ -415,12 +413,9 @@ async function compile(
     outputFilename,
     options
   );
-  const srcs = getSrcs(
-    entryModuleFilenames,
-    outputDir,
-    outputFilename,
-    options
-  );
+  const srcs = options.noAddDeps
+    ? entryModuleFilenames.concat(options.extraGlobs || [])
+    : getSrcs(entryModuleFilenames, outputDir, outputFilename, options);
   const transformedSrcFiles = await Promise.all(
     globby
       .sync(srcs)

diff --git a/build-system/externs/amp.extern.js b/build-system/externs/amp.extern.js
@@ -59,16 +59,6 @@ FormDataWrapperInterface.prototype.getFormData = function () {};
 
 FormData.prototype.entries = function () {};
 
-/**
- * A type for Objects that can be JSON serialized or that come from
- * JSON serialization. Requires the objects fields to be accessed with
- * bracket notation object['name'] to make sure the fields do not get
- * obfuscated.
- * @constructor
- * @dict
- */
-function JsonObject() {}
-
 /**
  * @typedef {{
  *   YOU_MUST_USE: string,
@@ -450,12 +440,6 @@ HTMLAnchorElement.prototype.origin;
 /** @typedef {number}  */
 var time;
 
-/**
- * This type signifies a callback that can be called to remove the listener.
- * @typedef {function()}
- */
-var UnlistenDef;
-
 /**
  * Just an element, but used with AMP custom elements..
  * @constructor @extends {Element}

diff --git a/build-system/tasks/check-types.js b/build-system/tasks/check-types.js
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+const argv = require('minimist')(process.argv.slice(2));
+const globby = require('globby');
 const {
   createCtrlcHandler,
   exitCtrlcHandler,
@@ -23,87 +25,150 @@ const {
 } = require('../compile/debug-compilation-lifecycle');
 const {cleanupBuildDir, closureCompile} = require('../compile/compile');
 const {compileCss} = require('./css');
+const {cyan, green, yellow, red} = require('kleur/colors');
 const {extensions, maybeInitializeExtensions} = require('./extension-helpers');
 const {log} = require('../common/logging');
 const {typecheckNewServer} = require('../server/typescript-compile');
 
+const EXTERNS_GLOB = 'src/core{,/**}/*.extern.js';
+
+/**
+ * Generates a list of source file paths for extensions to type-check
+ * Must be run after `maybeInitializeExtensions`
+ * @function
+ * @return {!Array<string>}
+ */
+const getExtensionSrcPaths = () =>
+  Object.values(extensions)
+    .filter((ext) => !ext.noTypeCheck)
+    .map(({name, version}) => `extensions/${name}/${version}/${name}.js`)
+    .sort();
+
+/**
+ * The main configuration location to add/edit targets for type checking.
+ * Properties besides `entryPoints` are passed on to `closureCompile` as
+ * options. * Values may be objects or functions, as some require initialization
+ * or filesystem access and shouldn't be run until needed.
+ * @type {Object<string, Object|function():Object>}
+ */
+const TYPE_CHECK_TARGETS = {
+  'src': {
+    entryPoints: [
+      'src/amp.js',
+      'src/amp-shadow.js',
+      'src/inabox/amp-inabox.js',
+      'ads/alp/install-alp.js',
+      'ads/inabox/inabox-host.js',
+      'src/web-worker/web-worker.js',
+    ],
+    extraGlobs: ['src/inabox/*.js', '!node_modules/preact'],
+    warningLevel: 'QUIET',
+  },
+  'src-core': () => ({
+    externs: globby.sync(EXTERNS_GLOB),
+    extraGlobs: [
+      // Include all core JS files
+      'src/core/{,**/}*.js',
+      // Exclude all core extern files (already included via externs)
+      `!${EXTERNS_GLOB}`,
+    ],
+  }),
+  'extensions': () => ({
+    entryPoints: getExtensionSrcPaths(),
+    extraGlobs: ['src/inabox/*.js', '!node_modules/preact'],
+    warningLevel: 'QUIET',
+  }),
+  'integration': {
+    entryPoints: '3p/integration.js',
+    externs: ['ads/ads.extern.js'],
+    warningLevel: 'QUIET',
+  },
+  'ampcontext': {
+    entryPoints: '3p/ampcontext-lib.js',
+    externs: ['ads/ads.extern.js'],
+    warningLevel: 'QUIET',
+  },
+  'iframe-transport-client': {
+    entryPoints: '3p/iframe-transport-client-lib.js',
+    externs: ['ads/ads.extern.js'],
+    warningLevel: 'QUIET',
+  },
+};
+
+/**
+ * Performs closure type-checking on the target provided.
+ * @param {string} targetName key in TYPE_CHECK_TARGETS
+ * @return {!Promise<void>}
+ */
+async function typeCheck(targetName) {
+  let target = TYPE_CHECK_TARGETS[targetName];
+  if (typeof target == 'function') {
+    target = target();
+  }
+
+  if (!target) {
+    log(
+      red('ERROR:'),
+      'No type-check configuration defined for target',
+      cyan(targetName)
+    );
+    throw new Error(
+      `No type-check configuration defined for target ${targetName}`
+    );
+  }
+
+  const {entryPoints = [], ...opts} = target;
+  // If no entry point is defined, we want to scan the globs provided without
+  // injecting extra dependencies.
+  const noAddDeps = !entryPoints.length;
+  // If the --warning_level flag is passed explicitly, it takes precedence.
+  opts.warningLevel = argv.warning_level || opts.warningLevel || 'VERBOSE';
+
+  // For type-checking, QUIET suppresses all warnings and can't affect the
+  // resulting status, so there's no point in doing it.
+  if (opts.warningLevel == 'QUIET') {
+    log(
+      yellow('WARNING:'),
+      'Warning level for target',
+      cyan(targetName),
+      `is set to ${cyan('QUIET')}; skipping`
+    );
+    return;
+  }
+
+  await closureCompile(entryPoints, './dist', `${targetName}-check-types.js`, {
+    noAddDeps,
+    include3pDirectories: !noAddDeps,
+    includePolyfills: !noAddDeps,
+    typeCheckOnly: true,
+    ...opts,
+  });
+  log(green('SUCCESS:'), 'Type-checking passed for target', cyan(targetName));
+}
+
 /**
  * Runs closure compiler's type checker against all AMP code.
  * @return {!Promise<void>}
  */
 async function checkTypes() {
   const handlerProcess = createCtrlcHandler('check-types');
+
+  // Prepare build environment
   process.env.NODE_ENV = 'production';
   cleanupBuildDir();
   maybeInitializeExtensions();
   typecheckNewServer();
-  const compileSrcs = [
-    'src/amp.js',
-    'src/amp-shadow.js',
-    'src/inabox/amp-inabox.js',
-    'ads/alp/install-alp.js',
-    'ads/inabox/inabox-host.js',
-    'src/web-worker/web-worker.js',
-  ];
-  const extensionValues = Object.keys(extensions).map((key) => extensions[key]);
-  const extensionSrcs = extensionValues
-    .filter((ext) => !ext.noTypeCheck)
-    .map((ext) => `extensions/${ext.name}/${ext.version}/${ext.name}.js`)
-    .sort();
   await compileCss();
-  log('Checking types...');
+
+  // Use the list of targets if provided, otherwise check all targets
+  const targets = argv.targets
+    ? argv.targets.split(/,/)
+    : Object.keys(TYPE_CHECK_TARGETS);
+
+  log(`Checking types for targets: ${targets.map(cyan).join(', ')}`);
   displayLifecycleDebugging();
-  await Promise.all([
-    closureCompile(
-      compileSrcs.concat(extensionSrcs),
-      './dist',
-      'check-types.js',
-      {
-        include3pDirectories: true,
-        includePolyfills: true,
-        extraGlobs: ['src/inabox/*.js', '!node_modules/preact'],
-        typeCheckOnly: true,
-        warningLevel: 'QUIET', // TODO(amphtml): Make this 'DEFAULT'
-      }
-    ),
-    // Type check 3p/ads code.
-    closureCompile(
-      ['3p/integration.js'],
-      './dist',
-      'integration-check-types.js',
-      {
-        externs: ['ads/ads.extern.js'],
-        include3pDirectories: true,
-        includePolyfills: true,
-        typeCheckOnly: true,
-        warningLevel: 'QUIET', // TODO(amphtml): Make this 'DEFAULT'
-      }
-    ),
-    closureCompile(
-      ['3p/ampcontext-lib.js'],
-      './dist',
-      'ampcontext-check-types.js',
-      {
-        externs: ['ads/ads.extern.js'],
-        include3pDirectories: true,
-        includePolyfills: true,
-        typeCheckOnly: true,
-        warningLevel: 'QUIET', // TODO(amphtml): Make this 'DEFAULT'
-      }
-    ),
-    closureCompile(
-      ['3p/iframe-transport-client-lib.js'],
-      './dist',
-      'iframe-transport-client-check-types.js',
-      {
-        externs: ['ads/ads.extern.js'],
-        include3pDirectories: true,
-        includePolyfills: true,
-        typeCheckOnly: true,
-        warningLevel: 'QUIET', // TODO(amphtml): Make this 'DEFAULT'
-      }
-    ),
-  ]);
+
+  await Promise.all(targets.map(typeCheck));
   exitCtrlcHandler(handlerProcess);
 }
 
@@ -117,6 +182,7 @@ checkTypes.description = 'Check source code for JS type errors';
 checkTypes.flags = {
   closure_concurrency: 'Sets the number of concurrent invocations of closure',
   debug: 'Outputs the file contents during compilation lifecycles',
+  targets: 'Comma-delimited list of targets to type-check',
   warning_level:
     "Optionally sets closure's warning level to one of [quiet, default, verbose]",
 };
diff --git a/build-system/test-configs/conformance-config.textproto b/build-system/test-configs/conformance-config.textproto
@@ -104,15 +104,15 @@ requirement: {
   error_message: 'string.prototype.trimStart is not allowed'
   value: 'string.prototype.trimStart'
   value: 'string.prototype.trimLeft'
-  allowlist: 'src/string.js'
+  allowlist: 'src/core/types/string.js'
 }
 
 requirement: {
   type: BANNED_PROPERTY_CALL
   error_message: 'string.prototype.trimEnd is not allowed'
   value: 'string.prototype.trimEnd'
   value: 'string.prototype.trimRight'
-  allowlist: 'src/string.js'
+  allowlist: 'src/core/types/string.js'
 }
 
 requirement: {

diff --git a/src/.eslintrc.js b/src/.eslintrc.js
@@ -59,5 +59,9 @@ module.exports = {
       ],
       'rules': {'import/no-restricted-paths': isCiBuild() ? 0 : 1},
     },
+    {
+      'files': ['./core/window.extern.js'],
+      'rules': {'local/no-global': 0},
+    },
   ],
 };
diff --git a/src/core/assert.js b/src/core/assert.js
@@ -33,11 +33,12 @@ import {remove} from './types/array';
  * - messageArray: The elements of the substituted message as non-stringified
  *   elements in an array. When e.g. passed to console.error this yields
  *   native displays of things like HTML elements.
- * @param {string} sentinel
+ * @param {?string} sentinel
  * @param {T} shouldBeTruthy
  * @param {string} opt_message
  * @param {...*} var_args Arguments substituted into %s in the message
  * @return {T}
+ * @template T
  * @throws {Error} when shouldBeTruthy is not truthy.
  */
 function assertion(
@@ -93,6 +94,7 @@ function assertion(
  * @param {*=} opt_8 Optional argument
  * @param {*=} opt_9 Optional argument
  * @return {T}
+ * @template T
  * @throws {UserError} when shouldBeTruthy is not truthy.
  * @closurePrimitive {asserts.truthy}
  */
@@ -140,6 +142,7 @@ export function pureUserAssert(
  * @param {*=} opt_8 Optional argument
  * @param {*=} opt_9 Optional argument
  * @return {T}
+ * @template T
  * @throws {Error} when shouldBeTruthy is not truthy.
  * @closurePrimitive {asserts.truthy}
  */