Use goog.requireType when importing interfaces (etc.)

[cpcallen]

The basics

• I branched from goog_module
• My pull request is against goog_module
• My code follows the style guide
• I have run npm test.

The details

Resolves

Related to #5026

Proposed Changes

Use goog.requireType instead of goog.require when:

• importing modules containing only interfaces and
• Importing any other names used only in type annotations.

Also ensure that an import is marked /* eslint-disable-next-line no-unused-vars */ if and only if it is a goog.requireType.

Reason for Changes

Interfaces have no code, so should never be referred to outside of (JSDoc) comments, and so the modules that define only interfaces never need to be goog.require'd—goog.requireType is always sufficient. This is also true of other non-interface types which appear only in comments.

Test Coverage

• Ran npm test; all tests passed.
• Ran npm run test:compile:advanced (with some local temporary fixes to un-break it); no increase in number of warnings generated.

Additional Information

I've applied this change to all the I* modules (hungarian notation FTW!)

[cpcallen]

Rebased to fix conflicts in core/workspace.js, .../workspace_svg.js and tests/deps.js due to migration of workspace.js.

[rachel-fenichel]

Continuation of a conversation from #5317:

Monica: Interfaces that are implemented need to be require'd explicitely.

Chris: I'm not aware of any such requirement. Can you direct me to more information about this? As far as I knew interfaces are nothing but a type—the only code is an empty function () {}—so the interface name should only appear in comments and so there ought not to be any reason to require it.

(Things are different in TypeScript, of course, where interface names appear outside of comments.)

Monica got this requirement from me. I know that I said this because of compiler errors or warnings that I was getting from using requireType instead of require.

I think that those were warnings that I got when I added the stricterMissingRequires flag to our error list in gulpfiles/build_tasks.js and then ran npm run build:strict:log and looked at the results.

[cpcallen]

@rachel-fenichel: thanks for copying over the discussion.

Monica got this requirement from me. I know that I said this because of compiler errors or warnings that I was getting from using requireType instead of require.

I think that those were warnings that I got when I added the stricterMissingRequires flag to our error list in gulpfiles/build_tasks.js and then ran npm run build:strict:log and looked at the results.

That is super interesting. Ok, short summary of what I have learned this morning:

• According to the comment by @samelhusseini in build_tasks.js the flag is now just missingRequire, and indeed the stricterMissingRequire flag now causes an error:

  gulp-google-closure-compiler: java.lang.NullPointerException: No warning class for name: stricterMissingRequire
  

• Enabling missingRequire for the code in this PR cause just three additional errors to be emitted by npm build:strict:

  core-bubble.js:45:16: ERROR - [JSC_MISSING_REQUIRE_IN_PROVIDES_FILE] 'Blockly.IBubble' references a namespace which was not required by this file.
  Please add a goog.require.
    45|  * @implements {Blockly.IBubble}
                        ^^^^^^^^^^^^^^^
  

  core-metrics_manager.js:31:16: ERROR - [JSC_MISSING_REQUIRE_IN_PROVIDES_FILE] 'Blockly.IMetricsManager' references a namespace which was not required by this file.
  Please add a goog.require.
    31|  * @implements {Blockly.IMetricsManager}
                        ^^^^^^^^^^^^^^^^^^^^^^^
  

  core-renderers-common-renderer.js:41:16: ERROR - [JSC_MISSING_REQUIRE_IN_PROVIDES_FILE] 'Blockly.IRegistrable' references a namespace which was not required by this file.
  Please add a goog.require.
    41|  * @implements {Blockly.IRegistrable}
                        ^^^^^^^^^^^^^^^^^^^^
  

  Notably these are the only three files touched which have not yet been migrated to goog.module and named requires. I note that the error is "JSC_MISSING_REQUIRE_IN_PROVIDES_FILE", and I also see a "JSC_MISSING_REQUIRE_TYPE_IN_PROVIDES_FILE", and that these are different from "JSC_MISSING_REQUIRE" which is emitted for migrated files.
• This makes me suspicious that the error actually in the compiler, not the code (I know… it's never a compiler error—but it's plausible, since it is specifically an error generated for goog.provides files, which is probably a much less tested code path these days).

So I think the changes to already-migrated files are fine. For the not-yet-migrated ones, I could back out the change for now, but I think it's probably better to leave it in so that whoever does the migration doesn't need to remember to do it then.

[rachel-fenichel]

I retain my standard skepticism about whether it's a compiler bug--but since you were able to reproduce the errors in the unconverted files, and show that they don't happen in the converted files, this seems safe. Approved.

[cpcallen]

I retain my standard skepticism about whether it's a compiler bug…

I've filed google/closure-compiler#3851; we'll see what they say.

[cpcallen]

I retain my standard skepticism about whether it's a compiler bug…

I've filed google/closure-compiler#3851; we'll see what they say.

It seems that you are correct and I am wrong: there is a compiler bug, but apparently the bug is that no error message is issued when goog.requireTypeing an interface type in a goog.module. The mind boggles.