Amphtml: `third_party/` code must not be treated as closure code

[rsimha]

AMP components like extensions/amp-subscriptions/ include third party code that is checked in to our repo under third_party/. When we run the type checker via gulp check-types or minify code via gulp dist, closure compiler treats the code in third_party/ as if it were closure code.

• This has always been the case, but we've never realized it
• This is risky because code in third_party/ may interfere with compilation optimizations around inlining, devirtualization, minification, etc.

@dvoytenko recently discovered this due to following events:

1. The version of closure compiler used by AMP was upgraded from an old version to v20190301 (https://github.com/ampproject/amphtml/pull/21618)
2. The latest compiler version has a bug where it does not properly detect relative path annotations (https://github.com/google/closure-compiler/issues/3041)
3. The latest update to third_party/subscriptions-project/swg.js contains such an annotation
4. As a result, the type checker complains about a bad closure type annotation in third party code.

To fix this, we must no longer treat code in third_party/ as closure code.

• One way to do this is to strip away all JSDoc comments before passing the code to closure compiler.
• There may be a way to pass in options to closure compiler to treat certain files as non-closure code.

This issue is meant to discuss and track a fix.

[rsimha]

/cc @erwinmombay @dvoytenko @choumx @cramforce for visibility and for ideas.

[cramforce]

s/\/\*\*/\/*x/g may do it. (Remove the JSDoc /** open marker.

[dvoytenko]

@cramforce yes, /** -> /* removes the problem.

[cramforce]

I'd just keep the number of chars the same for source maps :)

On Thu, Apr 11, 2019 at 1:57 PM Dima Voytenko [e-mail]
wrote:

@cramforce https://github.com/cramforce yes, /* -> / removes the
problem.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/21828#issuecomment-482310910,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAV4TZAJ6MLENWJTDAOYJ3PP6QZZANCNFSM4HFLHCDA
.

[choumx]

I hit this during the last CC upgrade, which started failing on missing type paths. So this might've been an issue even before Raghu's PR.

If this regex is still being used, we should just update it to catch this instance.

https://github.com/ampproject/amphtml/blob/71339c2a2455b3c98b2cf03c541902d62c9b711f/third_party/subscriptions-project/README.amp#L8-L10

Add a |< to the first capture group so it catches Promise<./foo> in addition to !./foo and ?./foo:

(!|\?|<)\.[^>)}]*

[rsimha]

In #21830, I tried what at first seemed like an elegant solution to replace /** with /* in the gulp stream that's passed to gulp-closure-compiler, but it didn't work because gulp-closure-compiler ignores the contents of the file stream passed to it, and instead, picks up the filename and re-reads it from disc.

@choumx

• You're right! I just noticed that your closure compiler upgrade from last year made wholesale changes to typenames in swg.js
• Not sure that regex is still being used with each upgrade. In fact, I believe @dvoytenko would like to avoid having to touch any code in swg.js.

I'll pick this up tomorrow. I'm still in search of a good way to replace the contents of source files before passing them to closure compiler. (I believe multi-pass gulp dist compiles files in-place, so we can't modify files in the source directory.)

[erwinmombay]

@rsimha , you might need to write the swg files into build folder (transformed), and change the paths. this trick might not work for singlepass though, ill check

[choumx]

I think @jridgewell has looked into modifying source files pre-compile to fix source maps due to version string replacement and license minification, though IIRC there isn't a good way yet.

[rsimha]

We currently use gulp-closure-compiler, which is no longer receiving updates. (The repo has been deleted too.)

I think we can get around this by directly using google-closure-compiler with gulp.src. See here. Trying that out.

[jridgewell]

Yah, I looked into the source code of the gulp-closure-compiler and it was rereading files directly from disk. When I tried writing the modifications to a temp directory before passing to the complier, it was getting other errors (or just ignoring them completely).

Switching to Closure's official gulp plugin might work. It looks like they take the modifications buffer and apply it before running the compiler.

[rsimha]

Crap, it seems like there's no way to get closure's official gulp plugin to use our custom runner.jar. Digging in...

/cc @erwinmombay

[erwinmombay]

should be possible, see https://github.com/ampproject/amphtml/blob/master/build-system/single-pass.js#L61

[rsimha]

With ~#21868~ #21900, we are no longer blocked by the weird behavior of gulp-closure-compiler. The coast is now clear to replace JSDoc comments in third_party/. This is coming up in #21878.

[rsimha]

Update: A mechanism for removing JSDoc from third party code is now available in #21878. We now need a strategy for fixing type errors in extensions code that uses third party code. See https://github.com/ampproject/amphtml/pull/21878#issuecomment-483908074

[evmar]

(I work on JS within Google and have investigated this problem in depth; Googlers can read go/tpl-js .)

In general you cannot compile arbitrary third-party JS in Closure because the code may use patterns that the optimizer eats. If you are ok with this and only care about errors, I believe the --hide_warnings_for=<path prefix> flag actually turns off all error checking in files matching the path. I am not confident on this but I think this has worked for us and from a glance at the code (which I also don't understand too well) it appears it turns checks off:

https://github.com/google/closure-compiler/blob/30523994c65c8abb5d1d14f17ee6a9945c45f845/src/com/google/javascript/jscomp/ShowByPathWarningsGuard.java#L61

Using --hide_warnings_for=third_party/ is very common within Google.

[dvoytenko]

@evmar thanks a lot!

@rsimha Do you recollect what were the kind of errors we saw with --hide_warnings_for?

@evmar Generally, our goals are:

1. Import library as close to the source as we can.
2. Ensure that we do not have overly aggressive optimizations that we cannot guarantee would work correctly on this subset of code specifically. We still want aggressive optimizations in our 1p code though.
3. We do need to connect some types. E.g. if a TPL exports a function that takes in an interface, we need to be able to correctly implement this interface in 1p code and not be afraid that some optimization techniques would break it.

[rsimha]

Do you recollect what were the kind of errors we saw with --hide_warnings_for?

@dvoytenko I tried @evmar's suggestion and added third_party/subscriptions-project/ to the set of --hide_warnings_for directories.

1. If I do this in addition to your original plan of removing JSDoc annotations from third_party/, we still see a couple dozen type errors in extensions/amp-subscriptions-google/ and extensions/amp-subscriptions/.
2. If the suggestion is to no longer remove JSDoc annotations from third_party/, and merely add third_party/subscriptions-project/ to the set of --hide_warnings_for directories, it's a no-op because we aren't seeing any type errors at present. Also, I don't think this will prevent the compiler optimizations to third party code that you were trying to avoid.

The latest code is available at #21878.

[dvoytenko]

@rsimha what happens if we keep the annotations and set the hide_warnings_for. TBH, I was under the impression that we always had hide_warnings_for set up for the whole third_party.

[rsimha]

what happens if we keep the annotations and set the hide_warnings_for. TBH, I was under the impression that we always had hide_warnings_for set up for the whole third_party.

See item 2 in https://github.com/ampproject/amphtml/issues/21828#issuecomment-493533602.

[dvoytenko]

@rsimha , yes, but i was wondering if you had the log from before. There are no longer errors because we compensated on the SwG's side by changing it at the source. In other words, it's likely to happen again.

[rsimha]

The only logs I have are at https://github.com/ampproject/amphtml/pull/21878#issuecomment-483908074, but I'm not sure if that's what you're looking for. Maybe you can roll back swg.js to a revision that had errors and rerun gulp check-types?

[rsimha]

After discussing offline with @dvoytenko, there's no pressing need to do something as drastic as removing all type annotations from swg.js.

Part of the reason behind this discussion was https://github.com/ampproject/amphtml/pull/21827#pullrequestreview-225734306, which turned out to be a red herring due to an unnecessary global find-replace I did while upgrading closure compiler.

For now, we are fine with unknown types in swg.js because we ignore errors that stem from them:

https://github.com/ampproject/amphtml/blob/2b9201c681fa9e38a50979fc667571123c174b1e/build-system/compile/compile.js#L92-L95

This issue can be closed until we face other problems due to types in third party code.