closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

Missing JSC_MISSING_REQUIRE when importing `@interface` type via `goog.requireType` in a `goog.module`

Open cpcallen opened this issue 3 years ago • 5 comments

The following code, when compiled with CHECK_REQUIRES, produces a JSC_MISSING_REQUIRE_IN_PROVIDES_FILE warning:

Parent:

goog.provide('Interface');

/** @interface */
Interface = function() {};

Child:

goog.requireType('Interface');

/** @constructor @implements {Interface} */
function Implementation() {}

Warnings:

input1:3:30: WARNING - [JSC_MISSING_REQUIRE_IN_PROVIDES_FILE] 'Interface' references a namespace which was not required by this file.
Please add a goog.require.
  3| /** @constructor @implements {Interface} */
                                   ^^^^^^^^^

0 error(s), 1 warning(s), 55.5% typed

~~I believe this warning is spurious, because it does not occur if you use goog.require instead of goog.requireType, even though the import is used only for type annotation, and it does not occur if you use goog.module instead of goog.provide.~~

EDIT: Unfortunately, no error is emitted if you use goog.require instead of goog.requireType, even though the import is used only for type annotation, and it does not occur if you use goog.module instead of goog.provide even though (from discussion below) it appears that one should be.

cpcallen avatar Aug 12 '21 16:08 cpcallen

@lauraharker I think I need your look on this one. Do you think this is a valid bug?

h-joo avatar Aug 16 '21 10:08 h-joo

This was an intentional choice when the CheckMissingRequires pass was written. The problem was that the typechecker doesn't support @implements/@extends of a weak require very well. See also https://github.com/google/closure-compiler/issues/3583.

Here's the code in question: https://github.com/google/closure-compiler/blob/78ce1e53ae22900b5f7d5dad19a34babf1273763/src/com/google/javascript/jscomp/CheckMissingRequires.java#L146-L153.

One fix might be making the error message more explicit about why this error pops up for requireTyped names.

lauraharker avatar Aug 17 '21 20:08 lauraharker

Ahh, interesting. Am I correct in understanding that the bug is not the error when using goog.provides, but the lack of error when using goog.module? If so, that is quite surprising, and violates my (I think not entirely unreasonable) assumption of a rule that that an import which is mentioned only in comments (i.e., used only as a type) need only be requireTyped rather than required. It seems especially egregious that it is interface types which need be required, since they will never be mentioned in the source other than in comments as a type.

Is the requirement to require interface types documented somewhere other than in CheckMissingRequires.java?

cpcallen avatar Aug 24 '21 19:08 cpcallen

You should alway see the error when a interface is implemented if it isn't goog.required regardless of whether the original source is a goog.provide or a goog.module.

concavelenz avatar Aug 28 '21 22:08 concavelenz

I've updated the title and description to better match my updated understanding of the desired/expected behaviour based on the discussion above.

cpcallen avatar Aug 31 '21 18:08 cpcallen