🐛 Add several missing @return type annotations across the codebase

[rsimha]

It turns out there are hundreds of missing @return annotations across the AMP codebase, which means closure compiler has incomplete type information during minification.

PR highlights:

• Enables the jsdoc/require-returns lint rule to detect functions that return a value without specifying its type
• Adds @return {*} to all functions that are missing an annotation (with a TODO to specify the actual type )
• Specifies the correct return type for all errors detected by gulp check-types

Notes:

• This PR was written with the goal of making as few code changes as possible while satisfying the type checker
• The remaining @return {*} annotations that don't result in type errors can be gradually fixed in follow up PRs
• From here on, adding a function definition with a missing @return annotation will be flagged by gulp lint, and adding a function call where the return type is unspecified will be flagged by gulp check-types

[rsimha]

The majority of changes in this PR are JSDoc-only. A small number of code changes were needed to satisfy the type checker.

Adding several reviewers for visibility.
@jeffkaufman: A4A
@lannka: Ads
@zhouyx: Analytics
@erwinmombay: Babel plugins
@danielrozenberg: Infra
@jridgewell: Runtime
@newmuis: Stories
@dvoytenko: Subscriptions
@sparhami: UI

[jeffkaufman]

Thanks for noticing this, and starting the process of fixing it!

I commented on several places where you've used casts where I think annotating the return type would be better.

[rsimha]

@jeffkaufman Thanks for the patient review. The goal of this PR is to set us on the path to specifying type info wherever required with the minimum of code changes, while also alerting area owners that there is more to be done.

Unfortunately, some of your suggestions could have repercussions that require knowledge of the inner workings of Ads / AMP code. I'd argue that they are better off being made in separate pull requests by those who understand the code the most, so they can be tested in isolation, and trivially reverted if required.

[jeffkaufman]

@rsimha That's your call to make. I do think addressing the casts soon is valuable, though, because otherwise you can think the type system is protecting you from something when it's not.

[rsimha]

I do think addressing the casts soon is valuable, though, because otherwise you can think the type system is protecting you from something when it's not.

Definitely agree. In particularly, I'd love to remove the need for all the casts from Object to JsonObject across the codebase. I've filed #23567 to track the fix.

[sparhami]

I'm not sure that adding @return {*} TODO: Specify return type is a good change to make. It changes something that could be done as a lint (that could be required when changing a file) into a TODO that might never get done. I'm not sure if our lint setup allows for it, but i have worked in the past with linting setups that require you to make fixes only when a file is modified as part of a change.

[newmuis]

LGTM for stories code

[zhouyx]

analytics and consent related code LGTM

[rsimha]

@sparhami I've addressed your comments. Some replies below. PTAL.

I'm not sure that adding @return {*} TODO: Specify return type is a good change to make. It changes something that could be done as a lint (that could be required when changing a file) into a TODO that might never get done.

I don't fully agree with this assessment. There's an advantage to adding all the placeholder @return {*} annotations.

• Before this PR, call sites were essentially free to assume a function's return type.
• Now, gulp check-types will detect all newly added function calls with unspecified return types. Fixing them will be a one-step process because closure's error messages contain the expected type.
• Not putting in the placeholder and relying on the linter will make it a multi-step fix: Run gulp lint, then add a @return {*}, then run gulp check-types, and then specify the return type.

I'm not sure if our lint setup allows for it, but i have worked in the past with linting setups that require you to make fixes only when a file is modified as part of a change.

We did this in the past, but abandoned the approach because it made it unduly onerous to make small changes to files with numerous linter errors that would only show up when the file was edited.

[jridgewell]

Please file a tracking bug to fix these "TODO: Specify return type" todos.

[rsimha]

@jridgewell @erwinmombay Feedback addressed. PTAL.

Please file a tracking bug to fix these "TODO: Specify return type" todos.

#23582

[rsimha]

Tested this locally, Travis is green. Merging before more conflicts show up.

Edit: Spoke too soon. One merge conflict snuck in: #23577 (comment). Fix sent out with #23584