I2I: Restoring AMP’s Type Checking

[rcebulko]

Overview

For the past year, type checking has been accidentally disabled for the amphtml repository. Unfortunately, flipping the switch back on is not so simple because during this time period a very large number (1000s) of type errors have organically been created. Restoring type-checking would help avoid bugs and potentially allow for more advanced compilation optimizations

This document proposes a multi-pronged approach for gradually restoring type checking to AMP binaries.

Goals

• All JS files that end up in AMP binaries are type checked as part of CI.
• Organize AMP’s extern files and type definitions

Non-goals

• Type-checking build-system or test files.
• Conversion to TypeScript
• Exporting type definitions

Current state

• Type checking: All src and extensions files, except for the newly introduced src/core have type checking disabled.
• **Externs: **All of AMP’s externs live within 5 files in build-system/externs, and there are 100s of them. This has similar issues with having just a few giant CSS files. It is hard to track which source files depend on which externs and therefore hard to know when it is ok to delete one.
  • **"migrated externs" **- We’ve begun splitting the massive *.extern.js files in build-system to be co-located with the files that need them. Currently only src/core/\*\*/\*.extern.js (core externs)

Approaches

At a high level, there are two kinds of approaches we’d like to pursue: vertical and horizontal.

• Vertical means establishing a set of files that fully pass all type-checking rules and incrementally increasing the set of files in that set until everything is included.
• Horizontal means establishing specific type-checking rules that all files must pass, and then gradually increasing the set of rules until all rules are included.

Horizontal: Gradually increase the minimum type quality via crowdsourcing

Goal: Establish a floor of type quality that each and every AMP Developer is required to maintain.

Source files: --local_changes in each branch.

Encourage (force) developers to ensure the types within the files they are modifying have a baseline of quality. We do this by failing CI when specific JSC errors are found in the modified files. We can also gradually increase the failable error types as time goes on. The first set of errors we should include are

• JSC_TYPE_PARSE_ERROR - Types should at least parse properly.
• JSC_INVALID_PARAM - Parameter types should parse.

Top-down: expand passing targets

Goal: Lock down passing file(s), preventing new type errors from being introduced to them

**Source files: **any

Externs: migrated externs and build-system externs

• Single catch-all pride type-check target enabling CI type-checking
• Works well for files that could pass type-checking but haven't yet been sorted/organized via bottom-up approach into a folder that is fully passing

Bottom-up: target top-level src directories

Goal: Establish and lock-down type-safe directories

Source files: subdirectories of src (ex. context, polyfills, purifier, etc)

**Externs: **its own migrated externs along with core externs

• Minimal surface of external dependencies can be provided via shame.extern.js
  • This "shame" file contains externs that exist only during migration, and act as a to-do list of types that should be condensed or organized elsewhere
  • This also acts to unblock bug-fixes when a type is needed but not yet defined in available externs
• As each is made to pass, enable it in CI
• As files move to core, they'll be updated to pass (much of src/utils and src/\*.js flatfiles)
• Works well for vertically-separated code with minimal external dependencies

/cc @samouri

[samouri]

Can we switch the check-types task to using tsc instead of closure? cc @rileyajones

[rcebulko]

Can we switch the check-types task to using tsc instead of closure? cc @rileyajones

That would/will require re-writing almost all of the current typecheck annotations in src

[samouri]

Can we switch the check-types task to using tsc instead of closure? cc @rileyajones

That would/will require re-writing almost all of the current typecheck annotations in src

Why? tsc understands JSDoc types, and is currently powering the amp check-build-system command.
I could see externs being a problem (might need to convert those to .d.ts files)

[rcebulko]

Why? tsc understands JSDoc types, and is currently powering the amp check-build-system command.
I could see externs being a problem (might need to convert those to .d.ts files)

There's enough incongruities to make it non-trivial (I timeboxed a few hours to attempt at one point and progress was slow). Transitioning means updating everything at once, or disabling type-checking again in the interim (since making things work with TS == breaking them in CC). Not all JSDoc/CC annotations are handled by TS. CC supports type-narrowing after something like devAssertString; TS does not and requires an explicit cast after the assert. Getting around some of this requires not just annotations, but explicit TS syntax like function devAssertString(input): asserts input is string, which means the code is no longer JavaScript, which opens its own box of corners to deal with.

I would love if we used tsc for type-checking. Please try, and I hope you have better luck than I did :)

[rcebulko]

Also, I don't know if you've ever seen what the syntax looks like to try to add global externs or add properties to global symbols like Window, Element, etc. but it's not great

[samouri]

Now that all of src/core is typechecked, and we have a straightforward path for restoring types to Bento, I think this is worth closing. WDYT @rcebulko ?

[rcebulko]

Now that all of src/core is typechecked, and we have a straightforward path for restoring types to Bento, I think this is worth closing. WDYT @rcebulko ?

Do the honors!

[rileyajones]

👏 🎉