Skip to content

Commit 4e01c6d

Browse files
concavelenzcopybara-github
authored andcommitted
Fix incorrect error location for some missing goog.require warnings.
This change fixes a bug where the "missing require" compiler warning pointed to the wrong location in the code. For an expression like `dom.TagName.B`, the warning incorrectly highlighted the entire expression instead of just the part with the missing dependency, `dom.TagName`. The fix addresses two distinct issues: 1. For qualified names in code, the pass now correctly traverses the `GETPROP` tree to find the specific sub-expression that matches the missing namespace. 2. For qualified names in JSDoc, the pass now calculates the correct length of the missing namespace prefix to ensure the error highlight is accurate. This change also includes a variety of unit tests for edge cases and to verify the correctness of the new `getComponentCount` implementations. PiperOrigin-RevId: 800515083
1 parent de2b7df commit 4e01c6d

File tree

4 files changed

+183
-4
lines changed

4 files changed

+183
-4
lines changed

src/com/google/javascript/jscomp/CheckMissingRequires.java

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,13 @@ private void checkMissingRequireThroughShortNameInGoogScope(
506506
}
507507
}
508508
checkNormalizedShortNameImport(
509-
t, n, currentFile, originalImport, normalizedQualifiedName, referenceStrength);
509+
t,
510+
n,
511+
currentFile,
512+
originalImport,
513+
qualifiedName,
514+
normalizedQualifiedName,
515+
referenceStrength);
510516
return;
511517
}
512518

@@ -529,7 +535,13 @@ private void checkMissingRequireThroughShortNameInGoogScope(
529535
}
530536
QualifiedName normalizedQualifiedName = swapRootOfQualifiedName(qualifiedName, aliasedName);
531537
checkNormalizedShortNameImport(
532-
t, n, currentFile, originalImport, normalizedQualifiedName, referenceStrength);
538+
t,
539+
n,
540+
currentFile,
541+
originalImport,
542+
qualifiedName,
543+
normalizedQualifiedName,
544+
referenceStrength);
533545
}
534546

535547
private void checkMissingRequireThroughShortNameInModule(
@@ -558,14 +570,21 @@ private void checkMissingRequireThroughShortNameInModule(
558570
QualifiedName normalizeQualifiedName = normalizeQualifiedNamePlusImport(qualifiedName, require);
559571

560572
checkNormalizedShortNameImport(
561-
t, n, currentFile, originalRequiredFile, normalizeQualifiedName, referenceStrength);
573+
t,
574+
n,
575+
currentFile,
576+
originalRequiredFile,
577+
qualifiedName,
578+
normalizeQualifiedName,
579+
referenceStrength);
562580
}
563581

564582
private void checkNormalizedShortNameImport(
565583
NodeTraversal t,
566584
Node n,
567585
ModuleMetadata currentFile,
568586
ModuleMetadata originalImport,
587+
QualifiedName originalQualifiedName,
569588
QualifiedName normalizedQualifiedName,
570589
Strength referenceStrength) {
571590
// TESTCASES, where A.B should not be used through A:
@@ -620,7 +639,33 @@ private void checkNormalizedShortNameImport(
620639
? MISSING_REQUIRE_IN_GOOG_SCOPE
621640
: MISSING_REQUIRE_TYPE_IN_GOOG_SCOPE;
622641
}
623-
t.report(n, toReport, namespace);
642+
643+
Node errorNode = n;
644+
// Get the right location if it is a code reference.
645+
int diff = normalizedQualifiedName.getComponentCount() - subName.getComponentCount();
646+
if (n.isQualifiedName()) {
647+
for (int i = 0; i < diff && errorNode.isGetProp(); i++) {
648+
errorNode = errorNode.getFirstChild();
649+
}
650+
t.report(NodeUtil.getRootOfQualifiedName(errorNode), errorNode, toReport, namespace);
651+
} else {
652+
// JSDoc reference case
653+
// Trim the original qualified name from the source text by the same difference.
654+
QualifiedName errorQName = originalQualifiedName;
655+
for (int i = 0; i < diff; i++) {
656+
errorQName = errorQName.getOwner();
657+
}
658+
659+
String correctName = errorQName.join();
660+
// For JSDoc, the entire qualified name is a single STRINGLIT node.
661+
// To report the error on the correct sub-expression, we create a new
662+
// temporary node with the correct string and length.
663+
Node newErrorNode = n.cloneNode();
664+
newErrorNode.setString(correctName);
665+
newErrorNode.setLength(correctName.length());
666+
667+
t.report(newErrorNode, toReport, namespace);
668+
}
624669
}
625670

