Skip to content

Commit a5b37b3

Browse files
lauraharkercopybara-github
lauraharker
authored and
copybara-github
committed
Remove support for module$contents$/module$exports$ names in JSDoc types
(Second attempt at this, first was cl/549986197) PiperOrigin-RevId: 550988031
1 parent c7547ba commit a5b37b3

File tree

3 files changed

+193
-1
lines changed

3 files changed

+193
-1
lines changed

‎src/com/google/javascript/jscomp/CheckJSDoc.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
159159
validateClosurePrimitive(n, info);
160160
validateReturnJsDoc(n, info);
161161
validateTsType(n, info);
162+
validateJsDocTypeNames(info);
162163
}
163164

164165
private void validateSuppress(Node n, JSDocInfo info) {
@@ -767,4 +768,47 @@ private void validateTsType(Node n, JSDocInfo info) {
767768
private static boolean isFromTs(Node n) {
768769
return n.getSourceFileName().endsWith(".closure.js");
769770
}
771+
772+
private final CheckJsdocTypes checkJsDocTypesVisitor = new CheckJsdocTypes();
773+
774+
private void validateJsDocTypeNames(JSDocInfo info) {
775+
if (info == null) {
776+
return;
777+
}
778+
for (Node typeNode : info.getTypeNodes()) {
779+
NodeUtil.visitPreOrder(typeNode, checkJsDocTypesVisitor);
780+
}
781+
}
782+
783+
/** Ban any references to compiler internal implementation details */
784+
private static final class CheckJsdocTypes implements NodeUtil.Visitor {
785+
786+
@Override
787+
public void visit(Node typeRefNode) {
788+
if (!typeRefNode.isStringLit()) {
789+
return;
790+
}
791+
// A type name that might be simple like "Foo" or qualified like "foo.Bar".
792+
final String typeName = typeRefNode.getString();
793+
int dot = typeName.indexOf('.');
794+
String rootOfType = dot == -1 ? typeName : typeName.substring(0, dot);
795+
796+
// Prevent handwritten JS from referencing a module export or module content name that's
797+
// synthesized by ClosureRewriteModule. Prefix the JSDoc references with
798+
// "UnrecognizedType_" and leave it to the typechecker to report a JSC_UNRECOGNIZED_TYPE_ERROR
799+
// * why not report an error here? even if we did report an error, we
800+
// should still add the prefix to ensure the typechecker doesn't resolve this type. Some
801+
// builds and/or files suppress type errors.
802+
// TODO(lharker): consider reporting an unsuppressible error instead of doing this
803+
// rewriting, if we can clean up all existing violations of this error.
804+
// * why do this here instead of in the ClosureRewriteModule pass? the Es6RewriteModule runs
805+
// before ClosureRewriteModule and may add references to these module export names.
806+
// * note: for references in code, not JSDoc, undefined variable checks will handle this.
807+
// TODO(b/144593112): remove this when ClosureRewriteModule always runs after typechecking
808+
if (ClosureRewriteModule.isModuleExport(rootOfType)
809+
|| ClosureRewriteModule.isModuleContent(rootOfType)) {
810+
typeRefNode.setString("UnrecognizedType_" + typeName);
811+
}
812+
}
813+
}
770814
}

‎test/com/google/javascript/jscomp/CheckJsDocTest.java

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_SUPPRESS;
2929

3030
import com.google.javascript.jscomp.parsing.Config.JsDocParsing;
31+
import org.junit.Before;
3132
import org.junit.Test;
3233
import org.junit.runner.RunWith;
3334
import org.junit.runners.JUnit4;
@@ -36,6 +37,15 @@
3637
@RunWith(JUnit4.class)
3738
public final class CheckJsDocTest extends CompilerTestCase {
3839

40+
private JsDocParsing jsdocParsingMode;
41+
42+
@Override
43+
@Before
44+
public void setUp() throws Exception {
45+
super.setUp();
46+
this.jsdocParsingMode = JsDocParsing.INCLUDE_DESCRIPTIONS_WITH_WHITESPACE;
47+
}
48+
3949
@Override
4050
protected CompilerPass getProcessor(final Compiler compiler) {
4151
return new CheckJSDoc(compiler);
@@ -45,7 +55,7 @@ protected CompilerPass getProcessor(final Compiler compiler) {
4555
protected CompilerOptions getOptions() {
4656
CompilerOptions options = super.getOptions();
4757
options.setWarningLevel(DiagnosticGroups.MISPLACED_SUPPRESS, CheckLevel.WARNING);
48-
options.setParseJsDocDocumentation(JsDocParsing.INCLUDE_DESCRIPTIONS_WITH_WHITESPACE);
58+
options.setParseJsDocDocumentation(jsdocParsingMode);
4959
options.setPreserveDetailedSourceInfo(true);
5060
return options;
5161
}
@@ -1204,4 +1214,31 @@ public void testPublicClassField_typeDefError() {
12041214
public void testPublicClassComputedField_typeDefError() {
12051215
testWarning("class C { /** @typedef {number} */ [x] = 2;}", MISPLACED_ANNOTATION);
12061216
}
1217+
1218+
@Test
1219+
public void testMangleClosureModuleExportsContentsTypes() {
1220+
// disable parsing anything besides types; otherwise this test case fails because the
1221+
// "sourceComment"s from the actual/expected JSDocInfo do not match
1222+
jsdocParsingMode = JsDocParsing.TYPES_ONLY;
1223+
1224+
test(
1225+
"/** @type {!module$exports$foo$bar} */ let x;",
1226+
"/** @type {!UnrecognizedType_module$exports$foo$bar} */ let x;");
1227+
test(
1228+
srcs(
1229+
"goog.module('foo.bar'); exports = class {};",
1230+
"/** @type {!module$exports$foo$bar} */ let x;"),
1231+
expected(
1232+
"goog.module('foo.bar'); exports = class {};",
1233+
"/** @type {!UnrecognizedType_module$exports$foo$bar} */ let x;"));
1234+
test(
1235+
"/** @type {!module$exports$foo$bar.A.B} */ let x;",
1236+
"/** @type {!UnrecognizedType_module$exports$foo$bar.A.B} */ let x;");
1237+
test(
1238+
"/** @type {!module$contents$foo$bar_local} */ let x;",
1239+
"/** @type {!UnrecognizedType_module$contents$foo$bar_local} */ let x;");
1240+
test(
1241+
"/** @type {!Array<module$exports$foo$bar>} */ let x;",
1242+
"/** @type {!Array<UnrecognizedType_module$exports$foo$bar>} */ let x;");
1243+
}
12071244
}

‎test/com/google/javascript/jscomp/integration/ClosureIntegrationTest.java

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import com.google.javascript.jscomp.VariableRenamingPolicy;
4545
import com.google.javascript.jscomp.WarningLevel;
4646
import com.google.javascript.jscomp.testing.JSChunkGraphBuilder;
47+
import com.google.javascript.jscomp.testing.JSCompCorrespondences;
4748
import com.google.javascript.jscomp.testing.TestExternsBuilder;
4849
import com.google.javascript.rhino.Node;
4950
import com.google.javascript.rhino.StaticSourceFile.SourceKind;
@@ -66,6 +67,116 @@ public final class ClosureIntegrationTest extends IntegrationTestCase {
6667

6768
private static final String CLOSURE_COLLAPSED = lines("var COMPILED = true;");
6869

70+
@Test
71+
public void testReferencingMangledModuleNames_rewriteModulesBeforeTypechecking() {
72+
CompilerOptions options = createCompilerOptions();
73+
options.setClosurePass(true);
74+
options.setCheckTypes(true);
75+
options.setBadRewriteModulesBeforeTypecheckingThatWeWantToGetRidOf(true);
76+
compile(
77+
options,
78+
new String[] {
79+
lines(
80+
"goog.module('foo.bar');",
81+
"class Inner {}",
82+
"class Outer extends Inner {}",
83+
"exports.Outer = Outer;"),
84+
lines(
85+
"/** @type {!module$exports$foo$bar} */ let err1;",
86+
"/** @type {!module$contents$foo$bar_Inner} */ let err2;")
87+
});
88+
assertThat(lastCompiler.getWarnings())
89+
.comparingElementsUsing(JSCompCorrespondences.DESCRIPTION_EQUALITY)
90+
.containsExactly(
91+
"Bad type annotation. Unknown type UnrecognizedType_module$exports$foo$bar",
92+
"Bad type annotation. Unknown type UnrecognizedType_module$contents$foo$bar_Inner");
93+
}
94+
95+
@Test
96+
public void testReferencingMangledModuleNames_rewriteModulesAfterTypechecking() {
97+
CompilerOptions options = createCompilerOptions();
98+
options.setClosurePass(true);
99+
options.setCheckTypes(true);
100+
options.setBadRewriteModulesBeforeTypecheckingThatWeWantToGetRidOf(false);
101+
compile(
102+
options,
103+
new String[] {
104+
lines(
105+
"goog.module('foo.bar');",
106+
"class Inner {}",
107+
"class Outer extends Inner {}",
108+
"exports.Outer = Outer;"),
109+
lines(
110+
"/** @type {!module$exports$foo$bar} */ let err1;",
111+
"/** @type {!module$contents$foo$bar_Inner} */ let err2;")
112+
});
113+
assertThat(lastCompiler.getWarnings())
114+
.comparingElementsUsing(JSCompCorrespondences.DESCRIPTION_EQUALITY)
115+
.containsExactly(
116+
"Bad type annotation. Unknown type UnrecognizedType_module$exports$foo$bar",
117+
"Bad type annotation. Unknown type UnrecognizedType_module$contents$foo$bar_Inner");
118+
}
119+
120+
@Test
121+
public void testFunctionDeclarationInGoogScope_usingGoogModuleGetType() {
122+
CompilerOptions options = createCompilerOptions();
123+
options.setClosurePass(true);
124+
options.setCheckTypes(true);
125+
options.setBadRewriteModulesBeforeTypecheckingThatWeWantToGetRidOf(true);
126+
127+
compile(
128+
options,
129+
new String[] {
130+
lines("goog.module('foo.bar.Types');", "exports.Klazz = class {};"),
131+
lines(
132+
"goog.requireType('foo.bar.Types');",
133+
"goog.scope(function() {",
134+
"const fooBarTypes = goog.module.get('foo.bar.Types');",
135+
"/** @param {!fooBarTypes.Klazz} k */",
136+
"function f(k) {}",
137+
"f(null);", // expect a type error here
138+
"});")
139+
});
140+
141+
assertThat(lastCompiler.getWarnings())
142+
.comparingElementsUsing(JSCompCorrespondences.DESCRIPTION_EQUALITY)
143+
.containsExactly(
144+
lines(
145+
"actual parameter 1 of $jscomp$scope$98477071$0$f does not match formal parameter",
146+
"found : null",
147+
"required: module$exports$foo$bar$Types.Klazz"));
148+
assertThat(lastCompiler.getErrors()).isEmpty();
149+
}
150+
151+
@Test
152+
public void testEsModuleInterop_esModuleUsingGoogRequireType() {
153+
CompilerOptions options = createCompilerOptions();
154+
options.setClosurePass(true);
155+
options.setCheckTypes(true);
156+
options.setBadRewriteModulesBeforeTypecheckingThatWeWantToGetRidOf(true);
157+
158+
compile(
159+
options,
160+
new String[] {
161+
lines("goog.module('foo.bar.Types');", "exports.Klazz = class {};"),
162+
lines(
163+
"const {Klazz} = goog.requireType('foo.bar.Types');",
164+
"/** @param {!Klazz} k */",
165+
"export function f(k) {}",
166+
"f(null);" // expect a type error here
167+
)
168+
});
169+
170+
assertThat(lastCompiler.getWarnings())
171+
.comparingElementsUsing(JSCompCorrespondences.DESCRIPTION_EQUALITY)
172+
.containsExactly(
173+
lines(
174+
"actual parameter 1 of f$$module$i1 does not match formal parameter",
175+
"found : null",
176+
"required: module$exports$foo$bar$Types.Klazz"));
177+
assertThat(lastCompiler.getErrors()).isEmpty();
178+
}
179+
69180
@Test
70181
public void testProcessDefinesInModule() {
71182
CompilerOptions options = createCompilerOptions();

0 commit comments

Comments
 (0)