626671
// We found the imported namespace: done

src/com/google/javascript/rhino/QualifiedName.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ public String join() {
128128
return sb.toString();
129129
}
130130

131+
public abstract int getComponentCount();
132+
131133
/**
132134
* Returns a new qualified name object with {@code this} name as the owner and the given string as
133135
* the property name.
@@ -176,6 +178,11 @@ public Iterable<String> components() {
176178
return terms.subList(0, size);
177179
}
178180

181+
@Override
182+
public int getComponentCount() {
183+
return size;
184+
}
185+
179186
@Override
180187
public boolean matches(Node n) {
181188
int pos = size - 1;
@@ -237,6 +244,11 @@ public boolean matches(Node n) {
237244
&& RhinoStringPool.uncheckedEquals(n.getString(), prop)
238245
&& owner.matches(n.getFirstChild());
239246
}
247+
248+
@Override
249+
public int getComponentCount() {
250+
return owner.getComponentCount() + 1;
251+
}
240252
}
241253

242254
/**
@@ -284,6 +296,17 @@ public String join() {
284296
public boolean matches(Node n) {
285297
return n.matchesQualifiedName(node);
286298
}
299+
300+
@Override
301+
public int getComponentCount() {
302+
int count = 1;
303+
Node current = this.node;
304+
while (current.isGetProp()) {
305+
count++;
306+
current = current.getFirstChild();
307+
}
308+
return count;
309+
}
287310
}
288311

289312
private static final String THIS = RhinoStringPool.addOrGet("this");

test/com/google/javascript/jscomp/CheckMissingRequiresTest.java

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -835,6 +835,67 @@ public void testWarning_missingRequireType_nestedProvideIndirectRef() throws Exc
835835
""");
836836
}
837837

838+
@Test
839+
public void testIndirectNamespaceRef_errorLocation() {
840+
test(
841+
srcs(
842+
"goog.provide('goog.dom');",
843+
"goog.provide('goog.dom.TagName'); goog.dom.TagName = {B: 'b'};",
844+
"""
845+
goog.module('test');
846+
const dom = goog.require('goog.dom');
847+
const x = dom.TagName.B;
848+
"""),
849+
warning(CheckMissingRequires.INDIRECT_NAMESPACE_REF_REQUIRE)
850+
.withMessage(
851+
"'goog.dom.TagName' should have its own goog.require.\n"
852+
+ "Please add a separate goog.require and use that alias instead.")
853+
.withLocation(3, 10, 11));
854+
}
855+
856+
@Test
857+
public void testIndirectNamespaceRef_jsdoc_errorLocation() {
858+
test(
859+
srcs(
860+
"goog.provide('goog.dom');",
861+
"""
862+
goog.provide('goog.dom.TagName');
863+
/** @const */
864+
goog.dom.TagName = {};
865+
goog.dom.TagName.C = class {};
866+
""",
867+
"""
868+
goog.module('test');
869+
const dom = goog.require('goog.dom');
870+
/** @type {dom.TagName.C} */
871+
const x = null;
872+
"""),
873+
warning(CheckMissingRequires.INDIRECT_NAMESPACE_REF_REQUIRE_TYPE)
874+
.withMessage(
875+
"'goog.dom.TagName' should have its own goog.requireType.\n"
876+
+ "Please add a separate goog.requireType and use that alias instead.")
877+
.withLocation(3, 11, 11));
878+
}
879+
880+
@Test
881+
public void testIndirectNamespaceRef_jsdoc_errorLocation_withFunction() {
882+
test(
883+
srcs(
884+
"goog.provide('goog.dom');",
885+
"goog.provide('goog.dom.TagName'); /** @enum {string} */ goog.dom.TagName = {B: 'b'};",
886+
"""
887+
goog.module('test');
888+
const dom = goog.require('goog.dom');
889+
/** @type {function(dom.TagName)} */
890+
const x = null;
891+
"""),
892+
warning(CheckMissingRequires.INDIRECT_NAMESPACE_REF_REQUIRE_TYPE)
893+
.withMessage(
894+
"'goog.dom.TagName' should have its own goog.requireType.\n"
895+
+ "Please add a separate goog.requireType and use that alias instead.")
896+
.withLocation(3, 20, 11));
897+
}
898+
838899
@Test
839900
public void testWarning_missingRequire_nestedModuleIndirectRef() throws Exception {
840901
checkIndirectNamespaceRefRequireWarning(
@@ -1983,4 +2044,24 @@ private void checkRequireForGoogModuleGetWarning(String namespace, String... js)
19832044
warning(CheckMissingRequires.MISSING_REQUIRE_FOR_GOOG_MODULE_GET)
19842045
.withMessageContaining("'" + namespace + "'"));
19852046
}
2047+
2048+
@Test
2049+
public void testIndirectNamespaceRef_jsdoc_errorLocation_longNamespace() {
2050+
test(
2051+
srcs(
2052+
"goog.provide('goog.dom');",
2053+
"goog.provide('goog.dom.tags');",
2054+
"goog.provide('goog.dom.tags.misc'); /** @const */ goog.dom.tags.misc = {FOO: 'foo'};",
2055+
"""
2056+
goog.module('test');
2057+
const dom = goog.require('goog.dom');
2058+
/** @type {dom.tags.misc.FOO} */
2059+
const x = null;
2060+
"""),
2061+
warning(CheckMissingRequires.INDIRECT_NAMESPACE_REF_REQUIRE_TYPE)
2062+
.withMessage(
2063+
"'goog.dom.tags.misc' should have its own goog.requireType.\n"
2064+
+ "Please add a separate goog.requireType and use that alias instead.")
2065+
.withLocation(3, 11, 13)); // dom.tags.misc
2066+
}
19862067
}

test/com/google/javascript/rhino/QualifiedNameTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,4 +193,34 @@ public void testMatch_fromGetprop() {
193193
assertThat(n.matches(qname(IR.superNode(), "qux"))).isTrue();
194194
assertThat(n.matches(qname(IR.name("x"), "qux"))).isFalse();
195195
}
196+
197+
@Test
198+
public void testEdgeCases_fromString() {
199+
assertThat(QualifiedName.of("").components()).containsExactly("");
200+
assertThat(QualifiedName.of(".").components()).containsExactly("", "");
201+
assertThat(QualifiedName.of(".foo.bar").components()).containsExactly("", "foo", "bar");
202+
assertThat(QualifiedName.of("foo.bar.").components()).containsExactly("foo", "bar", "");
203+
assertThat(QualifiedName.of("foo..bar").components()).containsExactly("foo", "", "bar");
204+
}
205+
206+
@Test
207+
public void testGetComponentCount_fromString() {
208+
assertThat(QualifiedName.of("foo").getComponentCount()).isEqualTo(1);
209+
assertThat(QualifiedName.of("foo.bar").getComponentCount()).isEqualTo(2);
210+
assertThat(QualifiedName.of("foo.bar.baz").getComponentCount()).isEqualTo(3);
211+
}
212+
213+
@Test
214+
public void testGetComponentCount_fromNode() {
215+
assertThat(IR.name("foo").getQualifiedNameObject().getComponentCount()).isEqualTo(1);
216+
assertThat(qname(IR.name("foo"), "bar").getQualifiedNameObject().getComponentCount())
217+
.isEqualTo(2);
218+
assertThat(qname(IR.name("foo"), "bar", "baz").getQualifiedNameObject().getComponentCount())
219+
.isEqualTo(3);
220+
}
221+
222+
@Test
223+
public void testGetComponentCount_fromGetprop() {
224+
assertThat(QualifiedName.of("foo").getprop("bar").getComponentCount()).isEqualTo(2);
225+
}
196226
}

0 commit comments

Comments
 (0)